How can I reduce Cyclomatic complexity?

Question

I have a method like below. Please help to avoid Cyclomatic complexity.

private double getSum(Data data) {
    double total = 0;

    if(parameters.getParam1())
      total += data.getParam1();

    if(parameters.getParam2())
      total += data.getParam2();

    if(parameters.getParam3())
      total += data.getParam3();

    if(parameters.getParam4())
      total += data.getParam4();

    if(parameters.getParam5())
      total += data.getParam6();

    if(parameters.getParam6())
      total += data.getParam6();

    if(parameters.getParam7())
      total += data.getParam7();

    if(parameters.getParam8())
      total += data.getParam8();

    return total;
}

Sounds like a question for http://codereview.stackexchange.com. Please move it — René Link, Jan 22 '14 at 07:11
The `if` statement indicates `getParamX` returns a boolean, but the if-body indicates the `+=` operator can be used - which isn't possible with a boolean. Please make it make sense. — Jeroen Vannevel, Jan 22 '14 at 07:13
@JeroenVannevel but in the if OP uses `parameters` then inside the if `data` — René Link, Jan 22 '14 at 07:14
@ReneLink you could imagine that a warning is emitted by some analysis tool and the op wants to fix the code so SO is fine IMO. — assylias, Jan 22 '14 at 07:14
Are the results of getParamX() not independant in some way? Can you infer the value of getParamX_m() from getParamX_n()? — waTeim, Jan 22 '14 at 07:26
Here, Parameters class defines whether to use that parameter or not. If Parameters class return true the value of that parameter is taken from Data class. I have just changes the name of the parameter for security reasons. — user3222372, Jan 22 '14 at 08:25

score 1 · Answer 1 · 2014-01-22T17:36:18.913

1

I would create a method like this:

double doubleOrZero(boolean condition, double value) {
    return condition ? value : 0.0;
}

Then call it for each paramX, like this:

private double getSum(Data data) {
    double total = 0.0;

    total += doubleOrZero(parameters.getParam1(), data.getParam1());
    total += doubleOrZero(parameters.getParam2(), data.getParam2());
    // ...

edited Jan 22 '14 at 17:36

answered Jan 22 '14 at 07:25

Or simply `return a ? b : 0:` – assylias Jan 22 '14 at 08:03

score 1 · Answer 2 · answered Jan 22 '14 at 07:41

1

As other mentioned, you will be better of to rewrite your Parameter and Data class, to uses them like this:

double total=0; 
for (int i=1; i<=8;i++)
    if (parameters.hasParam(i))
        total+ = data.getParam(i);
return total;

answered Jan 22 '14 at 07:41

Chriss

5,157
7
41
75

score 0 · Answer 3 · answered Jan 22 '14 at 07:18

0

For the given code sample, it is only possible the reduce boilderplate by using reflection, like this:

double total=0; 
for (int i=1; i<=8;i++)
    if (parameters.getClass().getMethod("getParam"+i).invoke(parameters)==Boolean.TRUE)
        total+ = (double)data.getClass().getMethod("getParam"+i).invoke(data);
return total;

answered Jan 22 '14 at 07:18

Chriss

5,157
7
41
75

I was working on a similar approach but without reflection. Reflect kills static type checking etc and IMHO probably makes the overall code quality worse then the original. – Miserable Variable Jan 22 '14 at 07:20
@MiserableVariable Yes thats indeed not best practice and unreadable too, but the only option i know to reduce boilerplate in this case. – Chriss Jan 22 '14 at 07:23
It doesn't reduce cyclomatic complexity either as your trading 8 if statements for a for loop that counts to 8. Parameterizing getMethod() with i increases the number of states exactly as much as reducing the number of states by eliminating the if statements. – waTeim Jan 22 '14 at 07:24
How about just changing `getParam1()`, `getParam2()`, etc to `getParam(int i)`? Assuming the OP can actually do such a change. – ADTC Jan 22 '14 at 07:25
@waTeim the cyclomatic complexity does not increase with each iteration, it is statically determined and for a loop it adds 1, just as for if statement. – Miserable Variable Jan 22 '14 at 07:33
ok, i interpreted it to mean +1 path per iteration if a branch was possible inside the loop. – waTeim Jan 22 '14 at 07:39

Miserable Variable · Answer 4 · 2014-01-22T08:42:36.183

0

Here's a hint. Consider the following code:

for (int i = 0; i < 8; i++) {
  if (paramGetters[i].get(parameters)) {
    total += paramGetters[i].get(data);
  }
}

UPDATE1

Some more hints how this can be made to compile:

paramGetters is an array of objects of some type that have an overloaded get method: get(paramters) returns a boolean, get(data) returns a number. Also, each object calls one of the specific getParam method.

UPDATE2

The following lines set the first element of the array:

paramGetters[0] = new ParamGetter() {
    boolean get(Parameters p) { return p.getGame(); }
    double get(Data d) { return d.getGameValue(); }
}

This is based on OP's comment about the actual methods to be called. I will leave it you to define the class, the array, and the rest of the elements in the array.

edited Jan 22 '14 at 08:42

answered Jan 22 '14 at 07:25

Miserable Variable

28,432
15
72
133

I am sorry Guys. I think i put the example incorrectly. For security reasons i changes the parameter names like param1, param2 etc. The parameter names are very different like game, hotel, etc. – user3222372 Jan 22 '14 at 08:28
Correct example: private double getSum(Data data) { double total = 0; if(parameters.getGame()) total += data.getGameValue(); if(parameters.getHotel()) total += data.gethotelValue(); if(parameters.getBar()) total += data.getBar Value(); return total; } – user3222372 Jan 22 '14 at 08:31
You can still do that with my example. I will add one more hint now – Miserable Variable Jan 22 '14 at 08:39

How can I reduce Cyclomatic complexity?

4 Answers4