C#•3y ago

❔ best practices for updating objects using GroupBy

i've got a collection of data objects. these data objects are related by domain-specific ids, and what I want to do is group them so i can update a property on every other group.

i've got the following set up so far, and it works:

var group = this.results.GroupBy(x => x.DomainSpecificId).Where((i, idx) => idx % 2 ==0);

foreach (var groupedData in group)
{
    foreach (var data in groupedData)
    {
        data.PropertyToUpdate = true;
    }
}

var group = this.results.GroupBy(x => x.DomainSpecificId).Where((i, idx) => idx % 2 ==0);

foreach (var groupedData in group)
{
    foreach (var data in groupedData)
    {
        data.PropertyToUpdate = true;
    }
}

however, anyone who has read anything introductory about performance might feel as uneasy as i do about a loop nested within a loop.

am i worrying about the performance aspect of this too much, or am i missing a better way to do this? it looks like a lot of the top StackOverflow answers about this just use the nested loops to perform the iteration.

Anton•2/27/23, 10:32 PM

yes, you are worrying too much about performance.
you don't seem to understand time complexity. The code you showed, despite it including a nested loop, is O(N). what you should be wary about are nested loops over the same collection, O(N^2) complexity, and greater.

Anton•2/27/23, 10:47 PM

your code is perfect in terms of complexity

gr8stalinOP•2/27/23, 10:58 PM

thank you, it's far from the freshest thing in my mind, the nested loops part was the only thing i could recall while writing

Denis•2/28/23, 8:45 AM

The only thing I'd suggest is avouding using

. Use somethign more descriptive, like or

book

book

Denis•2/28/23, 8:46 AM

It makes the code much more readable

ero•2/28/23, 9:28 AM

i mean i wouldn't say you're worrying too much. we don't know how time critical this code is, and it can absolutely be optimized a whole bunch

ero•2/28/23, 9:29 AM

but the code it would take to optimize this might not be worth it. this could take hours to perfect

DDenis The only thing I'd suggest is avouding using `x`. Use somethign more descriptive...

Anton•2/28/23, 10:10 AM

bad advice in this case. the variable scope is too small for this to matter

ero•2/28/23, 10:10 AM

what?

ero•2/28/23, 10:10 AM

readability always matters

ero•2/28/23, 10:10 AM

how is that bad advice

Anton•2/28/23, 10:11 AM

that can compromise conciseness in cases like this

ero•2/28/23, 10:11 AM

it cannot

ero•2/28/23, 10:12 AM

at least use the beginning letter of the type that it is

ero•2/28/23, 10:12 AM

like for

User

User

or for

D.Mentia•2/28/23, 3:36 PM

Yeah the implementation already looks fine regarding the nested loop, but if that code is all it's doing, there's no point to the GroupBy - the Where already does it all... maybe?
Well. Maybe not, you could get every odd ID, but that isn't necessarily every other entry. But either way, I think the linq can possibly be simplified one way or another,

D.Mentia•2/28/23, 3:42 PM

performance wise, I'd be most concerned with the linq itself, depending on your .net version... newer linq is pretty fast, and IDK when they got fast or how fast they really are, but it looks like you've got at least 2 extra iterations over the data from the linq query, before you iterate a third time
Which... really isn't a huge deal, in big O 3N is still just N, but it's also still 3x more processing. You can probably write it in a way that iterates only once, with a regular for loop, if performance is a huge concern (but it's not worth it unless this is like the bottleneck)

Accord•3/1/23, 4:03 PM

Was this issue resolved? If so, run

/close

/close

otherwise I will mark this as stale and this post will be archived until there is new activity.