AtomicInteger & lambda expressions in single-threaded app-CodePudding

I need to modify a local variable inside a lambda expression in a JButton's ActionListener and since I'm not able to modify it directly, I came across the AtomicInteger type.

I implemented it and it works just fine but I'm not sure if this is a good practice or if it is the correct way to solve this situation.

My code is the following:

newAnchorageButton.addActionListener(e -> {
    AtomicInteger anchored = new AtomicInteger();
        
    anchored.set(0);
    
    cbSets.forEach(cbSet ->
        cbSet.forEach(cb -> {
            if (cb.isSelected())
                anchored.incrementAndGet();
        })
    );
        
    // more code where I use the 'anchored' variable...
}

I'm not sure if this is the right way to solve this since I've read that AtomicInteger is used mostly for concurrency-related applications and this program is single-threaded, but at the same time I can't find another way to solve this.

I could simply use two nested for-loops to go over those arrays but I'm trying to reduce the method's cognitive complexity as much as I can according to the sonarlint vscode extension, and leaving those for-loops theoretically increases the method complexity and therefore its readability and maintainability.

Replacing the for-loops with lambda expressions reduces the cognitive complexity but maybe I shouldn't pay that much attention to it.

CodePudding user response：

While it is safe enough in single-threaded code, it would be better to sum them up in a functional way, like this:

int anchored = cbSets.stream()  // get a stream of the sets
    .flatMap(List::stream)      // flatten to list of cb's
    .mapToInt(cb -> cb.isSelected() ? 1 : 0)  // and convert to int
    .reduce(0, Math::addExact); // sum them up

Note: we never mutate any values; instead, we combine each successive value with the preceding one, producing a new value. It is not very important in this example, but instead of simply using .sum() to get the total, I prefer using a reduction with Math::addExact so that it will throw an ArithmeticException in the case of overflow. But of course, you're never going to have that many checkboxes.

CodePudding user response：

and leaving those for-loops theoretically increases the method complexity and therefore its readability and maintainability.

This is obvious horsepuckey. x.forEach(foo -> bar) is not 'cognitively simpler' than for (var foo : x) bar; - you can map each AST node straight over from one to the other.

If a definition is being used to define complexity which concludes that one is significantly more complex than the other, then the only correct conclusion is that the definition is silly and should be fixed or abandoned.

To make it practical: Yes, introducing AtomicInteger, whilst performance wise it won't make one iota of difference, does make the code way more complicated. AtomicInteger's simple existence in the code suggests that concurrency is relevant here. It isn't, so you'd have to add a comment to explain why you're using it. Comments are evil. (They imply the code does not speak for itself, and they cannot be tested in any way). They are often the least evil, but evil they are nonetheless.

The general 'trick' for keeping lambda-based code cognitively easily followed is to embrace the pipeline:

You write some code that 'forms' a stream. This can be as simple as list.stream(), but sometimes you do some stream joining or flatmapping a collection of collections.
You have a pipeline of operations that operate on single elements in the stream and do not refer to the whole or to any neighbour.
At the end, you reduce (using collect, reduce, max - some terminator) such that the reducing method returns what you need.

The above model (and the other answer follows it precisely) tends to result in code that is as readable/complex as the 'old style' code, and rarely (but sometimes!) more readable, and significantly less complicated. Deviate from it and the result is virtually always considerably more complicated - a clear loser.

Not all for loops in java fit the above model. If it doesn't fit, then trying to force that particular square peg into the round hole will take a lot of effort and almost always results in code that is significantly worse: Either an order of magnitude slower or considerably more cognitively complicated.

It also means that it is virtually never 'worth' rewriting perfectly fine readable non-stream based code into stream based code; at best it becomes a percentage point more readable according to some personal tastes, with no significant universally agreed upon improvement.

Turn off that silly linter rule. The fact that it considers the above 'less' complex, and that it evidently determines that for (var foo : x) bar; is 'more complicated' than x.forEach(foo -> bar) is proof enough that it's hurting way more than it is helping.

CodePudding user response：

I have the following to add to the two other answers:

Two general good practices in your code are in question:

Lambdas shouldn't be longer than 3-4 lines
Except in some precise cases, lambdas of stream operations should be stateless.

For #1, consider extracting the code of the lambda to a private method for example, when it's getting too long. You will probably gain in readability, and you will also probably gain in better separating UI from business logic.

For #2, you are probably not concerned since you are working in a single thread at the moment, but streams can be parallelized, and they may not always execute exactly as you think it does. For that reason, it's always better to keep the code stateless in stream pipeline operations. Otherwise you might be surprised.

More generally, streams are very good, very concise, but sometimes it's just better to do the same with good old loops. Don't hesitate to come back to classic loops.

When Sonar tells you that the complexity is too high, in fact, you should try to factorize your code: split into smaller methods, improve the model of your objects, etc.