• NoOne (unregistered)

    For some reason, the function goes through the extra step of iterating across the returned list and constructing a new one. There's no real good reason for this, though it does force LINQ to be eager- by default, the Where expression won't be evaluated until you check the results.

    In fact, it is .ToList() which forces LINQ to be eager, not the following iteration.

  • (nodebb)

    It doesn't matter how many operations you do on each item in the list, the function is still O(n), and that's what's important.

  • (nodebb)

    The lock is in there for thread safety

    Ah no, this is a deadlock scenario. When you use this code in a parallel or asynchronous context and the execution thread is in the same synchronization context it will just hang.

    So no, there's a reason why C# (and .net) doesn't feature eager locking with low level types, after all, you can only balance race conditions against dead locks and it's always a high level trade off on an architectural level and not an implementation detail.

    Addendum 2025-04-24 08:33: Oh and yes, lock (this) is the worst thing you can do (on so many levels, simply never do this), always use an synchronization anchor object or the modern Lock object.

  • (nodebb) in reply to MaxiTB

    Nit: lock (this) is perfectly valid IFF it's an internal field - avoids an extra allocation for the lock object. But in code meant to be visible higher up, it is indeed terrible.

  • (nodebb) in reply to Dragnslcr

    It doesn't matter how many operations you do on each item in the list, the function is still O(n), and that's what's important.

    Depends on what the operations are. If one of them involves a scan across the whole list (or some other list that is of necessity exactly equal in size), it becomes O(n squared)...

  • (nodebb) in reply to colejohnson66

    It's terrible, because you no longer have control over how the critical section is behaving due to someone from the outside potentially locking the object.

    That's the reason why since 1.0 it's considered an anti-pattern and best practice was to lock on a static readonly member instance on an object or since .net 9 use the Lock object instance: https://learn.microsoft.com/en-us/dotnet/api/system.threading.lock?view=net-9.0

  • (nodebb)

    We don't see how _foos is declared, but the whole lock situation could be avoided by using SynchronizedCollection<foo> for its declaration.

  • Mr. WTF (unregistered)

    I pity the Foos that don't have unique IDs

  • löchlein deluxe (unregistered)

    Ah, "I learned Jahava 1.3, and I know that whenever I tried to do anything with these datatypes, I got ConcurrentModificationExceptions. So I lock the object and copy it and then I can do how I please." (I'm kinda surprised it's a list and not "count the length and create an array", which is an optimization over "create and copy an array for each appended element")

    But yeah, after fifteen years in HEd admin, the only thing worse than a CompSci PhD is a CompSci PhD in admin and the only thing even worse than that is a jealous overaged CompSci MSc in admin.

  • LZ79LRU (unregistered)

    For some reason, the function goes through the extra step of iterating across the returned list and constructing a new one.

    There can be a good reason to do this. He is just doing it wrong. Looking at the code, my guess is that the original architect thinks he is making a copy of the list elements so that, when modified later, they do not reference back and alter the originals stored in the list. And there are legitimate reasons why you might want to do this. Unfortunately, if that is the case, he appears to be under the very wrong belief that adding an item to a list is by value. It is not.

Leave a comment on “Tangled Up in Foo”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article