4
\$\begingroup\$

DataTable looks like below

enter image description here

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

        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ 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)? \$\endgroup\$ – 200_success Dec 5 '14 at 8:44
  • \$\begingroup\$ @200_success that is complex stored procedure.attached the screenshot of resultant output of the query. \$\endgroup\$ – SivaRajini Dec 5 '14 at 8:51
  • \$\begingroup\$ 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. \$\endgroup\$ – 200_success Dec 5 '14 at 9:00
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. :) \$\endgroup\$ – Abbas Dec 5 '14 at 9:29
  • \$\begingroup\$ 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. \$\endgroup\$ – BCdotWEB Dec 5 '14 at 9:42
3
\$\begingroup\$

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);
    }
}
\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.