Reduce complexity of 14 for a method

Question

Pmd told me this method (thirdRowsValidation) has a complexity of 14 but I cannot reach a patter to reduce the code.

indexBookngEnd, indexTravelStart ... all those variables are indexes from other array made in another loop iteration (Header of the columns of a csv file) - Ref1

public void thirdRowsValidation(String thirdRowCsv) {
        String[] lines1 = thirdRowCsv.split(",");
        for (int i = 0; i < lines1.length; i++) {

            if (indexBookngEnd == i && "".equals(temporalValidateBookngEnd)) {
                temporalValidateBookngEnd = (" to " + lines1[i] + "\n");

            }
            if (indexBookngStart == i
                    && !("".equals(temporalValidateBookngEnd))) {
                finalOutput.append("Then it should have booking window ");

                indexBookngStart = -1;

            }

            if (indexTravelEnd == i && "".equals(temporalValidateTravelEnd)) {
                temporalValidateTravelEnd = (" to " + lines1[i] + "\n");

            }

            if (indexTravelStart == i
                    && !("".equals(temporalValidateTravelEnd))) {

                finalOutput.append("Then it should have travel window ");

                String idHeaderColumn = String.format("%1$-" + 5 + "s", "id");
                String typeHEaderColumn = String.format("%1$-" + 50 + "s","type");
                finalOutput.append("| ");
                finalOutput.append(idHeaderColumn);

                indexTravelStart = -1;
            }

            if (indexPackageDescription == i) {
                temporalPackages = String.format("%1$-" + 50 + "s", lines1[i]);

            }

            if (indexPackageCode == i
                    && !(lines1[i].matches("[+-]?\\d*(\\.\\d+)?"))
                    && indexTravelStart == -1) {

                finalOutput.append("| ");


            }

        }

    }

Ref1:

public void secondRowValidation(String secondRowCsv) {

        String[] lines1 = secondRowCsv.split(",");
        for (int i = 0; i < lines1.length; i++) {

            if ("Bookng start".equalsIgnoreCase(lines1[i])) {
                indexBookngStart = i;
            }
            if ("Bookng end".equalsIgnoreCase(lines1[i])) {
                indexBookngEnd = i;
            }

Array from \n and later for ","

public String getStoryFromCsv(String convert) {
        String[] lines = convert.split("(\n)");
        for (int j = 0; j < lines.length; j++) {

            arrayPerRow = lines[j];
            if (j == 0) { // get marketing type and number
                firstRowValidation(arrayPerRow);
            }
            if (j == 1) { // get headers
                secondRowValidation(arrayPerRow);
            }
            if (j > 1) { // get info and append according to headers
                thirdRowsValidation(arrayPerRow);
            }

        }

So what I have is: - The method thirdRowsValidation() has an NPath complexity of 649 - The method 'thirdRowsValidation' has a Cyclomatic Complexity of 14.

At the end I'm reaching a text like this just for you guys have an idea:

Then it should have booking window 8/8/16 to 10/8/16
Then it should have travel window 11/6/16 to 12/25/16
And it should have online packages:
| id    | type                                              |
| 34534 | aaa Pkg                                           |
| D434E | MKW Pkg + asdasdasdasdasdasdas                    |
| F382K | sds Pkg + Ddding                                  |
| X582F | OYL Pkg + Deluxe Dining                           |

You should start by extracting the contents of the loops into separate methods. That reduces complexity. Then, given that you seem to have quite many fields or global variables, that may be a bit refactored by moving that state into a separate object, making further refactorings a bit easier since you can move that state-object around. But these are only guesses, so start by whatever your IntelliJ suggests :-) — Dominik Sandjaja, Jul 21 '16 at 05:50
I understand that... Thats what i made for example here... if (j == 0) { firstRowValidation(arrayPerRow); } but as you can see there are two main loops needed which I cannot change — arnoldssss, Jul 21 '16 at 05:54
But you could move everything from line 4 to the last but third line into a method, so that your `thirdRowsValidation` would only have the `for`-loop and then call a method for every one of the `i`s. — Dominik Sandjaja, Jul 21 '16 at 05:56
I couldn't understand well.. could you please explain with code how the ifs will be? — arnoldssss, Jul 21 '16 at 05:59

score 2 · Accepted Answer · answered Jul 21 '16 at 13:18

The complexity of the method is that high because it is doing a lot of different things. Try doing one thing per method.

public String getStoryFromCsv(String csv) {
    StringBuilder story = new StringBuilder();
    String[] lines = csv.split("\n”);
    appendBookingWindowValidation(story, lines[0]);
    appendTravelWindowValidation(story, lines[1]);
    appendOnlinePackageValidation(story, lines);
    return story.toString();
}

private void appendBookingWindowValidation(StringBuilder story, String firstLine) {
    story.append("Then it should have booking window ");
    // extract start and end date from 'firstLine'
    // and append them to the story
}

private void appendTravelWindowValidation(StringBuilder story, String secondLine) {
    story.append("Then it should have travel window ");
    // extract start and end date from 'secondLine'
    // and append them to the story
}

private void appendOnlinePackageValidation(StringBuilder story, String[] lines) {
    story.append("And it should have online packages:\n")
         .append("| id    | type                                              |\n");
    for (int i = 2 ; i < lines.length; i++) {
      // create and append a row of the online packages table
   }
}

Try to pass everything a method needs as one of its argument. A method should not rely on the value of a field that is set in a different method. This keeps the complexity down and also makes the code easier to read, understand and test.

If a method or class has a high complexity then it usually means that it tries to do too many different things at once. Take a step back and try to identify the different things it does and implement them separately. This will automatically result in code with a low complexity.

Extracting an inner loop into a new method usually is a quick hack that will help reduce the complexity of a single method but not the complexity of the class as a whole.

score 1 · Answer 2 · answered Jul 21 '16 at 06:05

You could start by moving the inner logic of the for-loop into a separate method:

public void thirdRowsValidation(String thirdRowCsv) {
    String[] lines1 = thirdRowCsv.split(",");
    for (int i = 0; i < lines1.length; i++) {
      doSomethingWithTheRow(i, lines[i]);
    }
}

And inside the doSomethingWithTheRow() method, your inner code would reside:

doSomethingWithTheRow(int i, String row) {
    if (indexBookngEnd == i && "".equals(temporalValidateBookngEnd)) {
        temporalValidateBookngEnd = (" to " + row + "\n");

    }
    if (indexBookngStart == i
                && !("".equals(temporalValidateBookngEnd))) {
    ...

Not sure if this reduces the complexity to a level which you find acceptable, but it is a first start. Also, it is a Clean Code principle as defined by Uncle Bob. You have small methods, doing one thing (the method now only extracts single rows and then calls other methods to do something with that row) and which are, well, short. Namely, the SRP (Single Responsibility Principle) and KISS (Keep It Simple, Stupid) principles.

Reduce complexity of 14 for a method

2 Answers2