• Hans (unregistered)

    And what are the odds that you don't really need that ref in this.SetAttendanceBatchNumber(batchNumber, ref batch);?

  • Ollie Jones (unregistered)

    Twentyfrist! Oh, wait, Twentysceond!

  • The Dave G (unregistered)

    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.

  • Sole Purpose Of Visit (unregistered)

    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.)

  • Sole Purpose Of Visit (unregistered) in reply to Hans

    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.)

  • guest (unregistered) in reply to Hans

    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.

  • (nodebb) in reply to Hans

    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.

  • (nodebb)

    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.

  • Sole Purpose Of Visit (unregistered) in reply to The_MAZZTer

    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.

  • staircase27 (unregistered)

    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.

  • (nodebb) in reply to The_MAZZTer

    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.

    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.

  • (nodebb)

    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

  • (nodebb)

    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.

  • Argle (unregistered)
    Comment held for moderation.
  • Bob Frankston (unregistered)
    Comment held for moderation.
  • David Mårtensson (unregistered) in reply to staircase27
    Comment held for moderation.

Leave a comment on “Skip to the Loo”

Log In or post as a guest

Replying to comment #:

« Return to Article