18
duckWit
6y

Time for a soap box rant.

I just found this in one of our projects. I've simplified the example to make it more anonymous.

When I see code like this it automatically means there is a lack of attention to enumerations and/or understanding of what they are.

One may argue that in a certain execution of code it's a minor performance hit and therefore insignificant. It's still a performance hit. Furthermore, it takes even less time to do it the right way than it does to do it the wrong way.

Every one of these lines will enumerate the list from the beginning to try and find that one element you're interested in. Big O notation, people.

Throw that crap into a dictionary or hashset or similarly applicable data structure with direct reads at the beginning of your logic so that it only gets enumerated ONCE when the data structure instance is created. Then access it however many times you want.

Soap box rant over.

Comments
  • 3
    Refactoring that into dictionary/hashset would feel so good. I'd almost do it for free...
  • 3
    @irene a grievous violation at multiple levels, for sure.
  • 1
  • 5
    @irene keeping this example within the scope of the original one, this would be super duper fast in comparison to what it was, and only enumerating the results object one time instead of 9 times.
  • 1
    @duckWit AND IT'S SO PRETTY
  • 2
    @duckWit Ohh, make a local function that takes Id as param so you won't have to repeat the access stuff?
  • 0
    @irene Elegancy over speed is my motto. Users hate me 🤓 Haha not really, but almost.
  • 0
    @irene you are correct, setting applicable properties within if statements of a single for/each iteration accomplishes the same goal of only enumerating it once. This is where my personal preference comes in though because I do tend to frown on excessive amounts of if/else blocks. More often than not they can be harder to read and harder to test.
  • 0
    @irene language is C#
  • 0
    @irene not here specifically, but numerous if else statements in the same sequence that return different values for example require multiple tests. After commenting about that above I quickly edited it to say they *can* require more tests. You are right, here they could be covered with the same single test. My preference remains less if/else blocks though if they can be avoided.
  • 1
    @irene more along this tangent I also happen to similarly avoid switch statements except when they are utilized in an isolated factory-like design pattern. We can certainly do that here, but we also accomplish essentially the same thing with the direct reads on the dictionary example I provided above. I don't feel in this case it would warrant a switch statement.

    Anyway, as always there are numerous ways to tackle it. The take away is to make sure you are enumerating as few times as possible. After that it largely depends on personal or team preference.
  • 0
    @jschmold Courier New
  • 2
    I would still get rid of the repeated lines. Imagine the amount of work if there suddenly is 15 properties instead of 9 and you have to add those in 20 places in code instead of just modifying a single constant/enum/array.
  • 2
    This looks like one of those cases that justifies using metaprogramming if the language supports it.
  • 2
    Oh god this is ugly on so many levels. Kill it with fire.
Add Comment