CODEWITHSUNDEEP
javasonarqubestatic-analysis
daemon54
daemon54

Reputation: 1115

What is the best way to handle code with if/switch statements having high cyclomatic complexity?

I have a method that takes an input called capability id. Based on the capability id, I execute the business implementations put forward in form of a switch case. I have my functionality working, But came across a problem where the sonar report is showing high cyclomatic complexity around 12-14. The company I work for, uses a standard of 10 as the max cyclomatic complexity. My considerations is that, If I happen to break the code into too many block, the code readability is getting affected.

Note:- I don't have permissions to change sonar rules.

Unfortunately, I can't share the code. The code would be in the following way though,

// Dependency Injected
private someService;

public void processCapability(..., String capabilityId) {
    switch (capabilityId) {
        case ORDER_DISPENSED_WITH_SOURCE1:
            someService.doDispense1();
        case ORDER_DISPENSED_WITH_SOURCE2:
            someService.doDispense2();
        case ORDER_REJECTED:
            someService.doReject();
        case ORDER_CANCEL:
            someService.doCancel();
        case ORDER_PURGE:
            someService.doPurge();
            ...
            default: throw exception
    }
}

Update: I was able to resolve my problem, with a solution I posted in below answers.

Upvotes: 1

Views: 2676

Answers (4)

daemon54
daemon54

Reputation: 1115

I have figured one way to handle the big if/switch statements. Based on the ideas, I got from everyone of you. I compiled into a simple solution that is easy to understand and modify, also handles the complexity part. Please find my solution below,

// Dependency Injected
private someService;

public void processCapability(..., String capabilityId) {

  Boolean isCapabilityProcessed = processDispenseCapabilities(...) || processUpdateCapabilities(..);

  if(isCapabilityProcessed) {
    throw exception("Invalid Capability");
  }
}

private Boolean processDispenseCapabilities(..,String capabilityId) {

  Boolean result = false;

  switch (capabilityId) {
        case ORDER_DISPENSED_WITH_SOURCE1:
            someService.doDispense1();
            result = true;
        case ORDER_DISPENSED_WITH_SOURCE2:
            someService.doDispense2();
            result = true;
        case ORDER_REJECTED:
            someService.doReject();
            result = true;
            ...
        default: //do nothing
    }

  return result;
}

private Boolean processUpdateCapabilities(..,String capabilityId) {
  Boolean result = false;

    switch (capabilityId) {
        case ORDER_CANCEL:
            someService.doCancel();
            result = true;
        case ORDER_PURGE:
            someService.doPurge();
            result = true;
            ...
        default: //do nothing
    }

  return result;
}

Upvotes: 0

Spotted
Spotted

Reputation: 4091

I suggest that you refactor this switch statement (which is a code smell) using polymorphism. A possible way to do it is something like that:

public interface Order {
    void processCapability(...);
}

public final class RejectedOrder implements Order {
    private final SomeService someService;

    public RejectedOrder(SomeService someService) {
        this.someService = someService;
    }

    @Override
    public void processCapability(...) {
        someService.doReject();
    }
}

public final class CancelledOrder implements Order {
    private final SomeService someService;

    public RejectedOrder(SomeService someService) {
        this.someService = someService;
    }

    @Override
    public void processCapability(...) {
        someService.doCancel();
    }
}

More useful resources talking about the 'switch smell':

Upvotes: 1

Bouke
Bouke

Reputation: 1614

If there is nothing wrong with your code, why make it more complex only to avoid SonarQube warnings? You might not be allowed to change SonarQube rules, but you certainly can suppress the warning by annotating your method with the following:

@SuppressWarnings("squid:MethodCyclomaticComplexity")

Upvotes: 0

abbath
abbath

Reputation: 2482

There are multiple ways to avoid large if-else statements.

Maybe the most common patterns are the template method pattern and the strategy pattern.

As you haven't provided any code, it's hard to help you to lower the cyclomatic complexity, but I'm absolutely sure, there's a way to avoid those if-else-s.

I'd suggest you to read a bit about the topic; I'll provide you some sources I've found useful:

  1. Using strategy to replace if else
  2. Using command pattern to replace conditional logic
  3. I'd encourage you to use enums, they are more usable in conditional logic than you'd think: using enums instead of switch
  4. Also it's good to know about various design patterns, maybe you can find a suitable one here

Also a readable code means it's easy to understand. I agree with @Erwin Bolwidt's comment, if you create more methods with good names, it will be easier to read. To read more about the topic, check the book "Clean Code" by Robert C. Martin.

Upvotes: 1

Related Questions