Opened on 02/28/2018 at 08:11:09 PM

Closed on 08/29/2019 at 05:43:18 PM

#6433 closed change (rejected)

Use generators to return lists of things

Reported by: mjethani Assignee:
Priority: Unknown Milestone:
Module: Platform Keywords: closed-in-favor-of-gitlab
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.

Attachments (0)

Change History (9)

comment:1 Changed on 02/28/2018 at 08:14:16 PM by mjethani

  • Cc agiammarchi sergz sebastian hfiguiere fhd greiner kzar added

comment:2 Changed on 02/28/2018 at 08:16:32 PM by mjethani

  • Description modified (diff)

comment:3 Changed on 03/01/2018 at 10:01:26 AM by mjethani

  • Description modified (diff)

comment:4 Changed on 03/01/2018 at 10:03:50 AM by mjethani

  • Description modified (diff)

comment:5 Changed on 03/01/2018 at 10:16:16 AM by mjethani

  • Description modified (diff)

comment:6 Changed on 03/01/2018 at 10:59:01 AM by mjethani

  • Blocked By 6434 added

comment:7 Changed on 03/01/2018 at 11:13:29 AM by mjethani

  • Blocked By 6434 removed

comment:8 Changed on 03/01/2018 at 11:38:15 AM by mjethani

  • Description modified (diff)

comment:9 Changed on 08/29/2019 at 05:43:18 PM by sebastian

  • Keywords closed-in-favor-of-gitlab added
  • Resolution set to rejected
  • Status changed from new to closed

Sorry, but we switched to GitLab. If this issue is still relevant, please file it again in the new issue tracker.

Add Comment

Modify Ticket

Change Properties
Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from (none).
 
Note: See TracTickets for help on using tickets.