Wednesday, March 21, 2012

Static Variables Considered Harmful

In my last post, I mentioned that a particular programmer had abused static initializers. I've been thinking more about this, and decided that the real problem was that this programmer actually abused static variables. The initializer abuse was simply a side effect, an attempt to make a bad design work.

To start, I'm going to give you my thoughts on static initializers: they're a code smell in and of themselves. If you're not familiar with a static initializer, here's an example: we have a static Map used for a lookup table, and want need to populate it before use. The static initializer is run at the time the class is loaded, before any code is allowed to use the class, so we can guarantee that the map will be populated.

public class MyClassWithALookupTable
{
    private static Map<String,String> states = new HashMap<String,String>();
    
    static
    {
        states.put("AK", "Alaska");
        states.put("AL", "Alabama");
        // ...
    }

This sort of code is, in my opinion, the only valid use for a static initializer: initializing an object that will never change, using code that is guaranteed never to throw. In effect, creating a constant.

The “guaranteed never to throw” part is important: static initializers run before any code is allowed to use the class; what happens if they do throw? The language spec says that static initializers are not permitted to throw checked exceptions, but they are allowed to throw unchecked exceptions (although a smart compiler will make you hide the exception in a called method). If the initializer does throw, you'll get an ExceptionInInitializerError the first time you access the class.

How do you debug such errors? You can't use a debugger, because the class has to be loaded to do set a breakpoint. You can insert a logging statement — assuming that your logging framework is able to initialize (one of the more painful things I've ever debugged was with a homegrown addition to Log4J that had a static initializer that threw). Even if you log the exception, you have to find where it's being thrown: people who abuse static initializers tend to do it a lot, so you might have to work through a long list of classes, adding exception handlers to each, before you find the one that's broken.

OK, so static initializers are a code smell. How do I make the leap from there to “static variables considered harmful?”

Going back to the legacy application in question, the original programmer used a static initializer to populate a lookup map. But the values to populate that map came from Hibernate. And since Hibernate was accessed through a DAO loaded as a Spring bean, he had to create a way to access the application context in a static manner. So he created a new class, with a static variable to hold the app context. And he wired this class into the DispatcherServlet configuration.

It's a long list of hacks, but here's the thing: every one of them logically proceeds from the one before it. And it comes down to the lookup table whose contents are allowed to vary.

There are certainly other solutions that he could have used. For example, I replaced the static initializer with a Spring post-construct from the associated DAO. But this is only one example of how static variables (versus constants) can lead you astray. More common are concurrency bugs, or “how did that value get there?” bugs. It seems a lot smarter to simply say “don't do this,” with the caveat for experts of “don't do this (yet).”

2 comments:

Mike said...

My favorite way of handling that example is with an anonymous class and an anonymous constructor:

private static Map states = new HashMap()
{
{
put("AK", "Alaska");
put("AL", "Alabama");
// ...
}
};

kdgregory said...

Functionally, that approach is almost the same as a static initializer on the containing class; the one difference is that you may be able to set a breakpoint inside the map's instance initializer. A better approach, IMO, is to use a MapBuilder such as the one in KDGCommons (http://kdgcommons.sourceforge.net/apidocs/index.html).

Any of these approaches work for a simple, literally-constructed static object. However, the bigger issue here is constructing a static object that isn't literally-constructed, but instead needs to pull in large chunks of your application just to be initialized.