Init Methods are Crap

This title of this post pretty much sums up how I feel about Init methods. Let’s take a look at an example why.

I’ve been on a soap-box yelling at anyone who’ll listen that “work in the constructor” is perhaps the worst thing you can do as a developer. You’ve drank the cool-aid and you have some fairly important initialization logic that needs to be called, so you move the logic out of the constructor and into a separate method. Your code now looks like this:

public class MenuProvider : IMenuProvider
{
    private Array<MenuItem> _menuItems;
    
    public void Initialize()
    {
        // do work to populate menu items, serialize from file, etc...
        _menuItems = // some value;
    }

    public IEnumerable<MenuItem> MenuItems
    {
        get
        {
            return _menuItems;
        }
    }

    public IEnumerable<MenuItem> InActiveMenuItems
    {
        get
        {
	    return _menuItems.Where(i => i.Active == false);
        }
    }

}

It's a contrived example, but in the above code MenuItems will be null and InActiveMenuItems throws an error unless you call Initialize() first. Certainly, there must be a better design than this? (If you answered put the work in the constructor, please come see me after class, I.. uh,.. have something for you.)

Putting the onus on the developer to call methods in a certain order is a very backward approach. You, the class designer, should make it difficult for people to do things the wrong way. We can take on some of that onerous effort inside the class, something like this should work nicely:

public class MenuProvider : IMenuProvider
{
   private bool _isInitialized;
   private object _lock = new object();
   private Array<MenuItem> _menuItems;

   public IEnumerable<MenuItem> GetMenuItems()
   {
        EnsureInitialized();
    
        return _menuItems;
   }

   public IEnumerable<MenuItem> GetInactiveMenuItems()
   {
        EnsureInitialized();

        return _menuItems.Where(i => i.Active == false);
   }

   private void EnsureInitialized()
   {
        if (!_initialized)
        {
            Initialize();
        }
   }
   
   private void Initialize()
   {
        lock(_lock)
        {
            if (!_initialized)
            {
                // do initialization work
                _initialized = true;
            }
        }
   }
}

In this design, I've made a few minor adjustments. For stylistic purposes, I converted the properties into methods because properties look awkward on interfaces. Properties also aren't a good choice if there's work involved in calculating them, so methods feel better here.

Outside of the cosmetic changes, one of the most notable changes is that my Initialize method is now private. I've also added some plumbing logic to ensure that the Initialize method only does work the first time it's called, and since this is good defensive programming, this might have already existed and shouldn't be a surprise.

What should stand out to you, is that each method is responsible for ensuring that the initialization logic is called on the user's behalf using the EnsureInitialized method before doing any other work. Some developers may grumble that this seems like a simple thing to forget when adding new methods and thus has potential for silly bugs. That’s fair, but because the API for the class has been simplified the tests to catch this should be much easier to write and find.

Alternatively, we can take this approach further by splitting the concerns into smaller parts. One part could read the file contents and the other part could be responsible for serving the data to consumers. This is effectively just moving the Initialize method to another class. Something like this:

public class MenuProvider : IMenuProvider
{
    private MenuReader _reader;

    public MenuProvider(MenuReader reader)
    {
        _reader = reader;
    }

    public IEnumerable<MenuItem> GetMenuItems()
    {
        return _reader.GetAll();
    }

    public IEnumerable<MenuItem> GetInactiveMenuItems()
    {
        return _reader.GetAll().Where(i => i.Active == false);
    }
}

public abstract class MenuReader
{
    public abstract IEnumerable<MenuItem> GetAll();
}

public class StaticFileMenuReader : MenuReader
{
    private Array<MenuItem> _items;

    public IEnumerable<MenuItem> GetAll()
    {
        if (_items != null)
        {
            // do serialization work
        }
        return _items;
    }
}

This small change greatly simplifies the MenuProvider and the extra step to move the serialization logic into another class may seem familiar to others as a Repository pattern. The repository pattern in this example also affords us additional test flexibility as the file-system can be abstracted away with a test-double.

To sum up, it seems a bit odd to design a class that requires methods to be called in a specific order. Initialization in general is something that the class designer should be well equipped to handle, so don’t push this responsibility to others – a few small changes to your class makes your code more usable, responsive and easier to test.