1
\$\begingroup\$

Is it possible to remove the inner loop by assigning data.Data.Tables[0].Rows[i].ItemArray directly?

object[,] arr = new object[52784, 21];

for (int i = 0; i < data.Data.Tables[0].Rows.Count; i++)
{
    for (int j = 0; j < data.Data.Tables[0].Columns.Count; j++)
    {
        arr[i, j] = data.Data.Tables[0].Rows[i][j];
    }
}

The reason why I need this code is because, I want to write to data of this dataTable to Excel using Interop which requires a 2-D array as input.

\$\endgroup\$
  • \$\begingroup\$ What kind of object is data.Data.Tables? \$\endgroup\$ – 200_success Sep 7 '15 at 8:47
  • 1
    \$\begingroup\$ This question has been mentioned on meta \$\endgroup\$ – Simon Forsberg Sep 7 '15 at 10:11
  • 1
    \$\begingroup\$ If you are using such external method that has that requirement and Rows isn't an object matrix Your algorithm is fine. If rows is an object matrix however you could just pass arr.Data.Tables[0].Rows \$\endgroup\$ – Bruno Costa Sep 8 '15 at 10:33
  • 3
    \$\begingroup\$ Note that it is a bit hard to review code like this because the code you are showing does not have any context. Unlike Stack Overflow, we prefer to look at real code instead of code that has been stripped of context for the purpose of showing it online. 52784 and 21 just looks like magic numbers right now, there's got to be meaning behind them. Note that hypothetical code is off-topic for Code Review, this question is very close to that line. \$\endgroup\$ – Simon Forsberg Sep 8 '15 at 12:46
  • 3
    \$\begingroup\$ @AnkitGoel Perhaps you should include the Excel Interop code as well. (There may be important clues/information in it that lead to a better answer.) \$\endgroup\$ – 410_Gone Sep 8 '15 at 14:52
6
\$\begingroup\$

If you need an object[,] your code is as good as i can see it being since a data table is an object[][] at its base (After digging through several different data structures that make up the DataTable. DataTable has DataRow[] which has DataColumn[] leading to an object[][] after boxing and pulling the actual data value).

Converting between the array structures will require code and while you may be able to do it with linq it would not be as readable as nested for loops (My personal opinion only).

The only enhancements I would suggest would be to store the row and column counts and limit the size of the array to the actual data size. This means you would not have data.Data.Tables[0].X.Count in multiple places and if you found a better way to figure out the counts would only need to change it in 2 places, one for the rows and one for the columns.

int rowCount = data.Data.Tables[0].Rows.Count;
int columnCount = data.Data.Tables[0].Columns.Count;

object[,] arr = new object[rowCount, columnCount];

for (int i = 0; i < rowCount; i++)
{
    for (int j = 0; j < columnCount; j++)
    {
        arr[i, j] = data.Data.Tables[0].Rows[i][j];
    }
}

You could use a foreach loop on the Row.ItemArray and maintain the count but I dont think it would be any faster

\$\endgroup\$
  • \$\begingroup\$ Your solution is same as mine. Seems like this is the only fastest way. thanks for your time. \$\endgroup\$ – Ankit Goel Sep 9 '15 at 5:18
5
\$\begingroup\$

Maybe you can remove the inner loop, but I don't think you should as it would make your code much harder to read and wouldn't be faster. This implementation is fine in my opinion.

Think about it, Excel works with rows and columns and actually your code reflects this pretty well, it is good!

Though, there are 3 things I'd like to point out about your snippet,

 object[,] arr = new object[52784, 21];

I'll assume you know why you use 52784 and 21, but I don't. What I mean is, when other developers are looking at your code, we can't know if these numbers are random or not. This problem is a code smell called Magic Numbers. To solve it, we must introduce a variable that'd make everything explanatory. Since your array must be the size of your DataTable, why not use the properties of the DataTable to initialize your array to the good values :

object[,] arr = new object[data.Data.Tables[0].Rows.Count, data.Data.Tables[0].Columns.Count];

Secondly, i, j and arr aren't the best variable names, I understand that when checking the code it is easy to see, but using rowIndex, columnIndex and excelFormatted (this one isn't that good, ok) would be better!

Finally, since you access your DataTable a lot, you should store it in a variable, it will save you the cost of using the DataTable's indexer every time.

var table = data.Data.Tables[0];
object[,] excelFormatted = new object[table.Rows.Count, table.Columns.Count];

for (int rowIndex = 0; rowIndex < table.Rows.Count; rowIndex++)
{
    for (int columnIndex = 0; columnIndex < table.Column.Count; columnIndex++)
    {
        arr[rowIndex, columnIndex] = table.Rows[rowIndex][columnIndex];
    }
}
\$\endgroup\$
  • \$\begingroup\$ Your solution is same as mine. Seems like this is the only fastest way. thanks for your time. \$\endgroup\$ – Ankit Goel Sep 9 '15 at 5:18

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.