5
\$\begingroup\$

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

            }
        }
    }
}
\$\endgroup\$
5
  • 3
    \$\begingroup\$ Review in three words : Too much nesting \$\endgroup\$ Aug 7, 2014 at 14:38
  • 1
    \$\begingroup\$ @Bhathiya-JaDogg-Perera: I rejected your edit because (1) I see no evidence that the asker is specifically asking about style, and (2) language need not be explicitly specified when the appropriate language tag exists first on the question. \$\endgroup\$
    – Schism
    Aug 7, 2014 at 15:04
  • \$\begingroup\$ @Schism : (1) Sorry about that, I thought it was clearly about coding style. (2) I didn't know that. I'll add a comment in meta. \$\endgroup\$ Aug 7, 2014 at 15:10
  • 3
    \$\begingroup\$ Your row.ItemArray[1].ToString() should be stored in a variable because otherwise each time you access it you return in your datatable \$\endgroup\$
    – IEatBagels
    Aug 7, 2014 at 15:13
  • \$\begingroup\$ ` b_dataValue = true; if (b_dataValue)` is always going to return true. For the nested If under this section, consider switching to a Switch/Case statement. I also don't see where you are using rowreport_data0Sum (or similar) outside of assignments. You can eliminate ` if ((col.ColumnName == "report_data0") || (col.ColumnName == "report_data1") ...` since the column names are checked lower. \$\endgroup\$
    – AWinkle
    Aug 7, 2014 at 15:33

3 Answers 3

8
\$\begingroup\$

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 + "%";

}
\$\endgroup\$
7
  • \$\begingroup\$ if (dt.Columns.Count <= 0) don't you mean if (dt.Columns.Count < 2) since toe code always tries to get column 1? \$\endgroup\$
    – Snowbody
    Aug 7, 2014 at 15:55
  • 1
    \$\begingroup\$ @Snowbody, I don't know what the code means. I saw a check for "more than zero columns"; and to me if there are zero columns then stop now, then we never risk referencing a non-existent column. \$\endgroup\$
    – radarbob
    Aug 7, 2014 at 16:04
  • 1
    \$\begingroup\$ Yes, but what if there's 1 column? Then the only valid column is 0, and the reference to column 1 will cause an exception. \$\endgroup\$
    – Snowbody
    Aug 7, 2014 at 16:32
  • \$\begingroup\$ That bug is built into the original code. "Refactoring means never having to say you inadvertently fixed a bug." :) \$\endgroup\$
    – radarbob
    Aug 7, 2014 at 16:41
  • \$\begingroup\$ @Snowbody actually am iterating columns then why it will throw exception. \$\endgroup\$
    – SivaRajini
    Aug 7, 2014 at 16:46
6
\$\begingroup\$

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.

enter image description here

\$\endgroup\$
8
  • \$\begingroup\$ I understand that humor doesn't always translate well over the net, but I assure you that this is said with much respect and a heavy dose of sarcasm. \$\endgroup\$
    – RubberDuck
    Aug 7, 2014 at 16:42
  • 2
    \$\begingroup\$ I honestly didn't look too hard. I was just having a little fun. But microsoft has an official naming guideline that you can read through. \$\endgroup\$
    – RubberDuck
    Aug 7, 2014 at 16:53
  • 4
    \$\begingroup\$ Also @AWinkle, just because they teach this out-dated nonsense in university doesn't make it right. \$\endgroup\$
    – RubberDuck
    Aug 7, 2014 at 18:04
  • 2
    \$\begingroup\$ the name hasValue will tell you that it is a boolean \$\endgroup\$
    – Malachi
    Aug 7, 2014 at 21:12
  • 1
    \$\begingroup\$ True @malachi, but in a more readable way. \$\endgroup\$
    – RubberDuck
    Aug 7, 2014 at 21:21
3
\$\begingroup\$
  • 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);
    }
  }
}
\$\endgroup\$
5
  • \$\begingroup\$ @Snowboy "You're also missing the using statements." what is that? \$\endgroup\$
    – SivaRajini
    Aug 7, 2014 at 16:50
  • \$\begingroup\$ using directive: msdn.microsoft.com/en-us/library/sf0df423.aspx \$\endgroup\$
    – Snowbody
    Aug 7, 2014 at 16:51
  • \$\begingroup\$ that will be there in page top with required namespaces \$\endgroup\$
    – SivaRajini
    Aug 7, 2014 at 16:53
  • 1
    \$\begingroup\$ I know it's in your source code, but you didn't post it here. I guess it's obvious you meant they're from System.Data. I'll remove the mention. \$\endgroup\$
    – Snowbody
    Aug 7, 2014 at 17:03
  • \$\begingroup\$ yes i got it your point.Thanks for your support and help \$\endgroup\$
    – SivaRajini
    Aug 7, 2014 at 17:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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