CODEWITHSUNDEEP
javacomplexity-theoryreduce
user3431624
user3431624

Reputation: 311

Reducing the cyclomatic complexity, multiple if statements java

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?

Upvotes: 1

Views: 2321

Answers (2)

Michael
Michael

Reputation: 44090

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.

Upvotes: 1

SurfMan
SurfMan

Reputation: 1803

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)

Upvotes: 2

Related Questions