There is bad code, and then there is…

…this gem:

public static void Do(ICollection people, string evaluationLogic)
{
for (int i = 0; i > people.ToList().Count; i++)
{
if (evaluationLogic == null) break;
if (evaluationLogic == string.Empty) people.ToList()[i].Name = "Bob";
if (evaluationLogic.Length >= 0) people.ToList()[i].Name = "Jim";
}
}

(mind the tab spacing – I’m still working out the kinks in some code formatters)
Psuedo-coded from actual production code.  In a site that was written this side of the 2000’s.  I will follow up this post with my commentary on hiring practices and some ways to improve getting good coding talent on your team, but suffice to say – whoever wrote this should not be writing production code.

Let me reiterate this – I actually found this live in production.

Where to even begin?  The Do() method is obviously a shell – but the point is you are given an ICollection – which has no indexers.  Some developers carry a preference to ToList() conversion, while some (like me) try to stick as closely to the least specific collection interface – which ends up being IEnumerable the vast majority of the time.  The advantage of coding against IEnumerable is it forces you to think of the target collection as an abstraction, allowing you to use the most functional and/or expressive LINQ statements to encapsulate your intent, without getting buried in nested for-loops.  Let’s call out the specifics:

  1. Repeated ToList() conversion.

Calling:

new List();

is valid C#.  It builds and runs.  It also does nothing for you as a developer.

Calling:

var content = new List();

instructs the runtime to assign a new collection object onto the heap, and store the reference to it in your given variable – here ‘content’.  The first snippet simply allocates an object, but does not capture a reference to it – so the GC will simply pick it up whenever it feels like.

When you feed a ToList() conversion into that first for-loop ‘i’ variable, you’re doing the exact same thing – allocating stuff onto the heap – accessing an int for a brief instant, but instantaneously releasing the reference, meaning your conversion was wasteful, and is needlessly stressing out the GC.

2. For vs ForEach.

The posted code seemed to utilize the poor ToList() conversion due to the need for the ‘i’ indexer – but when working with the collection interfaces, more than likely a foreach more than suffices.  The only reason not to is if you need to place a new object or change the reference of the object itself – with the foreach placeholder variable, you wouldn’t be able to do this.

3. Multiple redundant evaluations.

Notice the three if() checks?  Redundant evaluations.  The code I had assessed had mutually exclusive conditions, but was constantly rechecking them as part of normal loop flow.  An else or switch is everyday practice.

4. Break.

Code smell.  In something that isn’t algorithmic in nature (say, a performance optimized extension method, or just a highly performant loop), I find break to be awkward to use in everyday code.  If you’re breaking out of a for loop under a condition, then most of the time you can easily migrate to a LINQ statement that neatly encapsulates your intent.  Please don’t make me figure out your 10 lines of micro-optimized looping code (read: premature optimization), I have other things to do with my time.

2c96a9fa4c6b4c3cc0fe8322e13ae534

 

Leave a Reply

Your email address will not be published. Required fields are marked *