1
\$\begingroup\$

I've used this code to get the sum of the values of each column and display in footer. Here, the advantage is that I don't have to look for the column name, just get the count of total number of rows and columns and then iterate over each row and column.

Please suggest how I can make this code more efficient or if there is any way to make this code block more effective.

#region column sum and show in footer
//loop all the rows in the datatable
int[] RowTotals = new int[dt_.Columns.Count];
foreach (DataRow row in dt_.Rows)
{
    if(dt_.Rows.IndexOf(row) !=0)
    {
        //loop all the columns in the row
        for (int i = 1; i < dt_.Columns.Count; i++)
        {
            //add the values for each cell to the total

            RowTotals[i] += Convert.ToInt32(row[dt_.Columns[i].ColumnName]);
        }
    }

}

//loop all the columns again to set the values in the footer

for (int i = 1; i < dt_.Columns.Count; i++)
{

    GridView1.FooterRow.Cells[i].Text = string.Format("{0:N2}", RowTotals[i]);
}
///////
#endregion
\$\endgroup\$
2
\$\begingroup\$

What about rewriting your code somehow like that:

var columns = dt_.Columns;
var rows = dt_.Rows.OfType<DataRow>();

for (int i = 1; i < columns.Count; i++)
{
    var columnName = columns[i].ColumnName;
    var columnTotal = rows.Skip(1) // instead of dt_.Rows.IndexOf(row) !=0
                          .Sum(r => Convert.ToInt32(r[columnName]));

    GridView1.FooterRow.Cells[i].Text = $"{columnTotal:N2}";
}
\$\endgroup\$
  • 1
    \$\begingroup\$ .OfType<DataRow>() this is not necessary as all items are always of type DataRow. Although this imporvement makes the first loop more compact the problem remains the same which is, the loop calculates the total and updates the grid at the same time. \$\endgroup\$ – t3chb0t Jul 20 '17 at 6:19
  • \$\begingroup\$ Hmm.. Before to write .OfType<DataRow>() I went to Reference Source and found that DataTable.Rows returns DataRowCollection which doesn't implement IEnumerable<DataRow>. \$\endgroup\$ – Maxim Jul 20 '17 at 6:32
  • \$\begingroup\$ You need to call dataTable.AsEnumerable() extension first to make it enumerable. Probably you'll need to add a using for DataTableExtensions but even without it, it'd be better to say .Cast<DataRow>() I guess. \$\endgroup\$ – t3chb0t Jul 20 '17 at 6:34
  • \$\begingroup\$ AsEnumerable obviously is better than OfType<DataRow>. But as you can see Rows doesn't return IEnumerable<DataRow> :) \$\endgroup\$ – Maxim Jul 20 '17 at 6:37
1
\$\begingroup\$
#region column sum and show in footer

Before you start optimizing this block you should move it into a separate method. The region clearly indicates that it's a part of something bigger that is doing too much. I find it's pointless to try to optimize this as long as it's a region of something else.

Currently your method not only calculates the RowTotals but it also updates a grid-view. This should not be done by the same method because you cannot test whether the totals are calculated correctly. The only way to validate it is to look at the grid. This is very unreliable.

Basically it should look like this: you have two worker methods

int[] CalculateRowTotals(DataTable dataTable) { ... }

void UpdateTotalsGridView(int[] rowTotals) { ... }

and one method that combined the results of both:

void DisplayRowTotals(..)
{
    var rowTotals = CalcualteRowTotals(..);
    UpdateTotalsGridView(rowTotals);
}

About the code itself...

if(dt_.Rows.IndexOf(row) !=0)

It is not clear why you are doing this. A helper variable or method would be nice so that you can give it a meaning like I don't know isHeader or something that makes sense.

\$\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.