Modifying a data table of report data

Question

Is there any way of optimizing this code?

private void ModifyDataTable(DataTable dt)
{
    if (dt != null)
    {
        bool b_dataValue = false;
        decimal rowreport_data0Sum, rowreport_data1Sum, rowreport_data2Sum, rowreport_data3Sum, rowreport_data4Sum;
        rowreport_data0Sum = rowreport_data1Sum = rowreport_data2Sum = rowreport_data3Sum = rowreport_data4Sum = 0;
        foreach (DataRow row in dt.Rows)
        {

            if (row.ItemArray[1].ToString() == "$150,000 & Over" || row.ItemArray[1].ToString() == "$150,000 - $199,999" || row.ItemArray[1].ToString() == "$200,000 - $299,999" || row.ItemArray[1].ToString() == "$300,000 & Over")
            {

                rowreport_data0Sum += GetRowValue(row, "report_data0");
                rowreport_data1Sum += GetRowValue(row, "report_data1");
                rowreport_data2Sum += GetRowValue(row, "report_data2");
                rowreport_data3Sum += GetRowValue(row, "report_data3");
                rowreport_data4Sum += GetRowValue(row, "report_data4");
            }
        }
        if (dt.Columns.Count > 0)
        {
            foreach (DataRow row in dt.Rows)
            {

                if (row.ItemArray[1].ToString() == "$150,000 & Over")
                {
                    foreach (DataColumn col in dt.Columns)
                    {
                        if ((col.ColumnName == "report_data0") || (col.ColumnName == "report_data1") || (col.ColumnName == "report_data2") || (col.ColumnName == "report_data3") || (col.ColumnName == "report_data4"))
                        {

                            if (!row.IsNull(col))
                            {
                                b_dataValue = true;
                                if (b_dataValue)
                                {
                                    if (col.ColumnName == "report_data0") row[col] = rowreport_data0Sum + "%";
                                    else if (col.ColumnName == "report_data1") row[col] = rowreport_data1Sum + "%";
                                    else if (col.ColumnName == "report_data2") row[col] = rowreport_data2Sum + "%";
                                    else if (col.ColumnName == "report_data3") row[col] = rowreport_data3Sum + "%";
                                    else if (col.ColumnName == "report_data4") row[col] = rowreport_data4Sum + "%";
                                }
                                else
                                {

                                    dt.Rows.Remove(row);

                                }
                            }
                        }
                    }

                }
            }
            if (b_dataValue)
            {
                for (int i = dt.Rows.Count - 1; i >= 0; i--)
                {
                    DataRow row = dt.Rows[i];
                    if (row.ItemArray[1].ToString() == "$150,000 - $199,999" || row.ItemArray[1].ToString() == "$200,000 - $299,999" || row.ItemArray[1].ToString() == "$300,000 & Over")
                    {
                        dt.Rows.Remove(row);
                    }
                }

            }
        }
    }
}

Answer 1

is there any possibility of optimizing the above code

Eliminating unnecessary processing will help...

Remove Needless if

if (!row.IsNull(col))
{
    b_dataValue = true;
    if (b_dataValue)
    {
        // ...
    }
    else
    {
        dt.Rows.Remove(row);
    }
}

b_dataValue is always true, so the if is not necessary, and the else will never execute. So ...

if (!row.IsNull(col))
{
    b_dataValue = true;
    {
        // ...
    }
}

Move If's to the top

If the table is null or has no columns then there's no point in going on. So check for this up front. This removes 2 nesting levels.

private void ModifyDataTable(DataTable dt) {
    if (dt == null) return;
    if (dt.Columns.Count <= 0) return;
    // ...
}

Reference by Table Name and Remove more If-ing

I'm guessing that your code is handling at least 2 different tables. If this is true then based on the name call "thisTable" code or "thatTable" code. Then you can eliminate all that checking for column names because you know they are there. And it eliminates another layer of if

private void ModifyDataTable(DataTable dt) {
    if (dt == null) return;
    if (dt.Columns.Count <= 0) return;
    if (dt.TableName == "thisTable") DoThisTableStuff(dt);
    if (dt.TableName == "thatTable") DoThatTableStuff(dt);

    // Knowing what table we're dealing with, code like this
   if (col.ColumnName == "report_data0") row[col] = rowreport_data0Sum + "%";
   else if (col.ColumnName == "report_data1") row[col] = rowreport_data1Sum + "%";

   // could turn into this
   row["report_data0"] = rowreport_data0Sum + "%";
   row["report_data1"] = rowreport_data1Sum + "%";

}

Answer 2

It seems that someone else has addressed the structural issues, so I want to talk about Naming. Naming is hard; I get it. Still, I cringe every time I see Hungarian notation. Particularly in c#. It makes my eyes bleed. Why? Because it's useless. The IDE will tell you that it's a Boolean.

So kill the Hungarian notation with fire and replace b_dataValue with hasValue.

Answer 3

• Does GetRowValue() refer to this table or to something else? It doesn't really describe where the row value comes from.

• I think there's a bug in your use of b_dataValue. What were you intending to do with it? In particular it is very dangerous to remove items from a collection while you're iterating over it. I'm not going to even touch that.

• As Radarbob said, it makes sense to return from the function immediately if you can't continue. This makes the code simpler and reduces nesting, by keeping the focus on the normal flow of the program.

• don't repeat yourself. If you find yourself writing the same or similar code over and over again, you're doing something wrong, and should look for a language feature to automate it. In particular
  • Use an array to store sequential data (e.g. the "report_data" column names and the rowreport_dataSums ) instead of using a bunch of separate variables.
  • When you have a bunch of things and need to check if something is included in the bunch, use a HashSet and Contains() or Dictionary and ContainsKey(), instead of using multiple clauses in the if.
  • Then you can use a for or foreach loop instead of copy-pasting.
• Definitely check dt.Columns.Count before referring to row.ItemArray[1] so you don't cause an exception.

Here's an implementation of my suggestions, though i can't guarantee its correctness because I have no idea what you intended with b_dataValue. That's a horrible variable name by the way, gives no indication of its purpose or meaning.

private static readonly Dictionary<string,bool> incomeBrackets =
{
  {"$150,000 & Over", false},
  {"$150,000 - $199,999", true},
  {"$200,000 - $299,999", true},
  {"$300,000 & Over", true }
};

private static readonly string[] headings =
{ 
  "report_data0",
  "report_data1",
  "report_data2",
  "report_data3",
  "report_data4"
};

private void ModifyDataTable(DataTable dt)
{
  if (dt == null || dt.Columns.Count < 2)
  {
     return;
  }

  bool b_dataValue = false;
  decimal[] rowreport_dataSum = new decimal[5];

  foreach (DataRow row in dt.Rows)
  {
     if (incomeBrackets.ContainsKey(row.ItemArray[1].ToString())
     {
        for (int i=0; i<5; i++)
        {
           rowreport_dataSum[i] += GetRowValue(row, headings[i]);
        }
     }
  }
  foreach (DataRow row in dt.Rows)
  {
    if (row.ItemArray[1].ToString() == "$150,000 & Over")
    {
      for (int i=0; i<headings.Length; i++)
      {
        row[headings[i]] = rowreport_dataSum[i] + "%";
      }
    }
  }
  for (int i = dt.Rows.Count - 1; i >= 0; i--)
  {
    DataRow row = dt.Rows[i];
    bool shouldBeRemoved;
    incomeBrackets.TryGetValue(row.ItemArray[1].ToString,out shouldBeRemoved);
    if (shouldBeRemoved)
    {
      dt.Rows.Remove(row);
    }
  }
}