Friday, October 09, 2015

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.

submit to reddit

Wednesday, October 07, 2015

MEF Hacks: Constructors with parameters

Often, there’s a need to register an object into your dependency container that takes constructor arguments. There are a few ways to accomplish this with MEF.

You’re mileage will vary, but my favorite approach is a bit of a hack that leverages a lesser-known feature of MEF: Property Exports.

public class ComplexObjectExportFactory
{
    [Export]
    public MyComplexObject ComplexObject
    {
        get
        {
            return new MyComplexObject("config.xml");
        }
    }
}

This "simply works" and doesn't require you to do anything intrusive like sub-classing a dependency or registering named parameters on application start-up. The technique is especially useful if you need to provide any kind of conditional logic based on settings and it can even build upon other MEF registered parts, for example:

internal class ComplexObjectExportFactory
{
    private readonly IApplicationSettingsProvider _settings;
    private readonly IEventAggregator _eventAggregator;

    [ImportingConstructor]
    public ComplexObjectExportFactory(
            IApplicationSettingsProvider settings, 
            IEventAggregator eventAggregator)
    {
        _settings = settings;
        _eventAggregatore = eventAggregator;
    }

    [Export]
    public MyComplexObject ComplexObject
    {
        get
        {
            string configFile = _settings.GetSetting("ComplexObjectConfigFile");
            int timeout = _settings.GetSetting<int>("ComplexObjectTimeoutSeconds");
            
            return new MyComplexObject(_eventAggregator, configFile, timeout);
        }
    }
}

The only real down-fall to this overall approach is that the ComplexObjectExportFactory will never have any usages, so tools like Resharper will think this is dead-code, but this can be solved with a few well placed #pragmas and some comments. The other reason this feels like a hack, and this is more of an esthetics or stylistic difference, is that this feels like it should be a method (eg, factory.Create()) but MEF treats exported methods very differently.

Despite the awkwardness of this class, I’d prefer this to sub-classing third-party dependencies or bloating the bootstrapper with additional initialization logic. Maybe you think so too?

Happy coding.

submit to reddit

Monday, October 05, 2015

Application Services for WPF Applications

Suppose you’re building a WPF application and have a number of actions that you want to perform on application start-up. Here’s a pattern that I’ve used successfully for the last few years that works with Caliburn.Micro or other MVVM frameworks…

If you're using Caliburn.Micro, the framework provides a Bootstrapper as an entry point to your application and the overridable Configure method is a great place to initialize your application-specific services.

You might be tempted to do something like this:

protected override void Configure()
{
    Container = new CompositionContainer(
       new AggregateCatalog(AssemblySource.Instance.Select(x => new AssemblyCatalog(x)).OfType<ComposablePartCatalog>()));

    var batch = new CompositionBatch();

    // initialize Caliburn.Micro services and register with container
    EventAggregator = new EventAggregator();

    batch.AddExportedValue<IWindowManager>(new WindowManager());
    batch.AddExportedValue<IEventAggregator>(EventAggregator);
    batch.AddExportedValue(Container);
    Container.Compose(batch);

    // initialize and register all my custom components
    var myService = new MyService();
    myService.Configure("config.xml");
    // repeat for all other services...

}

While this strategy works for one or two services, you'll find that as you add additional services your Bootstrapper logic becomes quite bloated and simply owns too many responsibilities.

To simplify things, let's define a simple interface that describes all of our application services:

public interface IApplicationService : IDispose
{
    void Initialize();
}

And now we can generically initialize all services that implement this interface:

protected void Configure()
{
   // snip...
   Container.Compose(batch);

   InitializeApplicationServices();
}

protected void InitializeApplicationServices()
{
   var services = Container.GetExports<IApplicationService>();
   foreach (Lazy<IApplicationService> exportedService in services)
   {
       IApplicationService service = exportedService.Value;
       Log.InfoFormat("Initializing IApplicationService:{0}.", service.GetType().Name);
       service.Initialize();
   }
}

protected override OnExit(object sender, EventArgs e)
{
    Container.Dispose();

    base.OnExit(sender, e);
}

[Export(typeof(IApplicationService))]
public class MyApplicationService : IApplicationService
{
    private MyService _service;        

    public void Initialize()
    {
       _service = new MyService();
       _service.Configure("config.xml"); 
    }

    public void Dispose()
    {
       GC.SuppressFinialize(this);
       Dispose(true);
    }
    protected virtual void Dispose(bool disposing)
    {
       _service.Dispose();       
    }
}

There! That’s a lot cleaner. Now the Bootstrapper doesn’t know anything about our services and we can add new services without having to further extend this class. There are a few additional benefits:

  1. If you’re using MEF, all classes that are export IApplicationService are singletons by default.
  2. All services have a predefined start (Initialize) and end (Dispose), thereby avoiding work that is often put in the constructor.
  3. Perhaps not obvious, but if you’re using MEF, calling Container.Dispose() when the application exits automatically calls Dispose on all our application services, too.

So there you have a basic initialization routine for all your background services. My next few posts will demonstrate how to really take advantage of this pattern.

P.S: Astute readers of my blog might recognize that I'm using an Initialize() method which I've called out as a bad habit on my On Notice Board. While “Init methods” is high on my list, the context here is different. In this context, the only method I know about is Initialize(), whereas the “Init method” smell is a required method that must be called before calling other methods on the same class. I’ll blog about how to remove Init methods later.

submit to reddit