CODEWITHSUNDEEP
javacyclomatic-complexity
Abhilash
Abhilash

Reputation: 895

Improve the cyclomatic complexity code for if-else statements

I have a block of if-else statements in a method for which I am getting cyclomatic complexity issue. I have tried using switch statement for this but still the issue remains.

if (fileName.startsWith(HwErrorsEtlConstants.MR_RAW_GRADIENT_ERRORS)) {
    method1()...
} else if (fileName.startsWith(HwErrorsEtlConstants.MR_RAW_PFEI_ERRORS)) {
    method2()...
} else if (fileName.startsWith(HwErrorsEtlConstants.MR_RAW_RFAMP_ERRORS)) {
    method3()...
} and so on...

public static void method1(IOutputWriter writer, String fileName, InputStream fileInputStream) throws IOException {
        //logic for the method
    }

Upvotes: 2

Views: 1429

Answers (3)

Lino
Lino

Reputation: 19926

Edit: as you mentioned you have checked Exceptions that can be thrown and arguments in your method. As a Runnable doesn't declare that it can throw Exceptions and also doens't accept any parameters you have to create your own FunctionalInterface (click here to see what they really are):

public interface ThrowingRunnable {
    void run(IOutputWriter writer, String fileName, InputStream fileInputStream) throws IOException;
}

Then you just have to replace Runnable with ThrowingRunnable in my earlier proposed code below and you should be fine.

You can create a mapping of HwErrorsEtlConstants to the specific method (uses java 8):

static final Map<String, Runnable> MAPPING;
static {
    Map<String, Runnable> temp = new HashMap<>();
    temp.put(HwErrorsEtlConstants.MR_RAW_GRADIENT_ERRORS, this::method1);
    temp.put(HwErrorsEtlConstants.MR_RAW_PFEI_ERRORS, this::method2);
    temp.put(HwErrorsEtlConstants.MR_RAW_RFAMP_ERRORS, this::method3);
    MAPPING = Collections.unmodifiableMap(temp);
}

Then in your method you can use Streams introduced also in Java 8:

// Optional indicates a potentially absent value, it's just a wrapper around a Runnable
Optional<Runnable> optional = MAPPING
    // create a Stream from the entries
    .entrySet().stream()
    // keep the items that match the condition and drop every other
    .filter(e -> filename.startsWith(e.getKey()))
    // we had Map.Entry<String, Runnable>, but now we only need the value e.g. the Runnable
    .map(Map.Entry::getValue)
    // short circuit, e.g. we only want the first value that matches
    .findFirst();

// checks if anything is present, this is used as the MAPPING "could" be empty
if(optional.isPresent()) {
    // unpack the value and call it with arguments
    optional.get().run(aWriter, someFileName, anInputStream);
} else {
    // nothing matched, throw error or log etc.
}

Though as has been mentioned, your current solution does look fine, I guess you're using Sonar for code analysis. Sometimes Sonar just has false positives, so you can also safely ignore them.

Further reads to help you understand Java 8:

Upvotes: 3

Gaurav Srivastav
Gaurav Srivastav

Reputation: 2551

You can solve this via Java 8 approach using Predicate & Function functional interfaces. You have to put all the conditions through predicates and in Function, you can add implementation which needs to be executed when condition matches.

Map<Predicate,Function<String,String>> map = new HashMap<>();

    Predicate<String> p = (fileName)->fileName.startsWith(HwErrorsEtlConstants.MR_RAW_GRADIENT_ERRORS);

  Function<String,String> method = (input)->{ return "output";}; 

   map.put(p,method);
    Optional<String> result =
    map.entrySet()
    .stream()
    .filter(entry->entry.getKey().test(fileName))
    .map(entry->entry.getValue().apply())
    .findFirst();

OR 

  map.entrySet()
    .stream()
    .filter(entry->entry.getKey().test(fileName))
    .forEach(entry->entry.getValue().apply());

Upvotes: 1

Joop Eggen
Joop Eggen

Reputation: 109547

The cyclomatic complexity is one issue: indeed cycles.

Then there is the splitting of the flow on a property using if/switch, which is not object-oriented. It also may violate separation of concerns: having many methods handling entirely different smaller aspects.

If the number of fields is larger, and the number of lines is large, consider extracting classes handling one aspect.

For reduction of the cyclomatic complexity, check to control flow of the callers, repetitions of the same call, practical duplicate code. And then try to (re-)move cycles. Best is when you can put cycles together; and work on Lists/Sets/Streams.

Upvotes: 2

Related Questions