SonarLint's rule java:S135 says:
Loops should not contain more than a single "break" or "continue" statement The use of break and continue statements increases the complexity of the control flow and makes it harder to understand the program logic. In order to keep a good program structure, they should not be applied more than once per loop. This rule reports an issue when there is more than one break or continue statement in a loop. The code should be refactored to increase readability if there is more than one.
And gives this example:
for (int i = 1; i <= 10; i++) { // Noncompliant; two "continue" statements
if (i % 2 == 0) {
continue;
}
if (i % 3 == 0) {
continue;
}
// ...
}
Should be refactored to:
for (int i = 1; i <= 10; i++) {
if (i % 2 == 0 || i % 3 == 0) {
continue;
}
// ...
}
But it looks weird. I have this test example:
@Slf4j
public class TestService {
public int test(List<String> input) {
int count = 0;
for (String s : input) {
if ("ONE".equals(s)) {
log.info("one was called");
processOne(s);
continue;
}
String something = getSomethingBy(s);
if (something == null) {
log.info("something is null");
sendEmail();
continue;
}
if ("TWO".equals(s) && "hz".equals(something)) {
process(s);
count++;
}
}
return count;
}
and sonarLint claims:
If I change this code to:
public int testGoodForSonarLint(List<String> input) {
int count = 0;
for (String s : input) {
if ("ONE".equals(s)) {
log.info("one was called");
processOne(s);
} else {
String something = getSomethingBy(s);
if (something == null) {
log.info("something is null");
sendEmail();
} else if ("TWO".equals(s) && "hz".equals(something)) {
process(s);
count++;
}
}
}
return count;
}
It looks so bad. I hate that if-else chin and hell of {}. The code looks like a Christmas tree. And it is with 2 continue. In real code, I have 4. So, it is not always possible to refactor as SonarLint suggests:
if (i % 2 == 0 || i % 3 == 0) {
continue;
}
Because I need to do something in each condition, not just continue.
Maybe I don't understand something but interapting cycle by continue or break looks much better and readable than brackets hell. What do you think, is it possible to rewrite this code? if yes, how would you refactor my example?
You could create another method for an iteration and return
in it.
Then, it would not be jumping around within the method but only stopping execution of the method.
@Slf4j
public class TestService {
public int test(List<String> input) {
int count = 0;
for (String s : input) {
count = processString(s, count);
}
return count;
}
private int processString(String s, int count){
if ("ONE".equals(s)) {
log.info("one was called");
processOne(s);
return count;
}
String something = getSomethingBy(s);
if (something == null) {
log.info("something is null");
sendEmail();
return count;
}
if ("TWO".equals(s) && "hz".equals(something)) {
process(s);
count++;
}
return count;
}
}
or even better, don't pass count
as a parameter:
@Slf4j
public class TestService {
public int test(List<String> input) {
int count = 0;
for (String s : input) {
count += processString(s);
}
return count;
}
private int processString(String s){
if ("ONE".equals(s)) {
log.info("one was called");
processOne(s);
return 0;
}
String something = getSomethingBy(s);
if (something == null) {
log.info("something is null");
sendEmail();
return 0;
}
if ("TWO".equals(s) && "hz".equals(something)) {
process(s);
return 1;
}
return 0;
}
}
(you could also return a boolean
and increment count
iff it is true
)
But at the end, whether or not you like this code is your choice. This way doesn't eliminate exiting out of the if
but it gets rid of going to other positions within the current method which can be confusing.