Classifying rows of data by the number of days to close

Question

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);

        }
    }
}

Answer 1

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.

Answer 2

Creation of the additional columns

You should change the way you are adding the additional columns. By setting the DefaultValue property you simplify the for loop.

dt.Columns.Add("Averagedays", typeof(string));
dt.Columns.Add("0to3", typeof(string));
dt.Columns.Add("4to7", typeof(string));
dt.Columns.Add("Gtrthan7", typeof(string));  

should be changed to

dt.Columns.Add( new DataColumn(averageDays, typeof(String)) { DefaultValue = string.Format("{0:0.#}", 0) });
dt.Columns.Add( new DataColumn(range0To3, typeof(String)) { DefaultValue = string.Format("{0:P1}", 0) });
dt.Columns.Add( new DataColumn(range4To7, typeof(String)) { DefaultValue = string.Format("{0:P1}", 0) });
dt.Columns.Add( new DataColumn(gtrthan7, typeof(String)) { DefaultValue = string.Format("{0:P1}", 0) });

Style

You should extract the current and new columnnames to const variables.

Const String averageDays = "Averagedays";
Const String closed = "closed"; ....
row[averageDays] =
row[closed] =

This is less errorprone because if you then type it wrong for e.g row[] the compiler will warn you.You already have a spelling error there-> closed vs. Closed.

If..else

You should extract the value of row[closed] to a variable as you need this often.

Like

decimal closedValue = Convert.ToDecimal(row[closed]);  
if (closedValue > 0)
{
    decimal value = Convert.ToDecimal(row[totalDaysClose]) / closedValue ;
    row[averageDays] = string.Format("{0:0.#}", Math.Floor(value * 10) / 10);

    row[range0to3] = string.Format("{0:P1}", Convert.ToDecimal(row[closed_in0to3_count]) / closedValue);
    row[range4to7] = string.Format("{0:P1}", Convert.ToDecimal(row[closed_in4to7_count]) / closedValue);
    row[gtrthan7] = string.Format("{0:P1}", Convert.ToDecimal(row[closedGtr7_count]) / closedValue);

}

Update inspired by BCdotNET's answer

By using the IsRowInvalid() method which BCdotNET suggested, you can change the loop to

foreach (DataRow row in dt.Rows.Where(r=> !IsRowInvalid(r))
{

    decimal closedValue = Convert.ToDecimal(row[closed]);  
    if (closedValue > 0)
    {
        decimal value = Convert.ToDecimal(row[totalDaysClose]) / closedValue ;
        row[averageDays] = string.Format("{0:0.#}", Math.Floor(value * 10) / 10);

        row[range0to3] = string.Format("{0:P1}", Convert.ToDecimal(row[closed_in0to3_count]) / closedValue);
        row[range4to7] = string.Format("{0:P1}", Convert.ToDecimal(row[closed_in4to7_count]) / closedValue);
        row[gtrthan7] = string.Format("{0:P1}", Convert.ToDecimal(row[closedGtr7_count]) / closedValue);
    }
}

Answer 3

In addition to the other two answers, you may want to use the DataRow.IsNull method within BCdotNET's IsRowInvalid() method:

private bool IsRowInvalid(DataRow row)
{
    return row.IsNull("total_daysclose")
           ||row.IsNull("closed_in0to3_count")
           ||row.IsNull("closed_in4to7_count")
           ||row.IsNull("closedGtr7_count")
           ||row.IsNull("Closed");
}

The method handles both DBNull.Value and null values, and it is a little easier to read.