Hall of shame

We keep a “Hall of Shame” of WTF-level code snippets to remind us that however bad we might think things get, it could always be worse.  It also serves as a reminder to us that we can’t be complacent with ignorance, and lack of strong technical leadership in critical applications can have far-reaching negative consequences.  Here are a few fun examples.

Runtime obsolescence

I’ve seen this when we have members that we don’t want anyone to use anymore.  Unfortunately, it didn’t quite come out right:

public static bool Authenticate(string username, string password)
{
    throw new Exception("I'm obsolete, don't call me, bro!");
}

Nobody knows not to call this method until their codebase actually executes this code at runtime, which could be at any phase in development.  If it’s an obscure method, it might not get called at all until production deployment if test coverage is low.

We want to communicate to developers at compile-time that members are obsolete, and we can do this we the ObsoleteAttribute:

[Obsolete("Please use the SessionContext class instead.", true)]
public static bool Authenticate(string username, string password)
{
}

Both the IDE and compilers support this attribute to give feedback to developers when a member should no longer be used.

Exception paranoia

I’ve seen exception paranoia mostly when developers don’t have a thorough understanding of the .NET Framework.  A try…catch block is placed around code that can absolutely never fail, but juuuuuust in case it does:

public static MyIdentity Ident
{
    get
    {
        if (((HttpContext.Current != null) && (HttpContext.Current.User != null)) && (HttpContext.Current.User.Identity != null))
        {
            try
            {
                return (HttpContext.Current.User.Identity as MyIdentity);
            }
            catch (Exception exception)
            {
                ExceptionManager.Publish(exception);
                return null;
            }
        }
        return null;
    }
}

This is also another case of TCPS.  The “if” block already takes care of any failures of the “try” block, and as we’re using the “as” operator, the downcasting can never fail either.  The 13 lines of code above were reduced to only one:

public static MyIdentity Ident
{
    get
    {
        return HttpContext.Current.User.Identity as MyIdentity;
    }
}

If HttpContext.Current is null, I’d rather throw the exception, as my ASP.NET app probably won’t work without ASP.NET up and running.

Clever debug switches

This is probably my favorite Hall of Shame example.  So much so that we named an award at my previous employer after the pattern.  Sometimes I like to insert a block of code for debugging purposes.  If I want my debug statements to live on in source control, I have a variety of means to do so, including compiler directives, the ConditionalAttribute, or even source control to look at history.  A not-so-clever approach we found was:

public void SubmitForm()
{
    if (false)
    {
        MessageBox.Show("Submit button was clicked.");
    }
}

To make the code run, you change the block to “if (true)”.  This had us cracking up so much we named our Hall of Shame award after it, calling it the “If False Award”.  Whenever we found especially heinous or corner-cutting code, they deserved the “If False Award”.  We never awarded ignorance, only laziness.

Opportunities to improve

It’s easy to let a “Hall of Shame” create negative and unconstructive views towards the codebase.  We always used it as a teaching tool to show that we would fix ignorance, but we would not tolerate laziness.

Every developer starts their career writing bad code.  Unfortunately, some never stop.