- Feature Articles
- CodeSOD
- Error'd
-
Forums
-
Other Articles
- Random Article
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
And what are the odds that you don't really need that
refinthis.SetAttendanceBatchNumber(batchNumber, ref batch);?Admin
Twentyfrist! Oh, wait, Twentysceond!
Admin
This is, IMO, wrong on a bigger level. It seems that the task is to find the number of attendees per session, via counting the number of submissions.
Skip the fancy schmancy LINQ... go straight to the source.
select session, count(1) from attendenceSubmission group by session. QED.
Admin
I think I'd just stop at "they're using Linq within a for-loop." I mean, WTF?
Also, ForEach, which is a built-in Linq WTF. (Thankfully not exhibited here.)
Admin
Generally I see ref used in .Net code that has been munged from an original VB.Net. (In my current code base, a lot of this nonsense started with VB6. Which at least shows that VB.Net was a successful idea.)
Admin
What, you don't want downstream code to change a database-driven list? That also happens to be a reference type anyway? We don't need none of that sanity here.
Admin
It's pretty much certain. I assume they passed it by ref to avoid making a copy. Except that it would NOT make a copy, since
List<T>is a reference type anyway... ref types/value types and passing by ref/passing by value are probably the most misunderstood aspect of .NET.Admin
I mean if that is a database lookup they could be trying to paginate the data. But if they're going to eventually fetch all items anyway... yeah this makes no sense.
Plus part of the appeal of using LINQ this way is you can evaluate each item you fetch as it is available without needing to wait for the whole set or having to allocate memory for it.. But using ToList or ToArray blocks on fetching the whole set and will get you a block of memory for it. There are certainly cases where it makes sense to fetch a whole set, usually when you need to iterate though it multiple times it is more efficient to cache the result first locally. But int his case that doesn't apply.
Admin
The ToList() is pointless in this case. Enumerables have a perfectly sensible Any() method for the following check. The only possible reason to do this (and it it stupid) is because the ref call requires a List rather than an Enumerable -- in which case just change the called method. (Yes, even if the called method insists on an IList. Do that internal to the method.) Which incidentally would make it obvious that the ref isn't needed, although I suppose you could always end with a reference to a different enumerable.
The whole thing smacks to me of (a) getting Linq wrong (b) getting database access wrong (c) misunderstanding how to paginate using a cursor, assuming that is an efficient thing to do in this case (d) otherwise, loading the entire set as a class member (e) absolutely everything else in this misbegotten code base being written by monkeys flinging faeces at the wall.
Of course, I could be wrong.
Admin
The ToList() is definitely not useless. It specifies that now is the time to evaluate.
If most of the time you want the results, it is more efficient to query the results and then use them (if there are any) than to ask are there any and then afterwards query again to get what results there are.
If however the count or Any() check is always needed and only very occasionally are the actual results needed, then the .ToList() should be removed as a count or any check is much more efficient than actually loading the results.
However in this case, the correct method is to paginate the results properly using LINQ methods rather than to run a new query for each batch.
Admin
I wouldn't call it totally useless. I hit a case where I bunch of primary records, each of which was linked to two secondary tables. Unfortunately if the secondaries are needed at all they are almost certainly needed in their entirety and in some scenarios a lot of records are needed. Unfortunately, this ended up coming down to read the primary records, then use the keys to decide which secondary records to read. Batching those secondary reads rather than trying to read them all with one command made a considerable improvement in performance.
Admin
Kinda feels weird to me that one using a List<> or HashSet<> (because it has a Count property) and do pagination. Paginations only make sense on an IQueryable not on an IEnumerable really.
Addendum 2023-02-08 16:49: Regrading the iteration: Well, instead of i++ the correct term would be i+=20 and the bug would be fixed. Still a horrible way to do it anyway; a while loop would be way better in combination with an async IQueryable call.
Addendum 2023-02-08 16:50: this ... ref ... shug
Admin
No not mix up actual LINQ [Language Integrated Natural Query] "from cust in customers select cust;" with the usage of Fluent Syntax to implement this as a set of methods [many different implementations "List<T> All(this List<t>) { return this; }"...then again fal to distinguish LINQ to SQL which does in fact build a SQL statement that is not executed untile an item from the result is needed.