I’ve been using static methods and singletons blindly for so long that I’ve never paid much thought to how smelly they were until I was enlightened by The Productive Programmer recently. Static methods, as we all know, have one wonderful use: as a black-box, standalone, stateless method. The use of static methods of the Math class in Java illustrates good use. When you call the Math.sqrt() method, you don’t worry that a subsequent call might return the cube root instead of the square root because some state within the sqrt() method changed. Static methods work well when they are completely stateless. You get into trouble when you start mixing staticness and state.

The evil code smell emitted by the singleton design pattern arises from the combination of “static” and “state”. The goal of a singleton is to create a class that can be instantiated only once and all subsequent attempts to create an instance of the class return the original instance. In most code I’ve seen, as well as ALL the code I’ve written, the singleton is typically implemented like this:

public class ConfigSingleton
{
    private static ConfigSingleton myInstance;
    private Point _initialPosition;

    public Point getInitialPosition()
	{
        return _initialPosition;
    }

    private ConfigSingleton()
	{
        Dimension screenSize =
                Toolkit.getDefaultToolkit().getScreenSize();
        _initialPosition = new Point();
        _initialPosition.x = (int) screenSize.getWidth() / 2;
        _initialPosition.y = (int) screenSize.getHeight() / 2;
    }

    public static ConfigSingleton getInstance()
	{
        if (myInstance == null)
            myInstance = new ConfigSingleton();
        return myInstance;
    }
}

In the code above, the getInstance() method checks to see if one already exists, creates the sole instance if needed, then returns the reference to it. Note that this method isn’t thread safe, but this post isn’t about thread safety and it just adds more complexity. The thing that’s so evil about the singleton is the embedded state, which makes it untestable. Unit testing manipulates state, but there is no way to manipulate the state of this singleton object. Because construction is atomic in Java, there is no way you can test this class with any value of initialPosition rather than the constructed value derived from the current screen size. A singleton is the object-oriented version of a global variable, and everyone knows that global variables are bad. Of course, sad to say, this is how I’ve been writing singletons all my life.

Ultimately, what makes singleton so smelly is the fact that you have a single class that has two distinct responsibilities: policing the instances of itself and providing configuration information. Any time you have a single class with multiple unrelated responsibilities, you have a code smell. But it’s always useful to have only a single configuration object, so how can you achieve that without using a singleton? The recommendation from the Productive Programmer is that you can use a plain object plus a factory, delegating the individual responsibilities to each where the factory is responsible for policing the instance and the POJO deals only with configuration information and behavior.

Here is the updated configuration object as a POJO:

public class Configuration
{
    private Point _initialPosition;

    private Configuration(Dimension screenSize)
	{
        _initialPosition = new Point();
        _initialPosition.x = (int) screenSize.getWidth() / 2;
        _initialPosition.y = (int) screenSize.getHeight() / 2;
    }
    public int getInitialX()
	{
        return _initialPosition.x;
    }

    public int getInitialY()
	{
        return _initialPosition.y;
    }
}

This class is trivial to test. Both the unit test and factory will use reflection to create the class. In Java, private access is little more than documentation indicating suggested usage. In modern languages, you can always bypass it with reflection if the need arises. This represents a good case where you don’t expect anyone to instantiate one of these classes; therefore, the constructor is private. The following code shows the unit tests for this class, including the instantiation via reflection and accessing the private field via reflection to test the class’s behavior with different values:

public class TestConfiguration
{
      Configuration c;

      @Before
	  public void setUp()
	  {
          try
		  {
              Constructor cxtor[] =
                      Configuration.class.getDeclaredConstructors();
              cxtor[0].setAccessible(true);
              c = (Configuration) cxtor[0].newInstance(
                      Toolkit.getDefaultToolkit().getScreenSize());
          }
		  catch (Throwable e)
		  {
              fail();
          }
      }

      @Test
      public void initial_position_set_correctly_upon_instantiation()
	  {
          Configuration specialConfig = null;
          Dimension screenSize = null;
          try
		  {
              Constructor cxtor[] =
                      Configuration.class.getDeclaredConstructors();
              cxtor[0].setAccessible(true);
              screenSize = new Dimension(26, 26);
              specialConfig = (Configuration) cxtor[0].newInstance(screenSize);
          }
		  catch (Throwable e)
		  {
              fail();
          }
          Point expected = new Point();
          expected.x = (int) screenSize.getWidth() / 2;
          expected.y = (int) screenSize.getHeight() / 2;
          assertEquals(expected.x, specialConfig.getInitialX());
          assertEquals(expected.y, specialConfig.getInitialY());
      }

      @Test
      public void initial_postion_can_be_changed_after_instantiation()
	  {
          Field f = null;
          try
		  {
              f = Configuration.class.getDeclaredField("_initialPosition");
              f.setAccessible(true);
              f.set(c, new Point(10, 10));
          }
		  catch (Throwable t)
		  {
              fail();
          }
          Assert.assertEquals(10, c.getInitialX());
      }
}

The setUp() method creates the Configuration object via reflection and calls initialize() to create a valid object for most of the tests. You can access the private field _initialPosition using reflection to see what would happen in your configuration class if the initial position is something other than the default. Making the Configuration class a plain object ensures that it is easy to test and doesn’t compromise any of the functionality it had before. The factory responsible for creating configuration is also simple and testable; the code for ConfigurationFactory appears here:

public class ConfigurationFactory
{
    private static Configuration myConfig;

    public static Configuration getConfiguration()
	{
        if (myConfig == null)
		{
            try
			{
                Constructor cxtor[] =
                    Configuration.class.getDeclaredConstructors();
                cxtor[0].setAccessible(true);
                myConfig = (Configuration) cxtor[0].newInstance(
                        Toolkit.getDefaultToolkit().getScreenSize());
            }
			catch (Throwable e)
			{
                throw new RuntimeException("can't construct Configuration");
            }
        }
        return myConfig;
    }
}

Not surprisingly, this code looks just like the creation code from the original singleton. The important difference is that this code does only one thing: police the instances of the
Configuration class. The ConfigurationFactory is also very testable, as shown here:

public class TestConfigurationFactory
{
      @Test
      public void creation_creates_a_single_instance()
	  {
          Configuration config1 = ConfigurationFactory.getConfiguration();
          assertNotNull(config1);
          Configuration config2 = ConfigurationFactory.getConfiguration();
          assertNotNull(config2);
          assertSame(config1, config2);
      }
  }

Not only does it just feel right, it looks so much cleaner doesn’t it? I’m definitely going to refactor my singletons the next time I step into the office!