The onslaught of bad variable names

I was poking around in a C# method the other day, trying to find out what the methods did at each level; it was an arduous task because of the complex nature of the process, made worse by the programmer who coded this method.

I blame the programmer because somewhere along the way, he coded something like the following (I’ve simplified it for your sanity and my own):

public static ArrayList TransformerHelper(params object[] inputArray)
{
    ArrayList result = new ArrayList();
    foreach (object item in inputArray)
    {
        if (obj is IContainer)
        { 
            result.Add((IContainer)item);
        }
    }
    return result;
}

There was really no way to tell why the programmer was doing this, and the variable names he chose didn’t help any. There are at least (5) problems with the preceding code:

1. TransformerHelper: What is it helping to transform? Why not say ConvertToContainerArray();?

2. the parameter name and type for inputArray is useless at best, and dangerous at worse.  Can any object in the world really come into the method?  In the method itself we’re checking for an object that implements the IContainer interface, so we should restructure our code to reflect that to all callers (most developers that write this code hide behind the, “But we’ve got to be flexible” mantra. If you ever hear that, grab a pitchfork. 

3. result is named poorly. Result of what? Of some unknown calculation?  What exactly is going on in there?

4. result is not type safe.  We know what types are going into that ArrayList, so we should use a generic collection to make our intent clear.  There’s also a performance improvement with using a generic list over an ArrayList.

5. There’s a double-cast in the body of the method.  The is operator checks to see if it’s of the type on the righthand side, and there is an explicit cast below it.  It’s a micro-optimization (unless there are a lot of objects in the Array coming in), but it’s useful to clean it up.

Bonus: Since this converts one type to another, we can make this an extension method and use it more idiomatically.

Here’s that same method after it has been cleaned up:

/// <summary>
///Used to Convert IBucketLists to Container.
///Not every type that implements IBucket also implements IContainer.
///This is used to convert the ones that do. The others we're not worried about.
/// </summary>
public static IList ToContainerList(this IList buckets)
{
    IList convertedList = new List();
    foreach (IBucket bucket in buckets)
    {
        IContainer container = bucket as IContainer; 
        if (container != null)
        { 
            convertedList.Add(container);
        }
    }
    return convertedList;
}

Much better. It hasn’t lost any of the meaning, and it will work as expected now. The next programmer that has to look at it should be able to tell what was going on without comments.

When you’re working with code, you’re not just solving the problem for you, you’re solving it for the next programmer that has to look at this code, and the next, and the next, and the next.  Code makes or breaks your business, and the harder it is to modify, the more likely your business will fail.  If you’re going by a method without cleaning it up, you’re gambling with your livelihood.