Reducing the cyclomatic complexity, multiple if statements java

Question

I have a lot more of these else-if conditions that I have not included. How could I refactor this to reduce the cyclomatic complexity?

if (ONE.equalsIgnoreCase(eachTag.getNodeName()))
{
    myclassDto.setOne(formatter
            .getElementValueAfterNullCheckWithTrim((Element) eachTag));
}
else if (TWO.equalsIgnoreCase(eachTag.getNodeName()))
{
    myclassDto.setTwo(formatter
            .getElementValueAfterNullCheckWithTrim((Element) eachTag));
}
else if (THREE.equalsIgnoreCase(eachTag.getNodeName()))
{
    myclassDto.setThree(formatter
            .getElementValueAfterNullCheckWithTrim((Element) eachTag));
}
else if (FOUR.equalsIgnoreCase(eachTag.getNodeName()))
{
    myclassDto.setFour(formatter
            .getElementValueAfterNullCheckWithTrim((Element) eachTag));
}
else if (FIVE.equalsIgnoreCase(eachTag.getNodeName()))
{
    myclassDto.setFive(formatter
            .getElementValueAfterNullCheckWithTrim((Element) eachTag));
}
else if (SIX.equalsIgnoreCase(eachTag.getNodeName()))
{
    myclassDto.setSix(formatter
            .getElementValueAfterNullCheckWithTrim((Element) eachTag));
}

How can I reduce the cyclomatic complexity of this function in java?

SurfMan · Answer 1 · 2017-12-20T12:35:59.580

2

Your code would be easier to read, while it's just as "complex" (while it's not really that complex), if you:

Use a switch statement
Extract the value of getNodeName() beforehand

Extract the value returned by getElementValueAfterNullCheckWithTrim() beforehand

String value = formatter.getElementValueAfterNullCheckWithTrim((Element) eachTag);
String nodeName = eachTag.getNodeName();

switch (nodeName) {
    case ONE:
        myclassDto.setOne(value);
        break;
    case TWO:
        myclassDto.setTwo(value);
        break;
    ...
}

Edit: You might want to refactor your DTO so it's easier to use, like

  myclassDto.setValue(nodeName, value)

edited Dec 20 '17 at 12:35

answered Dec 20 '17 at 12:30

SurfMan

1,685
12
19

The DTO refactoring part is super important considering his actual architecture. – Yassine Badache Dec 20 '17 at 12:33
when I use switch reduce complexity 19 to 17. But need to reduce to 10. – user3431624 Dec 21 '17 at 04:02
Like I said in my answer, using `switch` is just as "complex" as using `if`. It just more readable. And what about refactoring the DTO? If your DTO is not efficient in accepting data, i.e. it has 100 set***() methods, I doubt you will get a lower complexity number. – SurfMan Dec 21 '17 at 08:23

Michael · Answer 2 · 2017-12-20T13:00:58.553

1

I'd define a mapping of Strings to the functions they correspond with. This can be static.

Then you can loop over the mappings, find the appropriate one (filter) and apply the function.

private static final Map<String, BiConsumer<MyClassDto, Element>> MAPPING = new HashMap();

static
{
    mapping.put(ONE, MyClassDto::setOne);
    mapping.put(TWO, MyClassDto::setTwo);
    //...
    mapping.put(SIX, MyClassDto::setSix);
}


//...

MAPPING.entrySet().stream()
    .filter(pair -> pair.getKey().equalsIgnoreCase(eachTag.getNodeName()))
    .map(Map.Entry::getValue)
    .forEach(func -> func.accept(
        myClassDto,
        formatter.getElementValueAfterNullCheckWithTrim((Element) eachTag)
    ));

You may want to consider the case where MAPPING can contain different keys which are treated equally by equalsIgnoreCase (e.g. "AAA" and "aaa").

One solution is to use findFirst().ifPresent() in place of forEach (as suggested by daniu) but this may mask some error cases so use it with caution.

edited Dec 20 '17 at 13:00

answered Dec 20 '17 at 12:36

Michael

41,989
11
82
128

That would have been my approach, but you can't do `forEach` with equivalent result; more than one mappings may get triggered and more than one `BiConsumer` called. Hence, `findFirst().ifPresent()`. Also, `getNodeName` outside of the stream maybe. – daniu Dec 20 '17 at 12:41
@daniu The issue I have with that is that `findFirst` might mask some hard to find bugs. If you have two mappings with different case Strings (say, "AAA" and "aaa"), and given that `HashMap` does not have predictable iteration order, using `findFirst` may result in swapping back and forth between each function. This would be incredibly hard to track down. If you want assurance that there are no two "equal" mappings, you'd be better off to write a function which iterates over the keys and checks for duplicates. – Michael Dec 20 '17 at 12:49
Agreed, but if you add "aaa" and "AAA" with your code, you'll call both `BiConsumer`s. Were they to eg add the match to a database with a uniqueness constraint, that would fail. I'd rather use `findFirst` with a `LinkedHashMap` for `MAPPING` (which I think provides deterministic iteration). Not that any of this matters for OP`s simple case ;) – daniu Dec 20 '17 at 12:55
@daniu Exactly. In that case, my example would fail fast and yours would be fail-safe. I would immediately know something was wrong and you would be blissfully unaware. :) – Michael Dec 20 '17 at 12:57

Reducing the cyclomatic complexity, multiple if statements java

2 Answers2