How do I reduce the "Cyclomatic Complexity" of the following code

Question

I am wondering how to reduce the Cyclomatic Complexity of the following code and if this is even something that I should be worried about.

Please refer to the method ValuePojo.getSomething() (Please don't worry about the variable naming, this has been re-written for clarity in this question)

public class ValuePojo
{
    private ValueTypeEnum type;

    private BigDecimal    value1;

    private BigDecimal    value2;

    private BigDecimal    value3;

    public ValuePojo()
    {
        super();
    }

    /**
     * This method reports as "HIGH Cyclomatic Complexity"
     * 
     * @return
     */
    public BigDecimal getSomething()
    {
        if (this.type == null)
        {
            return null;
        }

        switch (this.type)
        {
            case TYPE_A:
            case TYPE_B:
            case TYPE_C:
            case TYPE_D:
                return this.value1;

            case TYPE_E:
            case TYPE_F:
            case TYPE_G:
            case TYPE_H:
                return this.value2;

            case TYPE_I:
            case TYPE_J:
                return this.value3;
        }

        return null;
    }
}

11 I think, just high enough to trigger the condition in Sonar but not to a crazy level. — Corey Scott, Oct 09 '12 at 04:37

score 8 · Answer 1 · answered Oct 09 '12 at 05:01

8

If you really need to get the cyclomatic complexity down, you can consider using a Map. Obviously, in your implementation, it should only be created and initialized once.

  public BigDecimal getSomething() {
      if (this.type == null) {
          return null;
      }
      Map<Type,BigDecimal> map = new HashMap<Type,BigDecimal>();
      map.put(TYPE_A, value1);
      map.put(TYPE_B, value1);
      map.put(TYPE_C, value1);
      map.put(TYPE_D, value1);
      map.put(TYPE_E, value2);
      map.put(TYPE_F, value2);
      map.put(TYPE_G, value2);
      map.put(TYPE_H, value2);
      map.put(TYPE_I, value3);
      map.put(TYPE_J, value3);

      return map.get(type);
  }

answered Oct 09 '12 at 05:01

Kim Stebel

41,826
12
125
142

Curious solution, thanks for the idea. Unfortunately in my case the values can change so the map would have to be redone all the time. – Corey Scott Oct 09 '12 at 05:11
If you refactor value1...3 to use getters and setters (which you should probably do anyway), that's not a problem. – Kim Stebel Oct 09 '12 at 06:46
16

This is not really reducing complexity of the program, just hiding it from code analyzer by moving the logic from switch-case to a map. – satellite779 Mar 31 '15 at 05:49
But what is wrong with switch cases, why do switch considered part of cyclomatic complexity ?. As far as i know, switch works far better than other, As contradicted to if-else, at run time, switch will not take much memory address manipulation to jump to appropriate case. switching and mapping is almost same with compiled version of binary code. – chethan Jul 14 '17 at 10:41
@satellite779, it does reduce the complexity. When we see a line such as `dataRetentionPeriodMap.get(forEmployeeType)`, we immediately understand what the map is for and can continue reading code. If this was an if-else ladder or a switch statement, we will be forced to look through the mapping (while also being cautious not to miss any other logic that might have creeped in), if or not we're interested in it, at the moment. When its a map, you can go look at the map only when needed. It certainly reduces cognitive load in my experience. – Andrew Nessin Nov 13 '20 at 11:27

score 6 · Accepted Answer · edited May 23 '17 at 12:34

6

The Cyclomatic Complexity is determined by the number of branches of execution in your code. if - else blocks, switch statements - all increase the Cyclomatic complexity of your code and also increase the number of test cases you would need to ensure appropriate code coverage.

To reduce complexity in your code, I would suggest you remove the case statements that do not have a defined behavior and replace it with a default behavior in your switch statement.

Here is another question on Stack Overflows that addresses this issue.

edited May 23 '17 at 12:34

Community

1
1

answered Oct 09 '12 at 04:32

Luhar

1,859
2
16
23

Thanks for the answer, I did look at that thread and couldn't find a "solution" that adequately addressed my problem. I also wasn't sure that there was a problem. – Corey Scott Oct 09 '12 at 04:36
1

I think cyclomatic complexity is a fairly subjective measure. You do not need to worry about changing your code as long as it is readable and does the job. A high cyclomatic complexity may point to issues in your object model, but I do not think this is relevant to your example. – Luhar Oct 09 '12 at 04:50
Ok. Thanks for the effort you put in this answer – Corey Scott Oct 09 '12 at 05:10
The issue is that multiple values follow the same execution path, but the typical cyclomatic complexity calculation treats all 'case' statements as separate execution paths (because it easy to compute it that way). Either find a CC tool that is more strict (i.e. correct) about what it counts as an execution path, or use your own judgement around switch statements, i.e. decide to leave them be if they are OK but CC says otherwise. – redcalx Jul 01 '15 at 12:35
If you ever google for how Java works "under the hood" you will get lots of people scolding you because "preoptimization iz 4 n00bsz!11!", but this is one of the many cases where it pays to know Java under the hood: https://www.artima.com/underthehood/flowP.html ...The TLDR (and to Luhar's comment): whether you remove the extra case statements or not is arbitrary and subjective, because I'm about 90 percent sure that will get optimized by the compiler to a lookupswitch statement anyway (I haven't tested the example to know 100%, hence 90), but either way, what will the differences gain you? – searchengine27 Sep 25 '15 at 01:05

How do I reduce the "Cyclomatic Complexity" of the following code

2 Answers2