Cyclomatic complexity count of 31, where has this come from?

Question

I'm developing an application that pulls data from an Excel file (I don't have access to the actual database) and I've written a method which has as its only function to pull the data from the Excel Spreadsheet, as seen below.

private IEnumerable<SMEntity> ExtractSMData(List<MSExcel.Range> SMData)
{
    List<SMEntity> SMEntities = new List<SMEntity>();

    foreach (MSExcel.Range Row in SMData)
    {
        SMEntity entity = new SMEntity();
        entity.IncidentNumber = Row.get_Range("K1").get_Value();
        entity.SRNumber = Row.get_Range("L1").get_Value();
        entity.SRCategory = Row.get_Range("M1").get_Value();
        entity.SiebelClientCall = EntityConversions.DateTimeConversion(Row.get_Range("N1").get_Value());
        entity.SiebelOpenedDate = EntityConversions.DateTimeConversion(Row.get_Range("O1").get_Value());
        entity.IncidentOpenDate = EntityConversions.DateTimeConversion(Row.get_Range("P1").get_Value());
        entity.PickedUpBeforeClient = Row.get_Range("Q1").get_Value().ToString().ToLowerCase() == "no" ? false : true;
        entity.OutageStartTime = EntityConversions.DateTimeConversion(Row.get_Range("R1").get_Value());
        entity.DetectionPoint = EntityConversions.DateTimeConversion(Row.get_Range("S1").get_Value());
        entity.SecondsToDetection = EntityConversions.ConvertDetectionTimeToInt(Row.get_Range("T1").get_Value());
        entity.OutageEndTime = EntityConversions.DateTimeConversion(Row.get_Range("U1").get_Value());
        entity.MTTR = EntityConversions.ConvertMTTRStringToInt(Row.get_Range("V1").get_Value());
        entity.RepairedOnTime = Row.get_Range("W1").get_Value().ToString().ToLowerCase() == "no" ? false : true;
        SMEntities.Add(entity);
    }

    return SMEntities;
}

I've run Code Analysis (I'm using Visual Studio 2012 and developing in .NET 4.5) and I have a CA1502: Avoid excessive complexity (copied below). As a junior developer (I'm 17) I tried finding out more about this using MSDN however, I'm a little stumped as to why I have a cyclomatic complexity of 33.

CA1502

Avoid excessive complexity

'Extraction.ExtractSMData(List<Range>)' has a cyclomatic complexity of 33. Rewrite or refactor the method to reduce complexity to 25.

Core.Extraction.cs:104

I can see with my quick-ifs (condition ? if_true : if_false, what are these called?) that it might be bad, but I can still only see it as 5.

UPDATE:

Cyclomatic complexity is now at 33...

If I comment out entity.IncidentNumber = Row.get_Range("K1").get_Value(); the complexity becomes 32. I thought get_Range() and get_Value() were one each but okay...

If I comment out entity.RepairedOnTime = Row.get_Range("W1").get_Value().ToString().ToLower() == "no" ? false : true; the complexity becomes 28...

get_Range(), get_Value(), quick-if is 3, do ToString() and ToLower() count?

Your "quick-ifs" are called the [conditional operator](http://msdn.microsoft.com/en-us/library/ty67wk28(v=vs.110).aspx). I like your use of the phrase "quick-if" though. It's tidy. ;) — J. Steen, Dec 06 '12 at 10:11
@alexh It is NOT called ***the*** ternary operator - it's the **conditional operator** and it is ***a*** ternary operator — Andras Zoltan, Dec 06 '12 at 10:13
The loop also adds to the cyclomatic complexity (it is not _cyclic_, it is _cyclomatic_). — Oded, Dec 06 '12 at 10:13
? is as J. Steen says called the `conditional operator`. It is A ternary operator, not THE ternary operator. Altough it's the only ternary operator in the language. :) — Sani Huttunen, Dec 06 '12 at 10:14
`bool x = Row.get_Range("W1").get_Value().ToString().ToLowerCase() == "no" ? false : true;` can be replaced with `bool x = Row.get_Range("W1").get_Value().ToString().ToLowerCase() != "no"` which is not cnditional statement so you may reduce complexity a little bit. — petro.sidlovskyy, Dec 06 '12 at 10:14
@Andras Zoltan, thanks for the precision : we use the "ternary" term word in my country to define this operator. It is a mistake — AlexH, Dec 06 '12 at 10:20
@AlexH hey - I think I (and others) could be accused a certain amount of pedantry in this regard. In the same way that it's bad English grammar to split an infinitive, yet nearly everyone does it! However on SO I think the general consensus is to try and stick to the *proper* names of things; and tbh it's not like I or anyone else never made the same mistake :) — Andras Zoltan, Dec 06 '12 at 10:37
Ahhh. I see. thanks :) Googling for "? operator" doesn't yield much unfortunately. Maybe I need more coffee when I do my searches because ""?" operator" didn't help either! — Jake Hendy, Dec 06 '12 at 11:05
Also, the thing to google for would be "?: operator", as the colon is part of the operator. =) — J. Steen, Dec 06 '12 at 15:04

score 1 · Answer 1 · answered Dec 06 '12 at 13:28

1

I calculate the complexity for the method itself, the foreach, and the two conditional operators as 4 total. If each of the 13 calls to get_Range is worth +1 complexity and each of the 13 calls to get_Value is worth +1 complexity then the total complexity will add up to 30 (still 1 short, but close). I'm not sure why those two functions could be increasing the complexity but it seems plausible.

Try removing one of the lines calling get_Range and get_Value and see if the cyclomatic complexity goes down to 29.

answered Dec 06 '12 at 13:28

Mike Cowan

919
5
11

FxCop works on the generated IL-code. I assume the compiler inserts some COM interop code for each call (e.g. check for null). There are 28 COM calls (13xget_Range,13xget_value,GetEnumerator,GetNext), 2x ?: + foreach = 31 – adrianm Dec 06 '12 at 13:51
Check update guys. I thought so too Adrian but now it's at 33! – Jake Hendy Dec 07 '12 at 09:16

CSharpie · Answer 2 · 2012-12-06T13:56:26.537

Your return-type is IEnumerable, so dont use a list. That makes IENumerable useless. Otherwise you dont make use of Lazy-Evaluation See: http://blogs.msdn.com/b/pedram/archive/2007/06/02/lazy-evaluation-in-c.aspx

better use yield return then:

private IEnumerable<SMEntity> ExtractSMData(List<MSExcel.Range> SMData)
{
    foreach (MSExcel.Range Row in SMData)
    {
        SMEntity entity = new SMEntity();

        entity.IncidentNumber = Row.get_Range("K1").get_Value();
        entity.SRNumber = Row.get_Range("L1").get_Value();
        entity.SRCategory = Row.get_Range("M1").get_Value();
        entity.PickedUpBeforeClient = !Row.get_Range("Q1").get_Value().ToString().ToLowerCase() == "no"
        entity.RepairedOnTime = !Row.get_Range("W1").get_Value().ToString().ToLowerCase() == "no"

        entity.SiebelClientCall = EntityConversions.DateTimeConversion(Row.get_Range("N1").get_Value());
        entity.SiebelOpenedDate = EntityConversions.DateTimeConversion(Row.get_Range("O1").get_Value());
        entity.IncidentOpenDate = EntityConversions.DateTimeConversion(Row.get_Range("P1").get_Value());
        entity.OutageStartTime = EntityConversions.DateTimeConversion(Row.get_Range("R1").get_Value());
        entity.DetectionPoint = EntityConversions.DateTimeConversion(Row.get_Range("S1").get_Value());
        entity.OutageEndTime = EntityConversions.DateTimeConversion(Row.get_Range("U1").get_Value());


        entity.MTTR = EntityConversions.ConvertMTTRStringToInt(Row.get_Range("V1").get_Value());
        entity.SecondsToDetection = EntityConversions.ConvertDetectionTimeToInt(Row.get_Range("T1").get_Value());


        yield return entity;
    }

}

You could also write it like this:

private IEnumerable<SMEntity> ExtractSMData(List<MSExcel.Range> SMData)
{
    foreach (MSExcel.Range Row in SMData)
    {
        yield return new SMEntity 
        {

            IncidentNumber = Row.get_Range("K1").get_Value(),
            SRNumber = Row.get_Range("L1").get_Value(),
            SRCategory = Row.get_Range("M1").get_Value(),
            PickedUpBeforeClient = !Row.get_Range("Q1").get_Value().ToString().ToLowerCase() == "no"
            RepairedOnTime = !Row.get_Range("W1").get_Value().ToString().ToLowerCase() == "no"

            SiebelClientCall = EntityConversions.DateTimeConversion(Row.get_Range("N1").get_Value()),
            SiebelOpenedDate = EntityConversions.DateTimeConversion(Row.get_Range("O1").get_Value()),
            IncidentOpenDate = EntityConversions.DateTimeConversion(Row.get_Range("P1").get_Value()),
            OutageStartTime = EntityConversions.DateTimeConversion(Row.get_Range("R1").get_Value()),
            DetectionPoint = EntityConversions.DateTimeConversion(Row.get_Range("S1").get_Value()),
            OutageEndTime = EntityConversions.DateTimeConversion(Row.get_Range("U1").get_Value()),


            MTTR = EntityConversions.ConvertMTTRStringToInt(Row.get_Range("V1").get_Value()),
            SecondsToDetection = EntityConversions.ConvertDetectionTimeToInt(Row.get_Range("T1").get_Value())
        };
    }
}

Lazy evaluation is nice for simple in-memory stuff. When dealing with external dependencies or other complex things you better be eager to be able to handle errors. The consumer of the IEnumerable shouldn't have to deal with COMException, SqlExcepton, IOException, ... based on an implementation detail of the method? — adrianm, Dec 06 '12 at 14:19
The "source"-Data is in Memory allready as the function reuqires a List as Parameter. I think it comes down to "personal preference" here. If he wants to fail all rows at once, or retreive some before the psosible error occurs. — CSharpie, Dec 06 '12 at 14:34

Cyclomatic complexity count of 31, where has this come from?

2 Answers2