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