[S1612] "Lambdas should be replaced with method references" should not be suggested if it would result in longer code

[daniel-frak (Daniel Frąk)]

SonarQube version: Community Edition Version 7.9.2 (build 30863)

What steps will reproduce the issue?

Consider this example class:

public static class SomeVeryLongAndComplicatedClassName {

        public void doThing() {
            Collections.singletonList("test").forEach(s -> print(s));
        }

        public static void print(String str) {
            System.out.println(str);
        }
    }

Currently, SonarQube suggests replacing s -> print(s) with a method reference, which would result in the following code:

Collections.singletonList("test").forEach(SomeVeryLongAndComplicatedClassName::print);

What is the expected result?

SonarQube shouldn’t suggest replacing lambdas with method references if the result would be longer, less readable code (per Joshua Bloch’s advice in “Effective Java”).

[Quentin (Quentin Jaquier)]

I agree with the advice of Joshua Bloch’s, but how do you reflect this in the check? Keeping only the length is not enough, think about:

forEach(s -> print(s))
forEach(NotSoLong::print)
forEach(something -> print(something))

Which one will you pick? How can you justify that the check reports the last one, but not the first one?
This is probably why he added “less readable”. But readability is subjective, I cannot see a good heuristic making sense here…

In addition, having such a long class name is not a good idea in itself, if you don’t have this bad pattern in your code, you will not face this other issue. Adding an exception is kind of accepting that a long class name is fine.

All in all, I don’t see any obvious change that we can apply to the check, I would therefore advise you to resolve it as won’t fix (or rename the class?).
Of course, if you see a better alternative, feel free to describe it further here!

Best,
Quentin

[daniel-frak (Daniel Frąk)]

I don’t see a problem with the first example, the third example is longer than the second one, which to me justifies being refactored into the first or the second. In terms of readability, in my opinion the first is the most readable, the second is less readable and the third is least readable.

If readability is subjective, then the S1612 rule should not exist at all. How else can you justify its existence, if not readability? The rule itself states:

Method/constructor references are more compact and readable than using lambdas, and are therefore preferred.

How can it claim that method references are more readable, if readability is subjective? And because this rule exists, that subjectivity is further proof that Sonarqube shouldn’t be so gung-ho with slapping warnings for it.

Additionally, the direct quote from Joshua Bloch’s summary is:

Where method references are shorter and clearer, use them; where
they aren’t, stick with lambdas.

Going by typical programming assumptions, a method reference should be both clearer and shorter for it to be considered a good choice.

I know this sounds a bit anal but obviously there is no way to objectively define what code is “clearer” programmatically, so I think the length of code is a good enough heuristic.

It is definitely a much better one than simply always suggesting the change, regardless of the length of code. Checking the length will result in less false positives than the current method.

As for “having such a long class name”, that class name was obviously an example. A more realistic one would be a class called ValidationErrorResponse - in my opinion that name is not too long.
I will forever maintain that .map(ValidationErrorResponse::extractValidationError) is less readable than .map(e -> extractValidationError(e)). As you can see, I am facing this issue, even while having a reasonable class name. Yes, the e -> part is fluff, but so is ValidationErrorResponse::. Both have to be ignored while trying to discern what the code does, but the earlier is easier to ignore than the latter due to the length.

The length check is the obvious check. It’s better for Sonarqube to not report a minor violation of readability than fill the dashboard with false positives which have to be manually checked off. A code checking tool should never chastise its user for writing good code.

All in all, just because the check will not be 100% perfect does not mean it should not be implemented. I’m not advocating for a “Method reference should be replaced with lambda” rule here, I’m merely asking for Sonarqube not to punish me for writing clear, (most often) more readable code.

P.S.

Adding an exception is kind of accepting that a long class name is fine.

Class name length has nothing to do with this rule. If long class names are not fine (which I actually agree with), then that should be a separate SonarQube rule.

[daniel-frak (Daniel Frąk)]

A real life example I just came across - this is a piece of code for handling aggregations in Spring Data Elasticsearch:

List<String> keys = buckets.stream()
                .map(b -> b.getKeyAsString())
                .collect(toList());

Replacing the lambda with a method reference will result in:

List<String> keys = buckets.stream()
                .map(MultiBucketsAggregation.Bucket::getKeyAsString)
                .collect(toList());

The method reference is hardly more readable and I don’t have the option of refactoring Spring’s libraries (from which MultiBucketsAggregation.Bucket comes) for my own use. I think this is a great example of how S1612 can punish for writing readable code.

[Lena (Elena Vilchik)]

Hi Daniel,

Thank you for your contribution trying to improve this rule. We discussed this case inside the team working on Java analyzer and while we admit that sometimes some developers might prefer keeping lambda, we don’t think this can/should be generalized.

We updated a bit description of the rule to be explicit about the fact that this refactoring is not always good (https://jira.sonarsource.com/browse/RSPEC-1612), but that’s it what we want to do to the rule.