How can I reduce a Cognitive Complexity of its method?

Viewed 168

I have an method to convert roman numbers to common decimal. I used here an one for cycle and a lot of "if" conditions. SonarLint in my IDE says me that Cognitive Complexity of this method is 33 while 15 is allowed. How can I reduce this? I do not mind how can I fix this. Waiting for your reccomendations!

public static int roman2Decimal(String roman) {
        int decimal = 0;
        char previous = 0;

        for (int x = 0; x < roman.length(); x++) {
            if (roman.charAt(x) == 'I')
                decimal += 1;

            if (roman.charAt(x) == 'V') {
                System.out.println(previous);
                if (previous == 'I') {
                    decimal -= 2;
                }
                decimal += 5;
            }

            if (roman.charAt(x) == 'X') {
                if (previous == 'I') {
                    decimal -= 2;
                }
                decimal += 10;
            }

            if (roman.charAt(x) == 'L') {
                if (previous == 'X') {
                    decimal -= 20;
                }
                decimal += 50;
            }

            if (roman.charAt(x) == 'C') {
                if (previous == 'X') {
                    decimal -= 20;
                }
                decimal += 100;
            }

            if (roman.charAt(x) == 'D') {
                if (previous == 'C') {
                    decimal -= 200;
                }
                decimal += 500;
            }

            if (roman.charAt(x) == 'M') {
                if (previous == 'C') {
                    decimal -= 200;
                }
                decimal += 1000;
            }
            previous = roman.charAt(x);
        }
        return decimal;
    }

2 Answers

The first step would be replacing the repeated if statements with a switch :

public static int roman2Decimal(String roman) {
    int decimal = 0;
    char previous = 0;


    for (int x = 0; x < roman.length(); x++) {
        switch (roman.charAt(x)) {
            case 'I':
                decimal += 1;
                break;
            case 'V':
                if (previous == 'I') {
                    decimal -= 2;
                }
                decimal += 5;
                break;
            case 'X':
                if (previous == 'I') {
                    decimal -= 2;
                }
                decimal += 10;
                break;
            case 'L':
                if (previous == 'X') {
                    decimal -= 20;
                }
                decimal += 50;
                break;
            case 'C':
                if (previous == 'X') {
                    decimal -= 20;
                }
                decimal += 100;
                break;
            case 'D':
                if (previous == 'C') {
                    decimal -= 200;
                }
                decimal += 500;
                break;
            case 'M':
                if (previous == 'C') {
                    decimal -= 200;
                }
                decimal += 1000;
                break;
        }
        previous = roman.charAt(x);
    }
    return decimal;
}

If we go further with refactoring, we might notice other repeated patterns. Using an enum would help making this method much more concise:

enum RomanDigit {
    ZERO(0, null), // sentinel
    I(1, ZERO),
    V(5, I),
    X(10, I),
    L(50, X),
    C(100, X),
    D(500, C),
    M(1000, C);

    public final int inc;
    public final RomanDigit prev;

    RomanDigit(int inc, RomanDigit prev) {
        this.inc = inc;
        this.prev = prev;
    }
}

public static int roman2Decima2l(String roman) {
    int decimal = 0;
    RomanDigit previous = RomanDigit.ZERO;
    for (char c : roman.toCharArray()) {
        RomanDigit current = RomanDigit.valueOf(String.valueOf(c));
        if (previous.equals(current.prev)) {
            decimal -= 2 * previous.inc;
        }
        decimal += current.inc;
        previous = current;
    }
    return decimal;
}

Switch case is much more appropriate for this task as only one option can be true, or if you want to stick to “if” then do if else so you don’t need to check all of the option but only until you find the correct one.

Related