Thursday, November 29, 2007

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.

6 comments:

PandaWood said...

"If it's an obscure method, it might not get called at all until production deployment if test coverage is low"

WTF...?;-) If you're coverage is that low, you deserve to get this exception! ;-)

The fact that you're worried about this exception 'sneaking' through would be more worrying to me.

terry said...

if(false) itself is funny, but there's actually a good bit of smarts to take from it.

i don't know much about the .net compiler, but i know java will remove all if(false) blocks of code completely from the executable at compiletime. also, knowing that java treats literal constants like C does and goes through and replaces at compiletime, you can (with some code or compile time flags) change the efficiency of some code.

say you have a ton of logging in your application. while some people will say it's negligible, removing millions of if(log.isDebugEnabled()) calls can speed things up. so instead of writing those if statements everywhere, you can have some constants established and write

if(DEBUG_LOGGING) { ... }

once you set that to false, the debug logging is stripped and it's no longer doing anything to adversely affect the app's performance.

so, it's not directly if(false), but if you were watching the compiler, i guess you'd come across that.

Jimmy Bogard said...

Both log4j and log4net allow you to control that through configuration, so you never need "if (DEBUG)". It's pretty easy to design simple API to do the same: "Log.Publish('blarg')" could be sprinkled everywhere, and inside "Publish" it looks at configuration to see what to do and how to do it, including doing nothing.

We do it all the time with our logging, giving different levels of verbosity any time we put messages out, i.e. "Log.Publish('blarg', Level.Information)". When we run into issues, we don't have to recompile or redeploy, just change configuration to dial up or dial down the trace/debug messages.

terry said...

it's not a matter of configuration, that's why there's the if(log.isDebugEnabled()) stuff. you can go through that, and it'll go off configuration, but in that case (as well as assertions), it's nice to turn off code (assertions with compiler options, debugging with constants and compiler optimization) to speed up applications.

especially in verbose logging situations (which lots of applications are), you're doing several calls to another method which adds up.

configuration doesn't fix that. it helps (you could just do log.debug() and wait for it to get all the way down to where it's ready to write, and then backs out - which is why most people use if(log.isDebugEnabled()), but it doesn't solve the whole thing.

Jimmy Bogard said...

In .NET, you can use the "Condition" attribute, and the code won't get compiled. For example, "Debug.WriteLine" in .NET won't make it to any assembly compiled in Release mode.

A bunch of "if (log.isDebug)" still is duplication. If I'm concerned about run-time performance, I'll go for the "Condition" attribute.

Where I am though, cost to compile & deploy >>>> cost to change configuration. Plus we make really fast servers.

I don't buy the notion of a high cost of calling methods either, show me the profiling data first. It's usually external dependencies (services, DB, file I/O) that causes slowness, and that's easily discoverable with profiling tools.

terry said...

i think repetitive calls add up. obviously, just a few hundred don't, but several thousand do. several thousand logger checks over several thousand executions adds up.

there's no "don't compile" attribute in java, so you have to resort to using constants for compiler optimization. that said, asserts can be turn on/off at compile time if you're compiling for a release instead of a debug build.