11.1 - allow for-of loops in some cases?

[mjomble]

Example code:

for (const handler of requestHandlers) {
    const response = handler.handle(request);

    if (response) {
        sendResponse(response);
        break;
    }

    debug('did not match', handler);
}

@Vladisls converting to higher-order function doesn't seem to add much value and hurts readability in my opinion:

let isHandled = false;

requestHandlers.forEach((handler) => {
    if (isHandled) {
        return;
    }

    const response = handler.handle(request);

    if (response) {
        sendResponse(response);
        isHandled = true;
    } else {
        debug('did not match', handler);
    }
})

You could shorten the code by doing something like this:

requestHandlers.some((handler) => {
    const response = handler.handle(request);

    if (response) {
        sendResponse(response);
        return true;
    }

    debug('did not match', handler);
})

But I would consider that a hack 😛 And it wouldn't help much with readability.

PS. I totally support using map/filter/find etc (and some in its originally intended use), the only one that bugs me in some cases is forEach 🙂

[Vladisls]

I agree that there could be a possibility that forEach would be more readable/easier to understand in some circumstance, but preferring Higher-order functions could be more beneficial if used in conjunction with good function names.

eg ->

const respondToFirstSuccessfulRequest = (handler) => {
    const response = handler.handler(request);

    if (!response) {
        return true;
    }

    sendResponse(response);
    return false;
}

requestHandlers.every(respondToFirstSuccessfulRequest);

The every() function behaves exactly like forEach(), except it stops iterating through the array whenever the callback function returns a falsy value.

Developer who does not know how every works should not be bothered with it because the function name describes the actual intent.

PS. Not knowing how array functions work is not an excuse. By preferring Higher-order functions we would increase the mean knowledge of our developers.

[mihkeleidast]

I'd like there to be some better real-life examples under this rule so the code clarity benefit would be clearer. I generally agree that the way Vlad rewrote your example is great - but probably wouldn't have come up with that myself.

[mjomble]

Higher-order functions could be more beneficial...

Could you name some benefits of this specific example that the equivalent for-of example doesn't have?

---

PS. I would argue that respondToFirstSuccessfulRequest is not a good name for this function - it describes what the every call does when it uses this function, but not what the function itself does.

For example, requestHandlers.some(respondToFirstSuccessfulRequest) would also look like it responds to the first successful request, while in fact it does the opposite. So even though the name of the function is very readable, I'd say the pattern as a whole is not, and can easily lead to mistakes.

[mjomble]

In general, I think Array.some and Array.every should be used only for their original intent - to check whether the contents of an array match certain conditions. The callbacks passed to them should be pure functions without side effects and the code should actually use the returned boolean.

Using these functions for special cases of iteration with side effects is, in my opinion, a dirty hack.

We could, however, make our own alternatives, like forEachUntilFalse(array, callback) or something like that.

[Vladisls]

1. Benefits are that the way you iterate the array is not relevant when trying to understand what the code does. Code should just say what is its intent.
2. In terms of readability both .some(respondToFirstSuccessfulRequest) and .every(respondToFirstSuccessfulRequest) are equivalent. Functionally they work differently but that should not pass developers own testing phase and thus not relevant in the later stages of code lifecycle.

[mjomble]

Benefits are that the way you iterate the array is not relevant when trying to understand what the code does. Code should just say what is its intent.

Not sure if I understand what you mean. Would these two examples be equivalent because they both say what their intent is, despite iterating the array differently?

const respondToFirstSuccessfulRequest = (handlers, request) => {
  // code that iterates over handlers using a for-of loop
}

const respondToFirstSuccessfulRequest = (handlers, request) => {
  // code that iterates over handlers using a higher-order function
}

Or would the higher-order function example bring additional benefits?

[Vladisls]

Benefits are that the way you iterate the array is not relevant when trying to understand what the code does. Code should just say what is its intent.

Not sure if I understand what you mean. Would these two examples be equivalent because they both say what their intent is, despite iterating the array differently?

const respondToFirstSuccessfulRequest = (handlers, request) => {
  // code that iterates over handlers using a for-of loop
}

const respondToFirstSuccessfulRequest = (handlers, request) => {
  // code that iterates over handlers using a higher-order function
}

Or would the higher-order function example bring additional benefits?

yes - they are the same in terms of readability.

In this particular case using Higher order function would not give any benefits due to JS engines not prioritizing higher-order functions #DifferentEngines.

[mjomble]

In this particular case using Higher order function would not give any benefits due to JS engines not prioritizing higher-order functions #DifferentEngines.

Given that this is a JS-specific styleguide, would it be ok to allow both for-of and higher-order functions for iteration? (While still recommending/enforcing higher-order functions for .map(), .find() etc?)

[mihkeleidast]

This may be just a naming thing, but @mjomble's example of using respondToFirstSuccessfulRequest as the name of the wrapper function makes a lot more sense. I'd have no idea on how to name the HOF appropriately and Vlad's example naming does not look appropriate right now.

One of the most important impacts of these guidelines is to ease the code review process. Currently, it seems to me, reviewing these kinds of complicated uses of array methods is definitely not easy. But I'm still open for more real-life examples where they make more sense (if there are any).

[Vladisls]

I'm not sure if we wanted to enforce this rule with a linter or not. The reason why I want to keep it "Always use higher..." is that this way developers will think twice before using simple for loops. We have other rules that are enforced with linters but we have always allowed to write a comment with a reasoning - why you should bypass the rule here and disable linter for few lines.

Most of the times developers just use simple for loops as they don't know any better. Being more strict about using HOF we would force developers to learn and adapt and with time they will also know how to detect these corner cases where using HOF is not the most reasonable way.

[mjomble]

Most of the times developers just use simple for loops as they don't know any better. Being more strict about using HOF we would force developers to learn and adapt and with time they will also know how to detect these corner cases where using HOF is not the most reasonable way.

I agree. It seems we're thinking of two options:

1. Make the wording very strict ("Always use HOF. Never use for-of.") and assume developers know that exceptions with comments are implicitly allowed
2. Be more explicit about the exceptions, e.g. "Always use HOF. Experienced developers may use for-of loops in these exceptions. But when in doubt, use HOF."

I prefer option 2