patterncsharpMinor
Classifying rows of data by the number of days to close
Viewed 0 times
rowsclosethenumberclassifyingdatadays
Problem
DataTable looks like below
I would like to simplify this code in a better manner.
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:
This method you then call like this:
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.
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.
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.