Site-www: Loosen Effective Dart forEach advice to allow chaining?

[pq]

Following up on https://github.com/dart-lang/linter/issues/365, it would be great to settle whether the Effective Dart advice on forEach could be loosened to allow for functional-style chaining (e.g. a .where().forEach()) as proposed by @zoechi and mores recently by @sjmcdowall in https://github.com/dart-lang/linter/issues/365#issuecomment-458194690.

@munificent?

[sjmcdowall]

Just wanted to add my 2 cents in that this is an excellent programming technique as it leads to cleaner and more comprehensible code. I wouldn't even mind doing a PR on the second if there is a documented procedure on how to do that :)

[munificent]

I don't think using forEach() ever leads to particularly better code. But I do think allowing it at the end of a method chain is reasonable. The guideline is already soft ("AVOID" instead of "DON'T"), so how about we just add another exception, like:

One exception is if all you want to do is invoke some already existing function with each element as the argument. In that case, forEach() is handy.

Another is at the end of a long method chain that doesn't gracefully fit inside a for-in statement.

[kwalrath]

Could someone point me to a good, succinct, running example (maybe a dartpad/gist) that demonstrates this? That would increase the odds that this change gets in soon.

[pq]

I don't think using forEach() ever leads to particularly better code.

Challenge accepted. 😄

How about:

request.definitions?.forEach(_server.stdin.writeln);

(from fluttter_tools/.../compile.dart)

or

type.typeArguments?.arguments?.forEach(_resolveTypeName);

(from analyzer/.../resolver.dart)

vs... ?

(Actually, I'm not sure these are necessarily _better_, but certainly more terse and I think not _worse_ than their alternatives. WDYT?)

[sjmcdowall]

I was actually thinking more along the lines of something like array function chaining...

    .filter((Task xxx) => xxx.type == 2)
    .map((Task yyy) => yyy.readyPayload())
    .sort((Task a,Task  b) => a.compareTo(b))
    .forEach((result) {
        switch(result.command) {
            case 'transmit': {
                Xmiter.send(result.payload);
                break;
           }
           case 'dump': {
               print(result);
               break;
           }
           default : {
               Queue.insert(result);
               break;
         }
    }
  };

OK -- grant you -- rather contrived -- and I'm not sure it's syntactically correct .. but it's close..but I think it gets the idea?

[munificent]

@pq, your two examples already fall under the existing exception because you aren't passing a lambda to forEach().

@sjmcdowall, something along the lines of your example is a good one. Basically a case where you have a method chain too big to stick after the in in a for-in loop and where the last thing you do is a block of code executed for its side effect.

[sjmcdowall]

OK -- soooo -- are we stuck here?

[munificent]

Not stuck, but no one's put work into moving it forward. Can you come up with a smaller but more complete looking example that we can show as an exception? Yours is partway there, but something with real types and only a few lines long would be better. 😄

[sjmcdowall]

@munificent -- I can certainly try -- but where do I post such a thing? Here?

[munificent]

Yes, here is fine. :)

[dramos07]

Any updates on this? this is currently blocking dart-lang/linter#365

[munificent]

On further thought, I don't think we're helping users by loosening this guideline. If you have a long method chain that produces an Iterable and you need to walk that iterable and do some kind of side effect, I think it's clearer to simply use a local variable and a for-in loop. So this example would become:

var tasks = myArray
    .filter((Task xxx) => xxx.type == 2)
    .map((Task yyy) => yyy.readyPayload())
tasks.sort((Task a,Task  b) => a.compareTo(b));
for (var result in tasks) {
  switch(result.command) {
    case 'transmit':
      Xmiter.send(result.payload);
      break;
    case 'dump':
      print(result);
      break;
    default:
      Queue.insert(result);
      break;
  }
}

I think that's better than:

(myArray
    .filter((Task xxx) => xxx.type == 2)
    .map((Task yyy) => yyy.readyPayload())
    ..sort((Task a,Task  b) => a.compareTo(b)))
    .forEach((result) {
      switch(result.command) {
        case 'transmit':
          Xmiter.send(result.payload);
          break;
        case 'dump':
          print(result);
          break;
        default:
          Queue.insert(result);
          break;
      }
    });

My reasons:

• It's less error-prone. (Note that the original example has a syntax error. @sjmcdowall forgot the closing ) at the end. Also note that sort() can't be used in a method chain.)
• There's less indentation and less nesting.
• The formatting is more obvious and less likely to be annoying. (dartfmt doesn't always do what you want when there are long method chains and big nested lambdas.)
• You can do more powerful control flow inside the body. You can use return or break to interact with control flow outside of the loop.
• Keeping the rule simpler gives users less to think about and less to argue about in code reviews. If we say "long" method chains are the exception, then people have to agree on what "long" means.

So I think we should just keep the rule as it is. How does that sound?

[sjmcdowall]

I still disagree @munificent -- to me this sounds like YOUR opinion on how things should go. Yes, you have some good arguments but I don't believe they are worth taking away an option for a programmer to use a feature available in the ecosystem. I thought some reasonable (and simple) use cases to allow this were given -- yes, for a long complex chain maybe it's not the best (although who I am to dictate that anyway) but for simpler chains this is still annoying in my opinion.

Of course, the above is just my opinion -- but I'm more a libertarian programmer. :)

[munificent]

I still disagree @munificent -- to me this sounds like YOUR opinion on how things should go.

Well, it's my opinion and the opinions of anyone persuaded by my arguments. An automated formatter is basically a collection of consensus opinions.

but I'm more a libertarian programmer. :)

That's entirely valid, but in that case, an opinionated automated formatter might not be the right tool for you. :) dartfmt is not a libertarian tool. Its philosophy is that we are all more productive in aggregate when we agree to do things the same way, based on the assumption that we will frequently work with each others' code.

If the roads are empty, it doesn't matter which side you drive on, and you can leave it up to personal preference. But when traffic gets heavy, it's better for everyone to have established rules for which side to drive on, even when the rule itself is arbitrary.

But if you're driving on your own private road, by all means don't use dartfmt and drive how you like.