Learne ye crafte

I had the dubious honour yesterday of reviewing some “interesting” code. If you’d been watching my twitter stream you’d have seen that [WTFs per minute metric][http://www.osnews.com/story/19266/WTFs_m] was pretty high. Now that I’ve had some time to calm down I wanted to share some of the things that I found unacceptable about this code.

Just in case the authors happen to stumble upon this post I want to make it clear that my intention isn’t to shame you (in fact I’ve taken steps to alter the code making it less recognizable). I’m certain that I have violated each of these on several occasions and I hope that others can learn some anti-patterns from this post.

1. Your repository should be fully contained

In other words I should be able to check out the latest version from source control and build and run your app. If there are special environmental considerations (like files that should be in certain folders or assemblies in the GAC), then those should be well documented and then those items and the document should be placed into your source repository.

A good mechanism for ensuring that what’s in the repository always builds is to perform an automated continuous integration. That way as soon as something “breaks the build” you will know and someone will go in and fix it. Try to keep your CI machine as clean as you can. If you have to install a tool or plugin on the CI box to get the build to work then check the installer for that tool or plugin into the repository and install it from there.

2. The worst form of comment is commented out code

Code can be in one of two states. Either it came from the repository or you wrote it in the current session. If it came from the repository then you can safely delete it knowing that you can always go and get it back out of your source control system later. If you just wrote it and you don’t want it in the production code base for some reason then DO NOT CHECK IT IN.

Checking in commented out code says to me that you don’t understand what the impact of not having that code there is (i.e. We might want that later if it turns out my change didn’t work). Take the time to actually understand what the code is doing and if some code is not necessary then delete it. Your brain can only hold so much stuff in it, don’t waste the room with irrelevant details.

The longer commented out code is left in the system the less relevant it becomes. This is because the rest of the code around it changes but those changes are not reflected in the commented out code. Frequently I’ve found commented out code that is only a few weeks old that calls methods whose signatures have changed or have been renamed, or deleted altogether, or moved to another class. I also come across code that accesses local variables that have been deleted. If you find commented out code in the repository just delete it. No-one else will have the courage, assuming that it’s someone else’s responsibility.

When you delete the code you might find that the surrounding code looks a little absurd. Here’s my favourite sample of the week:

public Letter ClientLetter_Get(string type)
{
    Letter rv = null;
    // rv = _client.Letters.Find(item => item.Type == type);
    return rv;
}

You don’t have to look too closely to see that this method now returns null all the time. Without the commented code the method has become superfluous as well. We should delete it as well and who knows how far that would cascade down. As it turns out this method is used during binding to determine whether certain controls are visible or not. As the method will always return null, the controls are never rendered so they and all their event handlers can now be deleted. Hundred of useless lines of code, just gone. Here’s one more:

if (_engagement == null)
{
    return "";
}
else
{
    return "";//_engagement.PersonnelRole.Personnel.Rep;
}

Even with the comment still intact this looks a little ridiculous. How much of the rest of this method (not shown above) is made completely superfluous knowing that we’re going to return an empty string no matter what?

3. Code blocks that do nothing are worthless at best and putting a comment in them explaining their purpose doesn’t help

This is one I haven’t come across that often but is rife in the code I’m reviewing so it may be more of an issue out there than I thought. Here is the most absurd case:

byte[] array = new byte[file.ContentLength];
int size = file.InputStream.Read(array, 0, file.ContentLength);
if (size < file.ContentLength)
{
    //err
}

Error handling via comments?! If you are going to go to the trouble of detecting an error condition then you had better be prepared to do something about it. If not, just delete the whole if statement and get on with it. There are some less obvious versions of this though. Here’s one

switch (e.CommandName)
{
    case "LOAD_XL":
        if (e.CommandArgument == "MainFile")
        {
            Display_File("MainFile");
        }
        else
        {
            Display_File("SecondaryFile");
        }
        break;
    case "UPLOAD_XL":
        break;
    default:
        break;
}

What is the point of having UPLOAD_XL in there? if you delete it then the default case will be executed anyway. Even that is superfluous as it’s blank so you can delete that too. That leaves you with a single case and in my books that’s an if statement. You could even clean up that existing if statement as well but that’s beyond the point here. Last one for this section:

if (personID == null)
{
    //null personID
}

That comment really clears things up for me, thanks. There’s a bunch of else-if clauses which makes me suspect that this is an attempt to prevent duplication but that’s a different scary story.

4. Copy and Paste is not reuse, it’s abuse

I’ve ranted on this topic before so I won’t go into too much detail here. If you express the same concept/algorithm in 20 distinct places then you will never be able to change that concept/algorithm. It’s time expand on the null personID sample from above:

object personID = Session[SESSION_ENGAGEMENT_PERSONNELID];
int PersonnelId = -1;
if (personID == null)
{
    //null personID
}
else if (personID.GetType() == typeof(string))
{
    if (!string.IsNullOrEmpty((string)personID))
    {
        PersonnelId = int.Parse((string)personID);
    }
}
else if (personID.GetType() == typeof(int))
{
    PersonnelId = (int)personID;
}

This exact piece of code is repeated verbatim in 5 places in one file. At the very worst it could be extracted as a method and called from 5 places. An even better solution would be to wrap a property around it (why? Because then you could provide a setter as well and completely encapsulate the Session access).

Also on a side note, if you look at what is going on here it seems as if the implementer doesn’t know what type he/she should expect to find in the session variable either. That’s a bit sad as a quick find all references tells me that it is set in 3 places and each one sets it to a string. Had the developer wrapped a property setter around the code that updates the session variable they would have had more confidence about what to expect from the session.

5. The ternary operator in C# has a time and a place

Many people don’t like the ternary operator (?:) in C# stating that they find it makes code more difficult to read. I think under some circumstances it can enhance readability but it’s use has be carefully managed. For instance, I would definitely use it here:

if (_personnel == null)
{
    return null;
}
else
{
    return _personnel.Updated;
}

To get the more terse (but I don’t think less readable):

return _personnel == null ? null : _personnel.Updated;

But probably not here where I think your eyes (and brain) have to jump around too much to figure out what is going on:

value.ApplyFilter(
    item =>
    (
        item.PersonnelRole.Personnel.Client.DeliveryManager == null ? false
        : (item.PersonnelRole.Personnel.Client.DeliveryManager.Name == _currentUserComment)
    )
     ||
    (
        item.PersonnelRole.Personnel.Client.Exec == null ? false
        : (item.PersonnelRole.Personnel.Client.Exec.Name == _currentUserComment)
    )
);

Anyone want to try and decipher that? Actually the original statement is a bit bigger and longer and I messed with the names to protect the innocent. Now it looks much tidier :)

To be continued…

I have much more to say about this code but I am getting kinda tired of looking at it at the moment and I’ve just been assigned a more urgent task. It’s possible that I’m just getting old and cranky (I turned 30 this week) but I’m pretty sure that most people out there would have been frightened by this code.

Posted by: Mike Minutillo
Last revised: 27 May, 2011 04:54 PM History

Comments

21 Jul, 2009 05:22 AM @ version 0

@Mark Freedman - No I have not and that would normally be a reasonable assumption. The problem is that the code I was asked to review is ready for hand-over to a maintenance team and so I have to assume it is in a complete form.

31 May, 2009 09:31 PM @ version 0

Great article, but in point 3, I'd be willing to give the coder the benefit of the doubt that he/she was creating temporary stubs. Have you discussed this code with the original author?

27 May, 2009 10:19 PM @ version 0

Great article :-)

Re: copy and paste...I once worked on a web app where I found a massive if else statement...which was copy/pasted a number of times inside another if else statement...all of which was copy pasted across a number of methods in the page...and finally the page itself was also clearly copy/pasted a number of times!

Oh yeah and there were literally hundreds of try-catch-DONOTHING statements.

oh the fun we have...

27 May, 2009 08:27 AM @ version 0

Yeah swallowed exceptions is one of those "to be continued" items. As is catching exceptions and then throwing a brand new exception that adds nothing to the game. With code like this around Michael Feathers should be a billionaire.

27 May, 2009 08:21 AM @ version 0

Oh the timing! We were just looking into a an old code base that is apprently a CORE library. Written in VB it is cr@p... Throwing Exceptions over the wire, Swallowing exceptions, dozens of paramters per method, out params, sql in webservices, introspection of the dll's location to determine what config to use (ie test/dev/prod), concrete dependencies on Oracle to use an email sender, horrifical violations of SOLID... seriously WTF! Oh well it will keep me employee for a few more contracts.

No new comments are allowed on this post.