Improve the cyclomatic complexity code for if-else statements

Question

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
    }

Using [Array of function pointers in Java - Stack Overflow](https://stackoverflow.com/questions/2752192/array-of-function-pointers-in-java) may help. — user202729, May 29 '19 at 08:51
Is this code hard to read or understand? If not, just don't care. The cyclomatic complexity indicator *can* point to code that is too complex because it has a lot of nested branches. In this case, the code looks very straightforward to me. Focus on the important stuff. The indicator is a tool, and just a tool. — JB Nizet, May 29 '19 at 08:52
OOP. Create an interface with two methods: one to provide the prefix, one to do the methodXs. Create 3 implementations. Loop over the implementations, find the one with the matching prefix, execute the methodX. — Michael, May 29 '19 at 08:52

Lino · Accepted Answer · 2019-05-29T10:26:22.957

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:

My methods1,2,3.. are declared to throw checked exceptions. How can I handle them with this approach? — Abhilash, May 29 '19 at 09:46
Why use Map when you are not actually using it to look something up by a key? It could just be an array of tuples. Also, the fact that the constants are passed to `startsWith` means they can only be String, not enum. — DodgyCodeException, May 29 '19 at 09:49
@DodgyCodeException I didn't use a map because I wanted constant lookup time, as you can't really look up a "lambda", but instead have pairs of some `String`s to a method. — Lino, May 29 '19 at 09:56
@Abhilash28Abhi See my updated answer, you can't use `Runnable` but you can just create your own `interface` that declares the `Exception` that can be thrown — Lino, May 29 '19 at 10:01
If the constants are all the same length, then you _can_ look up the method by key! (Just use `filename.substring(0, constantLength)` as the key.) That wasn't specified in the OP, so we don't know if it can be done. But it's something the OP could consider. @Abhilash28Abhi — DodgyCodeException, May 29 '19 at 10:05
@Lino I was able to make progress with the approach now. One problem i am facing is that the methods are taking arguments and with the above code I get compilation error. Have modified the original code for reference — Abhilash, May 29 '19 at 10:09
@DodgyCodeException good point on which I only can agree. But as OP hasn't specified this. I didn't consider it at all — Lino, May 29 '19 at 10:23
@Lino Awesome works fine for me now :) Also I found this article which clearly describes all different approaches we can follow https://www.baeldung.com/java-replace-if-statements — Abhilash, May 29 '19 at 11:40

score 2 · Answer 2 · answered May 29 '19 at 09:51

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.

Gaurav Srivastav · Answer 3 · 2019-05-29T10:15:14.403

1

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());

edited May 29 '19 at 10:15

answered May 29 '19 at 08:57

Gaurav Srivastav

2,381
1
15
18

Can you please elaborate a bit for Predicate and Function class. How to go about it, I am a bit new to Java 8 – Abhilash May 29 '19 at 09:06
I have just updated the answer, please have a look. – Gaurav Srivastav May 29 '19 at 09:10
1

It would help if you added the argument to the `filter(...)` method, and also the `forEach(...)` call takes a `Map.Entry` argument so it doesn't have an `apply` method. – DodgyCodeException May 29 '19 at 09:20

Improve the cyclomatic complexity code for if-else statements

3 Answers3