# Classifying rows of data by the number of days to close

DataTable looks like below

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

DataTable dt = results.Results[0].DataTable;

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

}
}
}

• Could you provide more background? What is the goal of this code? Where do the data come from — an SQL query? If so, could you include the query (and the schema, if it is necessary to help us understand the query)? – 200_success Dec 5 '14 at 8:44
• @200_success that is complex stored procedure.attached the screenshot of resultant output of the query. – SivaRajini Dec 5 '14 at 8:51
• I strongly suspect that a proper optimization of the datatable code should start with analyzing the stored procedure that produced the mess to begin with. Perhaps you should post a related question about the stored procedure. – 200_success Dec 5 '14 at 9:00

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.

• Nice answer, I would only reverse the IsRowInvalid to IsRowValid. I'm more a fan of positive checks and in this case you wouldn't need the continue. :) – Abbas Dec 5 '14 at 9:29
• I had it as IsValid, but then you'd still have the code nested inside an if; I prefer the continue. Also IsValid changes to return to the same logic as the first if in the original code, and I wasn't too keen on that. – BCdotWEB Dec 5 '14 at 9:42

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


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

}


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


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.