How to refactor this method to reduce its cognitive complexity

Question

I need help, I'm trying to refactor this method to reduce its cognitive complexity sonarqube display this issue

private void PopulateBook(Book b)
{
    if (b.page.num001 == null) b.page.num001 = string.Empty;
    if (b.page.num002 == null) b.page.num002 = string.Empty;
    if (b.page.num003 == null) b.page.num003 = string.Empty;
    if (b.page.num004 == null) b.page.num004 = string.Empty;
    if (b.page.num005 == null) b.page.num005 = string.Empty;
    if (b.page.num006 == null) b.page.num006 = string.Empty;
    if (b.page.num007 == null) b.page.num007 = string.Empty;
    if (b.page.num008 == null) b.page.num008 = string.Empty;
    if (b.page.num009 == null) b.page.num009 = string.Empty;
    if (b.page.num010 == null) b.page.num010 = string.Empty;
    if (b.page.num011 == null) b.page.num011 = string.Empty;
    if (b.page.num012 == null) b.page.num012 = string.Empty;
    if (b.page.num013 == null) b.page.num013 = string.Empty;
    if (b.page.num014 == null) b.page.num014 = string.Empty;
    if (b.page.num015 == null) b.page.num015 = string.Empty;
    if (b.page.num016 == null) b.page.num016 = string.Empty;
    if (b.page.num017 == null) b.page.num017 = string.Empty;
    if (b.page.num018 == null) b.page.num018 = string.Empty;
    if (b.page.num019 == null) b.page.num019 = string.Empty;
    if (b.page.num020 == null) b.page.num020 = string.Empty;
}

What is `b.page`? And I guess it has 20 numbered fields? Can you show a little more context? — Fildor, May 05 '21 at 13:24
Start by putting all those fields/properties into a single array or list. Then you can just loop that and do a one line `if` — Charlieface, May 05 '21 at 13:24
I think you could get completely rid of this code, after all. — Fildor, May 05 '21 at 13:27
Making the pages/nums enumerable is probably the best solution. If this is not possible and your language version is C# 8.0 or higher, you could also consider using the [null-coalescing assignment operator](https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/operators/null-coalescing-operator) `??=`. E.g. `b.page.num001 ??= string.Empty;`. — thabs, May 05 '21 at 13:30

Morten Bork · Answer 1 · 2021-05-08T05:58:15.390

0

You could redesign the Page class.

I do not know what num001 etc, refer to as a number on a page. Is it word indexes? number of characters, specific characters? Or something else entirely?

But whatever it is:

public class Page {

   public List<Num> numList

   public Page()
   {
      NumList = InitializeNumList();
   }

   private List<Num> InitializeNumList()
   {
      NumList = new List<Num>{};
      for(int i = 0; i < 20; i++)
      {
          NumList.add(new Num(i))
      }
   }

} 

public class Num
{
   public int Id {get;set;};
   public string SomeString {get;set;}

   public Num(int id)
   {
       Id = id;
       SomeString = "";
   }
}

edited May 08 '21 at 05:58

answered May 05 '21 at 13:53

Morten Bork

1,413
11
23

Then you have a list of objects wrapping an int ... what do you win? Where do you put the obvious string value? – Fildor May 05 '21 at 13:54
obviously, there is no branching here. Complexity is reduced. Edited Num, to have a string – Morten Bork May 05 '21 at 14:03
Also, it is very easy to reset the strings. you can build a proper for-each loop and reset them all. – Morten Bork May 07 '21 at 06:43
Have a look again – Morten Bork May 08 '21 at 05:58
Close. I'd make the pages property a list of the class you named "Num" and rename it Page. But then it's pretty much it, yes. One could argue that pages should simply be `List` ... – Fildor May 08 '21 at 08:37
1

It would help immensely, if we new what "num" was supposed to represent? – Morten Bork May 10 '21 at 07:57

Andreas Hütter · Answer 2 · 2022-11-05T00:32:46.767

The cognitive complexity originates from the high amount of conditionals (here if statements). SonarQube by default objects cognitive complexity higher than 15, this method has a cognitive complexity value of 20.

Anyways, in this case it's easy to extract the repetitive logic into a separate method like this:

private void PopulateBook(Book b)
{
    b.page.num001 = GetValueOrDefault(b.page.num001);
    b.page.num002 = GetValueOrDefault(b.page.num002);
    // Do the same for num003 to num019
    b.page.num020 = GetValueOrDefault(b.page.num020);
}

private void GetValueOrDefault(string value)
{
    if (value == null)
    {
        return string.Empty;
    }
    return value;
}

By extracting the conditional logic to a seperate single place the cognitive complexity of the initial code has shrinked from 20 to 1. While the PopulateBook() now has a cognitive complexity of 0 the new extracted method GetValueOrDefault() (naming discussable) has a cognitive complexity of 1. Cognitive complexity is all about the workload for the human brain which is drastically reduced by allowing the brain to read the conditional only once instead of 20 times.

You still have 20 statements in the original method but none of these show conditional logic and in the context of this method it's just setting a value retrieved from a method which does not impose cognitive load on the brain.

To see how cognitive complexity is calculated and works in detail please have a look at this answer.

If this is the best way to design such a data structure in the first place is a different topic of course.

Note: of course the method GetValueOrDefault() could also be written using the teneray operator which might be a matter of taste but still would yield the same cognitive complexity of 1 for this method.

score -1 · Answer 3 · answered May 05 '21 at 13:32

-1

If you want to reduce number of lines of code and assuming your "Page" is a class with these properties only - Then you can use some sort of reflection to set properties using a loop.

  private static void PopulateBook(Book b)
        {
            var page = b.page;
            PropertyInfo[] properties = typeof(page).GetProperties();
            foreach(var prop in properties)
            {
                if(prop.GetValue(page) == null)
                prop.SetValue(page, String.Empty);
            }
        }

answered May 05 '21 at 13:32

Sangeeth Nandakumar

1,362
1
12
23

2

I don't think reflection improves the cognitive complexity of the code (which is what the question asks for). – thabs May 05 '21 at 13:51
1

Please do not resort to reflection every time you feel the need to "patch up" bad architecture. That's getting you nowhere except maintainance-hell. – Fildor May 05 '21 at 13:53

How to refactor this method to reduce its cognitive complexity

3 Answers3