How to reduce cyclomatic complexity in for...if loop in c#

Question

I have a simple code to print the rows and columns of a CSV file. This is my code and the cyclomatic complexity is coming out to be 6 for this class. I want to reduce it to as low as possible.

class PrintCSV
{
    /// <summary>
    /// Print all the rows of CSV File 
    /// </summary>
    /// <param name="lines"> </param>

    public static void PrintCSVRows(string[] lines)
    {

        foreach (string line in lines)
        {
            if (!string.IsNullOrEmpty(line) && line.Contains(','))
            {
                string columns = line.Split(',')[1];
                if (!string.IsNullOrEmpty(columns))
                {
                    PrintCSVCol(columns);
                }

            }
        }
    }

    /// <summary>
    /// Prints the column of CSV File
    /// </summary>
    /// <param name="columns"></param>

    public static void PrintCSVCol(string columns)
    {
        Console.WriteLine("{0}", columns);
    }

}

How do I change the loops to reduce the overall cyclomatic complexity??

This function is doing a lot - it is processing each line in an array, it is filtering away certain lines (== choosing certain lines to further process), it is processing a single line. Why not split those things into separate functions and see what happens? It's better software engineering - easier to test, for example. — davidbak, Sep 14 '20 at 15:54
I don't know about complexity, this seems relatively simple code to me. Although you could perhaps separate it into more functions (and then someone will debate whether that increases complexity in a different way no doubt!). But anyway IMHO `&& line.Contains(',')` is a bug because if the line contains just one column of data, applying this condition will mean that it won't print anything. And also `line.Split(',')[1]` ensures that it will only ever print the 2nd column of data. So this code doesn't "print the rows and columns of a CSV file", it's much more restricted than that. — ADyson, Sep 14 '20 at 15:54
@rene I've addressed that, see updated comment. The code doesn't do what the initial description claims it will do. Hard to know if that's intentional though - maybe it's just badly described. — ADyson, Sep 14 '20 at 15:57
Questions asking for help improving working code are off-topic for Stack Overflow. They _may_ be on-topic at https://codereview.stackexchange.com/, but you need to read their tour and make sure you meet their requirements (the above probably does not). — Peter Duniho, Sep 14 '20 at 17:16

score 2 · Accepted Answer · answered Sep 15 '20 at 02:22

If your goal is to reduce cyclomatic complexity only, you can extract methods like the following:

/// <summary>
/// Print all the rows of CSV File 
/// </summary>
/// <param name="lines"> </param>
public static void PrintCSVRows(string[] lines)
{
    foreach (string line in lines)
    {
        PrintLine(line);
    }
}

private static void PrintLine(string line)
{
    if (IsValidLine(line))
    {
        string columns = line.Split(',')[1];
        PrintCSVCol(columns);
    }
}

private static bool IsValidLine(string line)
{
    return !string.IsNullOrEmpty(line) && line.Contains(',');
}

/// <summary>
/// Prints the column of CSV File
/// </summary>
/// <param name="columns"></param>
public static void PrintCSVCol(string columns)
{
    if (!string.IsNullOrEmpty(columns))
    {
        Console.WriteLine("{0}", columns);
    }
}

With this code, each method has a cyclomatic complexity of 2 according to Visual Studio.

However, cyclomatic complexity is just a metric and doesn't necessarily mean that this code is better.

Bizhan · Answer 2 · 2020-09-14T16:34:39.300

1

This code does the same thing:

public static void PrintCSVRows(string[] lines)
{
    foreach (var columns in lines
        .Where(x => !string.IsNullOrEmpty(x))
        .Select(x => x.Split(','))
        .Where(x => x.Length > 1 && !string.IsNullOrEmpty(x[1]))
        .ToArray())
    {
        PrintCSVCol(columns[1]);
    }
}

Or

public static void PrintCSVRows(string[] lines)
{
    foreach (var columns in lines
        .Where(x => !string.IsNullOrEmpty(x))
        .Select(x => x.Split(','))
        .Where(x => x.Length > 1)
        .Select(x => x.Skip(1).First())
        .Where(x => !string.IsNullOrEmpty(x)))
    {
        PrintCSVCol(columns);
    }
}

edited Sep 14 '20 at 16:34

answered Sep 14 '20 at 16:24

Bizhan

16,157
9
63
101

Could drop the `if(columns.Count > 1)` by switching to `PrintCSVCol(columns.FirstOrDefault())` with the null check in the `PrintCSVCol` function. – asawyer Sep 14 '20 at 16:31
@asawyer that wouldn't do the same thing, it would print null – Bizhan Sep 14 '20 at 16:32
You can get the `foreach` down to this: `foreach (var columns in lines.Where(x => x != null).Select(x => x.Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries)).SelectMany(x => x.Skip(1).Take(1)))`. – Enigmativity Sep 15 '20 at 04:02
@Enigmativity that would perform differently in some corner cases (like "" or ",sth,sth"), but the StringSplitOptions.RemoveEmptyEntries is a good suggestion. – Bizhan Sep 15 '20 at 11:35

How to reduce cyclomatic complexity in for...if loop in c#

2 Answers2