Is this correct way to use loops or should it be in LINQ?

Posted on

Problem

The results variable is coming from an API that have a arrays in arrays, etc.

I have made a foreach with for loops inside, which looks like this:

foreach (var s in results)
{
    var dataRowsCollection = s.ResultRows;

    for (int g = 0; g <= dataRowsCollection.Count; g++)
    {
        for (int i = 0; i <= dataRowsCollection[g].ItemArray.Count(); i++)
        {
            var dataRows = dataRowsCollection[i].ItemArray;
            var Title = dataRows[0].ToString();
            var Url = dataRows[4].ToString();
            var TemplateId = dataRows[6].ToString();
        }
    }

}

Can I do this in LINQ or should I keep it like this? I can’t figure out how it would look like with LINQ since its the array objects I’m working with.

Note: I will later on map Title, Url and TemplateId to my model class properties.

Solution

As a general review of the existing code: This block does not make much sense to me:

for (int g = 0; g <= dataRowsCollection.Count; g++)
{
    for (int i = 0; i <= dataRowsCollection[g].ItemArray.Count(); i++)
    {
        var dataRows = dataRowsCollection[i].ItemArray;
        var Title = dataRows[0].ToString();
        var Url = dataRows[4].ToString();
        var TemplateId = dataRows[6].ToString();
    }
}

Your inner loop will only ever pull data out of the first N entries in dataRowsCollection depending what the longest ItemArray in any of the data rows is because you are doing this: dataRowsCollection[i].ItemArray;.

If we assume that each data row has an ItemArray of length 10 then you will pull the first 10 entries out of the dataRowsCollection over and over again. This seems broken.

So instead of worrying about “should I use LINQ instead of a for loop” you should worry about getting your code to work first properly. First make it work – then make it better.

Before you ask yourself “can I do this with LINQ?“, you need to look at the body of your loop:

var dataRows = dataRowsCollection[i].ItemArray;
var Title = dataRows[0].ToString();
var Url = dataRows[4].ToString();
var TemplateId = dataRows[6].ToString();

These variables are scoped to the loop’s body, which means your loop is essentially useless, because each iteration will re-assign a new value to a new variable that will become candidate for garbage collection as soon as the iteration finishes.

I suspect that you haven’t posted the entire loop body… or that your code doesn’t do what you think it does.

Note: I will later on map Title, Url and TemplateId to my model class properties.

This has to be done inside the loop’s body.

As it stands / as posted, there’s nothing in this loop that makes it a good candidate for a more LINQ representation that would be more readable and/or easier to maintain.

LINQ stands for Language-INtegrated Query, which means it’s useful for querying objects. Your loop isn’t doing that – it’s iterating objects, and then performing assignations.

To LINQify your loop, you would have to change your logic – query dataRowsCollection and project (Select) each ItemArray into an IEnumerable<T> that contains each item’s dataRows.

And then you could iterate the result of that query to assign your model class properties.


Naming

Naming conventions make our lives easier. Respecting them makes your code easier to follow and to understand. Local variables should be camelCase, like dataRows is.

The other local variables you’re declaring are PascalCase, which makes your naming inconsistent.

Leave a Reply

Your email address will not be published. Required fields are marked *