Search code examples
c#classcode-complexitycognitive-complexity

Reduce Class Complexity - Cognitive Complexity C#


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++;
    }

Solution

  • 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:

    1. extract while body code to a method
    2. extract "record type" body code to another methods
    3. 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.