CODEWITHSUNDEEP
javaswitch-statementsonarqubecyclomatic-complexity
Amar Magar
Amar Magar

Reputation: 881

Reduce Cyclomatic Complexity of Switch Statement - Sonar

I want to reduce cyclomatic complexity of my switch case my code is : 

public String getCalenderName() {
        switch (type) {
    case COUNTRY:
        return country == null ? name : country.getName() + HOLIDAY_CALENDAR;
    case CCP:
        return ccp == null ? name : ccp.getName() + " CCP" + HOLIDAY_CALENDAR;
    case EXCHANGE:
        return exchange == null ? name : exchange.getName() + HOLIDAY_CALENDAR;
    case TENANT:
        return tenant == null ? name : tenant.getName() + HOLIDAY_CALENDAR;
    default:
        return name;
    }
}

This code blocks complexity is 16 and want to reduce it to 10. country, ccp, exchange and tenant are my diffrent objects. Based on type I will call their respective method.

Upvotes: 14

Views: 31229

Answers (7)

Paul Jansen
Paul Jansen

Reputation: 1210

I think you can lower the complexity just by making sure that something else is fixed in your code.

Take the case:

case COUNTRY:
  return country == null ? name : country.getName() + HOLIDAY_CALENDAR;

This implies that if the calender type is COUNTRY, the country the calender is associated with could be null. This is something you should prevent by design because I can't see a reason why this could be a valid situation. Why would you have a country calender without a country?

So make sure that there is a non-null object associated with the calender as soon as you assign a type to it. In this way your cases will be like

case COUNTRY:
  return country.getName() + HOLIDAY_CALENDAR;

which lowers your cyclomatic complexity to 5.

Upvotes: 3

Amar Magar
Amar Magar

Reputation: 881

public String getName() {
    if (type == null) {
        return name;
    }
    if (type == BusinessCalendarType.COUNTRY) {
        return country == null ? name : country.getName() + HOLIDAY_CALENDAR;
    } else if (type == BusinessCalendarType.CCP) {
        return ccp == null ? name : ccp.getName() + " CCP" + HOLIDAY_CALENDAR;
    } else if (type == BusinessCalendarType.EXCHANGE) {
        return exchange == null ? name : exchange.getName() + HOLIDAY_CALENDAR;
    } else if (type == BusinessCalendarType.TENANT)  {
        return tenant == null ? name : tenant.getName() + HOLIDAY_CALENDAR;
    } else {
        return name;
    }
}

this worked for me

Upvotes: -4

Luk
Luk

Reputation: 2256

If your objects: country, cpp, exchange and tenant share the same interface e.g. ObjectWithGetName you could refactor your code as following: 

  public String getCalenderName() {
    ObjectWithGetNameMethod calendarType = null;
    switch (type) {
        case COUNTRY:
           calendarType = country;
           break;
        case CCP:
           calendarType = cpp;
           break;
        case EXCHANGE:
           calendarType = exchange;
           break;
        case TENANT:
           calendarType = tenant;
           break;
        default:
           calendarType = null;
    }
    return (calendarType != null ? (calendarType.getName() + HOLIDAY_CALENDAR) : name);
  }

Also I think it will be nice to move switch to separate method since it looks like something witch will be used in many different places.

Upvotes: 0

Loic Mouchard
Loic Mouchard

Reputation: 1141

If your first aim is only to reduce the cyclomatic complexity, you should create methods for each way of getting the name, like following.

 public String getCalenderName() {
    switch (type) {
    case COUNTRY:
        return getCountryName();
    case CCP:
        return getCcpName();
    case EXCHANGE:
        return getExchangeName();
    case TENANT:
        return getTenantName();
    default:
        return name;
    }
}

private String getCountryName() {
    return country == null ? name : country.getName() + HOLIDAY_CALENDAR;
}

private String getCcpName() {
    return ccp == null ? name : ccp.getName() + " CCP" + HOLIDAY_CALENDAR;
}

private String getExchangeName() {
    return exchange == null ? name : getName.toString() + HOLIDAY_CALENDAR;
}

private String getTenantName() {
    return tenant == null ? name : getName.toString() + HOLIDAY_CALENDAR;
}

Note in your specific example, I suppose that you have 1 class that gather (at least) 4 quite similar behaviours. A refactoring would certainly make more sense, in order to have for example one base implementation (abstract or not), and 4 other inherited classes.

Upvotes: 4

halil
halil

Reputation: 800

I believe it is a Sonar warning. I think Sonar warnings are not must-do-rules, but just guides. Your code block is READABLE and MAINTAINABLE as it is. It is already simple, but if you really want to change it you can try those two approaches below, and see if complexity becomes lower:

Note: I don't have compiler with me now so there can be errors, sorry about that in advance.

First approach:

Map<String, String> multipliers = new HashMap<String, Float>();
    map.put("country", country);
    map.put("exchange", exchange);
    map.put("ccp", ccp);
    map.put("tenant", tenant);

Then we can just use the map to grab the right element

    return map.get(type) == null ? name : map.get(type).getName() + HOLIDAY_CALENDAR;

2nd approach:

All your objects have same method, so you can add an Interface with getName() method in it and change your method signature like: 

getCalendarName(YourInterface yourObject){
    return yourObject == null ? name : yourObject.getName() + HOLIDAY_CALENDAR;
}

Upvotes: 12

sandeshch
sandeshch

Reputation: 11

You can remove all the null comparisons and check it prior to switch case. In that case complexity will reduce by 4 or may be more.

Upvotes: 0

mayank agrawal
mayank agrawal

Reputation: 658

As per my knowledge, do not use return statement in switch statement. Apply that logic after the switch statement using a variable. Create a method for checking the null value and call that method from switch then you will able to reduce the Cyclomatic Complexity

Upvotes: 1

Related Questions