Reduce Complexity Question

[beantownace]

Hey all,

I am dealing with some Sonar cube squaking and hoping someone may be able to have some suggestions. Two spots it does not like I just need to at least deal with one of them. Thanks to anyone that can provide some suggestions.

First ding:

double groupA = 0;
double groupB = 0;
//more code here so for brevity but this while OR check it does not like
while (groupA != 0.0 || groupB != 0.0) //this OR need to change it seems
{
    //do stuff
}

Second ding little harder:

//does not like the If conditions this is in a for loop as well.

if (largestInv.Amount > largestOrd.Amount)
{
  //code
}
else if (largestInv.Amount < largestOrd.Amount)
{
  //code
}
else if (largestInv.Amount == largestOrd.Amount)
{
  //code
}

 

[Skydiver]

For issue 1: Please post the specific SonarQube message and message id for issue 1. That is as simple as it gets unless nothing between the declaration on lines 1-2 and the beginning of the while loop on line 4 changes the values of groupA and groupB. If nothing changes the values of groupA and groupB, then the best simplification is to remove the entire while loop and its body (lines 4-7) because the condition will never be true.

If something can change groupA or groupB before the while loop, but nothing changes the values of groupA and groupB within the loop, then the while can very likely be change to an if and the body of the if will some kind of infinite loop in it.

For issue 2: You can replace line 11's else if(...) with simply else. This is because:

if (a > b)
{
    Console.WriteLine("A is bigger than B");
}
else if (a < b)
{
    Console.WriteLine("A is smaller than B");
}
else
{
    Console.WriteLine("A must be equal to B, if it is neither bigger nor smaller than B");
}

 

[Skydiver]

Have you also considered applying DeMorgan's Law:

(NOT a) OR (NOT b)

is equivalent to:

NOT(a AND b)

 

[beantownace]

Have you also considered applying DeMorgan's Law:

(NOT a) OR (NOT b)

is equivalent to:

NOT(a AND b)

I will try that thanks

 

[beantownace]

This is the exact syntax:

public static CalcCustomerAmounts(Invoice largestInvoice, PO largestPO )
{
  if (largestPO.Amount > largestInvoice.Amount)
  {
      largestPO.Amount += largestInvoice.Amount
      largestInvoice.Amount = 0;
  }
  else if (largestPO.Amount < largestInvoice.Amount)
  {
      largestInvoice.Amount -= largestPO.Amount
      largestPO.Amount = 0;
  }
  else if (largestInvoice.Amount == largestPO.Amount)
  {
      largestInvoice.Amount = 0;
      largestPO.Amount = 0;
  }
}

 

[Momerath]

double groupA = 0;
double groupB = 0;
//more code here so for brevity but this while OR check it does not like
while (groupA != 0.0 || groupB != 0.0) //this OR need to change it seems
{
    //do stuff
}

Just wanted to add that doing comparisons with exact floating point values is ... problematic. Floating point isn't exact and this can cause some really hard to find errors.

 

[Skydiver]

I was asking about the exact messages and message IDs given by SonarQube regarding issue 1.

 

[beantownace]

I was asking about the exact messages and message IDs given by SonarQube regarding issue 1.

Message is reduce cognitive complexity 18 out of 15 so too many

 

[Skydiver]

For issue 2, these two are equivalent:

  if (largestPO.Amount > largestInvoice.Amount)
  {
      largestPO.Amount += largestInvoice.Amount
      largestInvoice.Amount = 0;
  }
  else if (largestPO.Amount < largestInvoice.Amount)
  {
      largestInvoice.Amount -= largestPO.Amount
      largestPO.Amount = 0;
  }
  else if (largestInvoice.Amount == largestPO.Amount)
  {
      largestInvoice.Amount = 0;
      largestPO.Amount = 0;
  }

and

  if (largestPO.Amount > largestInvoice.Amount)
  {
      largestPO.Amount += largestInvoice.Amount
      largestInvoice.Amount = 0;
  }
  else if (largestPO.Amount < largestInvoice.Amount)
  {
      largestInvoice.Amount -= largestPO.Amount
      largestPO.Amount = 0;
  }
  else  // therefore largestPO.Amount == largestInvoice.Amount
  {
      largestInvoice.Amount = 0;
      largestPO.Amount = 0;
  }

 

[Skydiver]

Message is reduce cognitive complexity 18 out of 15 so too many

You need to show us the rest of the code. With the code you just showed the complexity for line 4 is 3. For your method to have reached 18, that means that you had a lot more branches and conditions that took you all the way up to 18.

See https://blog.devgenius.io/sonarqube-cognitive-complexity-265640dbad3e

 

[Skydiver]

Personally, I think this increases "cognitive complexity", but will likely make SonarQube happier:

  if (largestPO.Amount > largestInvoice.Amount)
  {
      largestPO.Amount += largestInvoice.Amount
      largestInvoice.Amount = 0;
  }
  else 
  {
      largestInvoice.Amount -= largestPO.Amount
      largestPO.Amount = 0;
  }

The reason why I think that it increases cognitive complexity is that the programmer now needs to stare at line 6-10 and realize that it is equivalent to:

  else if (largestPO.Amount < largestInvoice.Amount)
  {
      largestInvoice.Amount -= largestPO.Amount
      largestPO.Amount = 0;
  }
  else  // therefore largestPO.Amount == largestInvoice.Amount
  {
      largestInvoice.Amount = 0;
      largestPO.Amount = 0;
  }

because if largestPO.Amount > largestInvoice.Amount is false, then therefore largestPO.Amount <= largestInvoice.Amount.

In both cases of largestPO.Amount < largestInvoice.Amount and largestPO.Amount == largestInvoice.Amount, we set largestPO.Amount = 0;.

In the case when largestPO.Amount < largestInvoice.Amount, we subtract largestPO.Amount from largestInvoice.Amount.

In the case when largestPO.Amount == largestInvoice.Amount, the old code would set largestPO.Amount = 0. But since the two are the same value, largestPO.Amount from largestInvoice.Amount would be the same as setting largestPO.Amount to zero.

 

[Skydiver]

As a general rule of thumb. If you can keep a method to less than 20 lines of code, it will usually make the code easier for someone else to read (and make SonarQube think that the complexity is lower), and make the code more likely to follow the single responsibility principle. Perhaps the method in issue 1 is simply very long and so its SonarQube complexity score went up dramatically.

 

[beantownace]

Personally, I think this increases "cognitive complexity", but will likely make SonarQube happier:

  if (largestPO.Amount > largestInvoice.Amount)
  {
      largestPO.Amount += largestInvoice.Amount
      largestInvoice.Amount = 0;
  }
  else
  {
      largestInvoice.Amount -= largestPO.Amount
      largestPO.Amount = 0;
  }

The reason why I think that it increases cognitive complexity is that the programmer now needs to stare at line 6-10 and realize that it is equivalent to:

  else if (largestPO.Amount < largestInvoice.Amount)
  {
      largestInvoice.Amount -= largestPO.Amount
      largestPO.Amount = 0;
  }
  else  // therefore largestPO.Amount == largestInvoice.Amount
  {
      largestInvoice.Amount = 0;
      largestPO.Amount = 0;
  }

because if largestPO.Amount > largestInvoice.Amount is false, then therefore largestPO.Amount <= largestInvoice.Amount.

In both cases of largestPO.Amount < largestInvoice.Amount and largestPO.Amount == largestInvoice.Amount, we set largestPO.Amount = 0;.

In the case when largestPO.Amount < largestInvoice.Amount, we subtract largestPO.Amount from largestInvoice.Amount.

In the case when largestPO.Amount == largestInvoice.Amount, the old code would set largestPO.Amount = 0. But since the two are the same value, largestPO.Amount from largestInvoice.Amount would be the same as setting largestPO.Amount to zero.

thanks again this was very helpful