Opened 18 months ago

Last modified 18 months ago

#6433 new change

Use generators to return lists of things

Reported by: mjethani Assignee:
Priority: Unknown Milestone:
Module: Platform Keywords:
Cc: agiammarchi, sergz, sebastian, hfiguiere, fhd, greiner, kzar Blocked By:
Blocking: #6425 Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description (last modified by mjethani)

Background

Sometimes we have code like this:

function getURLsFromElement(element)
{
  let urls = [];

  if (element.src)
    urls.push(element.src);

  if (element.srcset)
  {
    for (let url of element.srcset.split(","))
      urls.push(url.trim());
  }

  for (let child of element.children)
    urls.push(...getURLsFromElement(child));

  return urls;
}

This is a function that takes an input and returns a list, in this case a list of URLs from the given element.

ES6 has support for generators, which makes it possible to write the same code in a cleaner and possibly more efficient way.

For example:

function* getURLsFromElement(element)
{
  if (element.src)
    yield element.src;

  if (element.srcset)
  {
    for (let url of element.srcset.split(","))
      yield url.trim();
  }

  for (let child of element.children)
    yield* getURLsFromElement(child);
}

On the calling side the only change required is to iterate over the result, for example:

  let urls = [...getURLsFromElement(element)];

There is often a for...of loop on the calling side, which already works with generators, so there's no need for any change. If the caller terminates the iteration, the entire generator chain returns, which has the added benefit of improving performance.

For example, in the following code:

for (let url of getURLsFromElement(element))
{
  if (url)
  {
    doSomething(url);
    break;
  }
}

The loop terminates if the first URL returned is truthy, and in that case the function getURLsFromElement does not bother checking the value of the srcset attribute and the attribute values of the children. Without generators, the callee would need to know the requirements of the caller up front in order to do this kind of optimization, but with generators the caller decides when to terminate the callee.

This is already an improvement, but we can go further in terms of code readability. For example, by adopting a functional style:

function* getURLsFromElement(element)
{
  if (element.src)
    yield element.src;

  if (element.srcset)
    yield* element.srcset.split(",").map(url => url.trim());

  for (let urls of [...element.children].map(getURLsFromElement))
    yield* urls;
}

Note that this does negate some of the aforementioned performance benefits, since now we're trimming every URL before it's even needed.

Further, if yielding falsy values is not a concern, this can be combined with object destructuring in the parameter list to make the code even more succinct:

function* getURLsFromElement({children: [...children], src, srcset = ""})
{
  yield src;
  yield* srcset.split(",").map(url => url.trim());

  for (let urls of children.map(getURLsFromElement))
    yield* urls;
}

With each change we may degrade the performance slightly in exchange for better readability.

Also note that this only works transparently with pure functions, i.e. functions that have no side effects, or where the side effects and the order in which they occur do not matter.

Also see #6434.

What to change

Find functions that return arrays that might be suitable candidates to be replaced with generators and make the appropriate changes.

Whether or not a specific change of this type is warranted, and to what extent, should be evaluated on a case-by-case basis.

Change History (8)

comment:1 Changed 18 months ago by mjethani

  • Cc agiammarchi sergz sebastian hfiguiere fhd greiner kzar added

comment:2 Changed 18 months ago by mjethani

  • Description modified (diff)

comment:3 Changed 18 months ago by mjethani

  • Description modified (diff)

comment:4 Changed 18 months ago by mjethani

  • Description modified (diff)

comment:5 Changed 18 months ago by mjethani

  • Description modified (diff)

comment:6 Changed 18 months ago by mjethani

  • Blocked By 6434 added

comment:7 Changed 18 months ago by mjethani

  • Blocked By 6434 removed

comment:8 Changed 18 months ago by mjethani

  • Description modified (diff)
Note: See TracTickets for help on using tickets.