javaoptimizationcode-complexity

How can I reduce a Cognitive Complexity of its method?


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;
    }


Solution

  • 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;
    }