Merge this if statement with the enclosing one

Question

I'm running my code on sonarqube, but it shows there's an issue with my code, saying"Merge this if statement with the enclosing one." I tried it, but still have no idea how to solve it.

if (splitStrings.length == 2) {
    if (!splitStrings[1].matches("\\d{1,3}") 
        || Integer.parseInt(splitStrings[1]) > 100 
        || Integer.parseInt(splitStrings[1]) < 1) {
            throw new IllegalArgumentException("Invalid Input");
    }
}

Answer 1

You could do three things:

1. Either you need to chain the conditions as below:

if (splitStrings.length == 2
    && (!splitStrings[1].matches("\\d{1,3}")
        || Integer.parseInt(splitStrings[1]) > 100
        || Integer.parseInt(splitStrings[1]) < 1)) {
    throw new IllegalArgumentException("Invalid Input");
}

2. You could declare local variable for Integer.parseInt(splitStrings[1]) inside the first if after adding the matches condition in it.

if (splitStrings.length == 2 && splitStrings[1].matches("\\d{1,3}")) {
    int val = Integer.parseInt(splitStrings[1]);
    if (val > 100 || val < 1)) {
        throw new IllegalArgumentException("Invalid Input");
    }
}

3. Even though i won't recommend it, you could also suppress the warning using //NOSONAR in the line in which warning is displayed or add @SuppressWarnings("squid:S1066") at the method level.

Answer 2

Try this.

if (splitStrings.length == 2
    && (!splitStrings[1].matches("\\d{1,3}")
        || Integer.parseInt(splitStrings[1]) > 100
        || Integer.parseInt(splitStrings[1]) < 1)) {
    throw new IllegalArgumentException("Invalid Input");
}

Answer 3

You should merge them by && between them and wrap the second with brackets, like:

if(splitStrings.length == 2 && (!splitStrings[1].matches("\\d{1,3}") || Integer.parseInt(splitStrings[1]) > 100 || Integer.parseInt(splitStrings[1]) < 1)){
    //code here
}

Answer 4

Integer.parseInt may be skipped altogether if a better regexp is provided to check for the digits in the range [1..100], thus the conditions could be simplified:

if (splitStrings.length == 2 && !splitStrings[1].matches("(?!0)\\d{1,2}|100")) {
    throw new IllegalArgumentException("Invalid Input");
}

Online demo of the regexp: (?!0)\d{1,2}|100 validating positive cases 0 < x <= 100

• (?!0)\d{1,2} - 1 or 2 digit numbers except 0 excluded with negative lookahead (?!0): 1 - 99
• |100 - OR 100

Answer 5

In my opinion the assertion that combining if statements makes code more readable is flawed. Often it makes sense to ask a series of questions in sequence, especially when each part is asking an essentially independent question.

This can (in direct opposition of the SonarQube assertion) make each part more readable. It also allows for correct placement of useful comments in cases where the meaning is not stupidly obvious.

Another valid reason for nested if statements would be to follow certain easily recognized coding patterns. An example would be in ASP.Net applications where Page_Load methods may often have an if (!PostBack) block, which can then contain additional logic that may include additional nested conditions. If there is only one nested condition and it is combined, later changes may need to undo that merge. (There may be a case at that point for refactoring the logic into a separate method at that point, but that is a different subject.)

The question of whether or not to combine nested if statements should be left to the discretion and good judgement of the author (and code reviewer) based on which form is more readable and/or maintainable.

In your particular case, your logic is asking two questions: (1) "Does the input have two parts?" and (2) "Is the second part valid"?

So yes, your nested if statements can be combined into a single (but logically more complex) statement, but this is not necessarily better.