Using loops

[yole]

Prefer using higher-order functions (filter, map etc.) to loops. Exception: forEach (prefer using a regular for loop instead).

[cypressious]

Use cases for forEach:

• Nullable receiver: list?.forEach {}
• Receiver is a return value that shouldn't be bound to a variable: list.filter {}.forEach {}

[voddan]

Another use case for forEach:

I have a collection list of N elements, and I want to execute an action N times. My options are:

• repeat(list.size) { /*do stuff*/ }
• list.forEach { /*do stuff*/ }

The latter is more preferable if list is unassigned: list.filter {}.forEach {}

[brentwatson]

Another elegant solution for nullable lists would be:

for (item in list.orEmpty()) { ... }

[matklad]

What about map/forEach combinations?

Currently, IDE suggest to replace

for (fieldDecl in fieldsToAdd) {
    val field = psiFactory.createStructExprField(fieldDecl.name!!)
    expr.addFieldBefore(field, null)
}

with

fieldsToAdd
    .map { psiFactory.createStructExprField(it.name!!) }
    .forEach { expr.addFieldBefore(it, null) }

Is this indeed the Kotlin style? I would personally prefer a loop for computations with side effects.

[valentinkip]

The reason why IDE suggests this, is that it does not know anything about side-effects. So that's up to the developer, of course, whether to use a loop or chained calls.

[matklad]

is that it does not know anything about side-effects.

I believe this is not true in this particular case, because it is specifically about.forEach, which exists only for side effects :)

Perhaps replacing loop with forEach could be an intention, and not a weak warning?

[valentinkip]

Perhaps replacing loop with forEach could be an intention, and not a weak warning?

It is an intention. But in the example above it's not simple replacing of loop with forEach, it's replacement with a call chain (which includes also map { }). That's why it's a weak warning. The reason for this difference is simple: just an intention does not show up visually and the user won't notice that this particular loop can be replaced with a call chain. This is not needed for simple replacement with forEach because every loop can be replaced with forEach.

[matklad]

Certainly! My point is that replacing a loop with a call chain with a .forEach as a termination operation is almost never a good idea, so it should not be a weak warning. FWIW, I would prefer a warning to go in the opposite direction, and to suggest to replace .forEach with a for loop.

But that of course depends on what style is preferred in Kotlin. If .forEach style is more conventional, I sure can live with it, but my current choice is

Exception: forEach (prefer using a regular for loop instead).

[valentinkip]

@matklad But why it's a bad idea? I don't understand. And BTW the suggestion is not even a weak warning, it's "info".

[matklad]

And BTW the suggestion is not even a weak warning, it's "info".

Yep, I was wrong here, it's indeed "info"!

@matklad But why it's a bad idea?

This depends on what Kotlin style is preferred, so this really is two questions:

1. What "style" is better: xs.map { f(it) }.forEach { doStuff(it) } or for (x in xs) { val y = f(x); doStuff(y) }

2. Should IDE issue info suggestions which violate the preferred style?

The second question is a no-brainier: IDE should nudge user in the direction of the Kotlin style, and not in the opposite one!

The first question is unresolved yet, but looks like the status quo is "use a for loop" (this is stated in the description of the issue). I personally also find that for loops are better in this context, because they are more readable (item names comes before collection name), clearly signal to the reader "beware of sideffects" and use less powerful and more familiar language machinery.

[valentinkip]

@matklad The current style guide (see https://github.com/JetBrains/kotlin-web-site/blob/636fe9e9d799eaa1e5c9d02b838f45aab1a13af2/pages/docs/reference/coding-conventions.md) says:

Prefer using higher-order functions (filter, map etc.) to loops. Exception: forEach (prefer using a regular for loop instead, unless the receiver of forEach is nullable or forEach is used as part of a longer call chain).

The above case is exactly the case when forEach is used as part of a longer call chain (I mean the code after suggested transformation).

[valentinkip]

@matklad Actually sometimes a call chain is easier to read and understand and sometimes it is not. Sometimes also the call chain is not good because of performance or side-effects. Unfortunately the IDE cannot be smart enough to evaluate the above criterias and make the correct decision instead of the developer.

[matklad]

@valentinkip thanks for pointing to the update in the style guide! It indeed settles the issue completely!

[NightlyNexus]

Should arrays be an exception in some cases? Some of the high-order-functions create collections which is can be an unexpected performance hit in some code.

The conventions do say, "understand the cost of the operations being performed in each case and keep performance considerations in mind," so maybe that's already covered.

[valentinkip]

@NightlyNexus But IDE cannot know whether performance does matter in the particular code. From my experience in 90% it does not. Only 10% (or less) of code is performance-critical.