Reduce Class Complexity - Cognitive Complexity C#

Question

How can I improve the Cognitive Complexity in my code?

I have a method which has while loop and inside that lot of IF ELSE blocks, I tried to remove IF ELSE with SWITCH Statements but no improvement with Cognitive Complexity as per SONAR cube analysis.

This is my existing code:

while (this.moveNextDelegate(fileLineEnumerator))
{
    var line = fileLineEnumerator.Current;
    var recordType = GetRecordType(line);  // This Method returns the type of record

    if (recordType == "1")
    {
        headerId++;
        fileHeader = line; // Here fileHeader is being used in downsteam code flow - line 19
        // some custom logic - deleted
    }
    else if (recordType == "5")
    {
        batchHeader = line; // Here batchHeader is being used in downsteam code flow - line 19
        isRepeativeRecord = false;
    }
    else if (recordType == "6")
    {
            batchHeaderId =     // some custom logic - deleted
             // Here batchHeaderId is being used in downsteam code flow - line 35 
            detailId++;
            isFlag = false;
            isRepeativeRecord = true;
            // some custom logic - deleted
    }
    else if (recordType == "7" && !isFlag)
    {
        addendaId++;
        detailRecordsExist = true;
        // some custom logic - deleted
    }
    
        currentIndex++;
}

My new code using Switch statement - but still there is no improvement with complexity

    while (this.moveNextDelegate(fileLineEnumerator))
    {
        var line = fileLineEnumerator.Current;
        var recordType = GetRecordType(line);

        switch (recordType)
        {
            case "1":
                {
                    headerId++;
                    fileHeader = line; 
                    // some custom logic - deleted
                    break;
                }

            case "2":
                {
                    batchHeader = line;
                    isRepeativeRecord = false;
                    break;
                }

            case "6": 
                {

                // some custom logic - deleted
                    break;
                }

            case "7": 
                {
                    if (!isFlag)
                    {
                // some custom logic - deleted
                    }

                    break;
                }

            case "8": 
                {
                    if (!isFlag)
                    {
                        // some custom logic - deleted
                    }

                    break;
                }
        }

        currentIndex++;
    }

https://www.sonarsource.com/docs/CognitiveComplexity.pdf – gunr2171 Aug 15 '21 at 20:32 — gunr2171, Aug 15 '21 at 20:32
https://codereview.stackexchange.com/ – Aug 15 '21 at 20:33 — , Aug 15 '21 at 20:33

score 0 · Accepted Answer · answered Aug 15 '21 at 21:31

Switching from if/else to switch statement can be a good idea, don't discard that from final code, but will not reduce complexity because the "if/else/switch" statement still exists in the body with the others if statements.

To avoid sonar complexity, write more simple methods, each with a single goal, it are better to test and have less logic to understand.

https://en.wikipedia.org/wiki/SOLID

Looking to your code, you can extract some methods, I did some refactor to reduce complexity, as you can see in the list bellow:

extract while body code to a method
extract "record type" body code to another methods
you can still refactor to classes (with caution, can be a over engineering)

private void Foo()
{
    FileEnumerator fileLineEnumerator = new FileEnumerator();

    while (this.moveNextDelegate(fileLineEnumerator))
    {
        string line = fileLineEnumerator.Current;
        ReadNextLine(line);
        currentIndex++;
    }
}

private void ReadNextLine(string line)
{
    var recordType = GetRecordType(line);

    if (recordType == "1")
    {
        this.ReadHeader(line);
    }
    else if (recordType == "5")
    {
        ReadBatchHeader(line);
    }
    else if (recordType == "6")
    {
        ReadRepeativeRecord();
    }
    else if (recordType == "7" && !this.isFlag)
    {
        ReadDetail();
    }
}

private void ReadDetail()
{
    this.addendaId++;
    this.detailRecordsExist = true;
}

private void ReadRepeativeRecord()
{
    this.batchHeaderId = this.detailId++;
    this.isFlag = false;
    this.isRepeativeRecord = true;
}

private void ReadBatchHeader(string line)
{
    this.batchHeader = line;
    this.isRepeativeRecord = false;
}

private void ReadHeader(string line)
{
    this.headerId++;
    this.fileHeader = line;
}

Looks like you are writing a reader, so if you have a lot of readers/steps like reading header, body, footer of file, with details and recursive content you can separate each fragment in a separate class like HeaderReader, BodyReader, DetailReader, but this can be a over engineering, so you will decide if worth it.

public class ReadState
{
    public RecordType RecordType { get; set; }

    public string Line { get; set; }
}

public class ReadResult
{
    public int HeaderId { get; set; }
}

public class BodyReader : IYourCustomFileFragmentReaderInterface
{
    public bool Test(ReadState state)
    {
        // check if is header
        return state.RecordType == RecordType.Body;
    }

    public ReadResult Read(ReadResult result, ReadState state)
    {
        // write state
        result.HeaderId = int.Parse(state.Line);
        return result;
    }
}

With separate readers you can group in a list of IYourCustomFileFragmentReaderInterface, call Test() and if true then call the Read method.

Thank you so much Leo for the detailed explanation – Manas Aug 16 '21 at 12:34 — Manas, Aug 16 '21 at 12:34

Reduce Class Complexity - Cognitive Complexity C#

1 Answers1