Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

            }
        }
    }
}
share|improve this question
3  
Review in three words : Too much nesting –  JaDogg Aug 7 at 14:38
1  
@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. –  Schism Aug 7 at 15:04
    
@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. –  JaDogg Aug 7 at 15:10
3  
Your row.ItemArray[1].ToString() should be stored in a variable because otherwise each time you access it you return in your datatable –  TopinFrassi Aug 7 at 15:13
    
` 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. –  AWinkle Aug 7 at 15:33

3 Answers 3

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

}
share|improve this answer
    
if (dt.Columns.Count <= 0) don't you mean if (dt.Columns.Count < 2) since toe code always tries to get column 1? –  Snowbody Aug 7 at 15:55
1  
@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. –  radarbob Aug 7 at 16:04
1  
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. –  Snowbody Aug 7 at 16:32
    
That bug is built into the original code. "Refactoring means never having to say you inadvertently fixed a bug." :) –  radarbob Aug 7 at 16:41
    
@Snowbody actually am iterating columns then why it will throw exception. –  SivaRajini Aug 7 at 16:46

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

share|improve this answer
    
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. –  RubberDuck Aug 7 at 16:42
2  
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. –  RubberDuck Aug 7 at 16:53
3  
Also @AWinkle, just because they teach this out-dated nonsense in university doesn't make it right. –  RubberDuck Aug 7 at 18:04
2  
the name hasValue will tell you that it is a boolean –  Malachi Aug 7 at 21:12
1  
True @malachi, but in a more readable way. –  RubberDuck Aug 7 at 21:21
  • 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);
    }
  }
}
share|improve this answer
    
@Snowboy "You're also missing the using statements." what is that? –  SivaRajini Aug 7 at 16:50
    
using directive: msdn.microsoft.com/en-us/library/sf0df423.aspx –  Snowbody Aug 7 at 16:51
    
that will be there in page top with required namespaces –  SivaRajini Aug 7 at 16:53
1  
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. –  Snowbody Aug 7 at 17:03
    
yes i got it your point.Thanks for your support and help –  SivaRajini Aug 7 at 17:05

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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