C#•2y 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.

10 Replies

Anton•2y ago

1. yes, you are worrying too much about performance. 2. 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. your code is perfect in terms of complexity

gr8stalin's mustache!OP•2y ago

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•2y ago

The only thing I'd suggest is avouding using x. Use somethign more descriptive, like student or book It makes the code much more readable

ero•2y ago

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 but the code it would take to optimize this might not be worth it. this could take hours to perfect

Anton•2y ago

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

ero•2y ago

what? readability always matters how is that bad advice

Anton•2y ago

that can compromise conciseness in cases like this

ero•2y ago

it cannot at least use the beginning letter of the type that it is like u for User or s for Student

D.Mentia•2y ago

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, 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•2y ago

Was this issue resolved? If so, run /close - otherwise I will mark this as stale and this post will be archived until there is new activity.

Gaming

Programming

❔ best practices for updating objects using GroupBy