HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Classifying rows of data by the number of days to close

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
rowsclosethenumberclassifyingdatadays

Problem

DataTable looks like below

I would like to simplify this code in a better manner.

DataTable dt = results.Results[0].DataTable;
dt.Columns.Add("Averagedays", typeof(string));
dt.Columns.Add("0to3", typeof(string));
dt.Columns.Add("4to7", typeof(string));
dt.Columns.Add("Gtrthan7", typeof(string));

foreach (DataRow row in dt.Rows)
{
    if (row["total_daysclose"] != System.DBNull.Value && row["closed_in0to3_count"] != System.DBNull.Value
        && row["closed_in4to7_count"] != System.DBNull.Value && row["closedGtr7_count"] != System.DBNull.Value
        && row["Closed"] != System.DBNull.Value)
    {
        if (Convert.ToDecimal(row["closed"]) > 0)
        {
            decimal value = Convert.ToDecimal(row["total_daysclose"]) / Convert.ToDecimal(row["Closed"]);
            row["Averagedays"] = string.Format("{0:0.#}", Math.Floor(value * 10) / 10);
            row["0to3"] = string.Format("{0:P1}", Convert.ToDecimal(row["closed_in0to3_count"]) / Convert.ToDecimal(row["Closed"]));
            row["4to7"] = string.Format("{0:P1}", Convert.ToDecimal(row["closed_in4to7_count"]) / Convert.ToDecimal(row["Closed"]));
            row["Gtrthan7"] = string.Format("{0:P1}", Convert.ToDecimal(row["closedGtr7_count"]) / Convert.ToDecimal(row["Closed"]));

        }
        else
        {
            row["Averagedays"] = string.Format("{0:0.#}", 0);
            row["0to3"] = string.Format("{0:P1}", 0);
            row["4to7"] = string.Format("{0:P1}", 0);
            row["Gtrthan7"] = string.Format("{0:P1}", 0);

        }
    }
}

Solution

In addition to Heslacher's review, I'd also move the initial if-check to a separate function:

private bool IsRowInvalid(DataRow row)
{
    return row["total_daysclose"] == System.DBNull.Value 
           || row["closed_in0to3_count"] == System.DBNull.Value
           ||row["closed_in4to7_count"] == System.DBNull.Value 
           || row["closedGtr7_count"] == System.DBNull.Value 
           ||row["Closed"] == System.DBNull.Value;
}


This method you then call like this:

foreach (DataRow row in dt.Rows)
{
    if (IsRowInvalid(row))
    {
        continue;
    }

    // other logic here
}


I would also rethink names like "0to3" and "total_daysclose" if possible: these naming styles are inconsistent and IMHO bad; I'd much prefer "ZeroToThree" and "TotalDaysClose". "Gtrthan7" is even worse, since it contains an abbreviation and I really can't see any reason why.

Note that I would also move column names to constants, perhaps even move those to a static class, e.g.

internal static class ColumnName
{
    public const string AverageDays = "AverageDays";
    public const string Closed = "Closed";
}


I didn't implement this in the method above because it was too much work.

I just noticed your edit, and I'm wondering whether the NULL checks are even necessary, since I don't see any NULLs in your sample output. I also agree with the comments that this code indicates issues elsewhere, and that simply focusing on this method is not enough.

Code Snippets

private bool IsRowInvalid(DataRow row)
{
    return row["total_daysclose"] == System.DBNull.Value 
           || row["closed_in0to3_count"] == System.DBNull.Value
           ||row["closed_in4to7_count"] == System.DBNull.Value 
           || row["closedGtr7_count"] == System.DBNull.Value 
           ||row["Closed"] == System.DBNull.Value;
}
foreach (DataRow row in dt.Rows)
{
    if (IsRowInvalid(row))
    {
        continue;
    }

    // other logic here
}
internal static class ColumnName
{
    public const string AverageDays = "AverageDays";
    public const string Closed = "Closed";
}

Context

StackExchange Code Review Q#71747, answer score: 4

Revisions (0)

No revisions yet.