5
\$\begingroup\$

After a somewhat lengthily discussion in the chat room, I have set up an IoC container with Unity to store my settings in.

This is my static IoC class:

public static class IoCContainer
{
    public static IUnityContainer Container = new UnityContainer();
}

In App.xaml.cs:

IoCContainer.Container.RegisterType<ApplicationSettingProvider<Theme>, ApplicationThemeProvider>(new ContainerControlledLifetimeManager());
IoCContainer.Container.RegisterType<ApplicationSettingProvider<FontSize>, ApplicationFontSizeProvider>(new ContainerControlledLifetimeManager());
IoCContainer.Container.RegisterType<ApplicationSettingProvider<AppVersion>, ApplicationStartupVersionProvider>(new ContainerControlledLifetimeManager());

Wherever I want to access my settings:

private ApplicationSettingProvider<Theme> _ThemeProvider = IoCContainer.Container.Resolve<ApplicationSettingProvider<Theme>>();
private ApplicationSettingProvider<FontSize> _FontSizeProvider = IoCContainer.Container.Resolve<ApplicationSettingProvider<FontSize>>();
private ApplicationSettingProvider<AppVersion> _StartupVersionProvider = IoCContainer.Container.Resolve<ApplicationSettingProvider<AppVersion>>();

And, an example setting, just to make everything complete:

public class ApplicationThemeProvider : ApplicationSettingProvider<Theme>
{
    public ApplicationThemeProvider()
    {
        ApplicationData.Current.DataChanged += (a, o) =>
        {
            CoreApplication.MainView.CoreWindow.Dispatcher.RunAsync(CoreDispatcherPriority.Normal, () =>
            {
                OnSettingChanged((Theme)ApplicationData.Current.RoamingSettings.Values["Theme"]);
            });
        };
    }

    public override Theme GetCurrentSetting()
    {
        object value;
        if (ApplicationData.Current.RoamingSettings.Values.TryGetValue("Theme", out value))
        {
            int themeValue = (int)value;
            return (themeValue >= 0 && themeValue <= 2) ? (Theme)themeValue : Theme.Default;
        }
        return Theme.Default;
    }

    public override void SetCurrentSetting(Theme theme)
    {
        ApplicationData.Current.RoamingSettings.Values["Theme"] = (int)theme;
        ApplicationData.Current.SignalDataChanged();
    }
}

Which is based on:

public class SettingChangedEventArgs<T> : EventArgs
{
    public T NewSetting { get; private set; }

    public SettingChangedEventArgs(T newSetting)
    {
        NewSetting = newSetting;
    }
}

public abstract class ApplicationSettingProvider<T>
{
    public abstract T GetCurrentSetting();
    public abstract void SetCurrentSetting(T setting);

    public event EventHandler<SettingChangedEventArgs<T>> SettingChanged;
    public void OnSettingChanged(T newSetting)
    {
        var handler = SettingChanged;
        if (handler != null)
        {
            handler(this, new SettingChangedEventArgs<T>(newSetting));
        }
    }
}

This review is primarily to make sure I am using Unity right. Please tell me everything that can be improved.

\$\endgroup\$

1 Answer 1

4
\$\begingroup\$
public static class IoCContainer
{
    public static IUnityContainer Container = new UnityContainer();
}

I've never used Unity, but this looks quite dangerous to me: your IoC container is accessible from anywhere in the code. Dependency Injection does not play well with anything public static - the IoC container is no exception here. In fact, the IoC container instance belongs in one, single place: your composition root - and that's app.xaml.xs. So instead of this:

IoCContainer.Container.RegisterType<ApplicationSettingProvider<Theme>, ApplicationThemeProvider>(new ContainerControlledLifetimeManager());
IoCContainer.Container.RegisterType<ApplicationSettingProvider<FontSize>, ApplicationFontSizeProvider>(new ContainerControlledLifetimeManager());
IoCContainer.Container.RegisterType<ApplicationSettingProvider<AppVersion>, ApplicationStartupVersionProvider>(new ContainerControlledLifetimeManager());

You should have that:

_container.RegisterType<ApplicationSettingProvider<Theme>, ApplicationThemeProvider>(new ContainerControlledLifetimeManager());
_container.RegisterType<ApplicationSettingProvider<FontSize>, ApplicationFontSizeProvider>(new ContainerControlledLifetimeManager());
_container.RegisterType<ApplicationSettingProvider<AppVersion>, ApplicationStartupVersionProvider>(new ContainerControlledLifetimeManager());

Where _container is an instance field of your App class. In a application this setup code would be in the OnStartup override: the UnityContainer instance only ever lives at the composition root level.

Speaking of composition, you've either not included it, or all you're doing is the registration part. And this explains it:

Wherever I want to access my settings:

private ApplicationSettingProvider<Theme> _ThemeProvider = IoCContainer.Container.Resolve<ApplicationSettingProvider<Theme>>();
private ApplicationSettingProvider<FontSize> _FontSizeProvider = IoCContainer.Container.Resolve<ApplicationSettingProvider<FontSize>>();
private ApplicationSettingProvider<AppVersion> _StartupVersionProvider = IoCContainer.Container.Resolve<ApplicationSettingProvider<AppVersion>>();

That's exactly the opposite of : everywhere you're accessing your settings, you have a tight coupling with that static class IoCContainer and its public static IUnityContainer field.

That's the Service Locator anti-pattern: anyone, anywhere, has access to anything that's registered with the IoC container. The typical Service Locator passes the container around as a dependency - but your is worse, because being a public static utility class, it becomes an ambient context that's accessible at any given time by anyone that wants to use it.

IoC containers are much, much more powerful than that. IoC stands for Inversion of Control - that means your types don't ask the container to resolve an instance of a service, but rather they tell it what their dependencies are, and the IoC container injects them... usually through the constructor.


The Hollywood Principle: Don't Call Them, They'll Call You

I'm going to have to make some things up here, because you've supplied us with the dependencies, but not with anything that depends on them.

So let's say your application is controller by a MainWindowViewModel.

This MainWindowViewModel class needs to access the settings; its constructor will statically document this fact, by taking in an interface that represents some SettingsProvider - here you've done that with an abstract class:

private readonly ApplicationSettingProvider<Theme> _themeSettings;
private readonly ApplicationSettingProvider<FontSize> _fontSizeSettings;
private readonly ApplicationSettingProvider<AppVersion> _appVersionSettings;

private readonly IView _view;

public MainWindowViewModel(ApplicationSettingProvider<Theme> themeSettings,
                           ApplicationSettingProvider<FontSize> fontSizeSettings,
                           ApplicationSettingProvider<AppVersion> appVersionSettings,
                           IView view)
{
    _themeSettings = themeSettings;
    _fontSizeSettings = fontSizeSettings;
    _appVersionSettings = appVersionSettings;

    _view = view;
}

The class may have more dependencies - the key is that it receives them all into its constructor.

Constructor Bloating

Everyone has their threshold. For most people, 3-4 constructor parameters is the upper limit, 5 in the extreme cases. When you start seeing that your constructor has too many dependencies, it's a sign that your class is responsible for too many things - you should then extract a class, wrap it with an interface, and inject that interface instead. In this case there could be a single IAppSettings abstraction injected there, and the implementation for that interface would receive the 3 SettingsProvider<T> into its own constructor.

But let's say these are the only 3 dependencies you have to deal with for now... plus the IView.

When you register the types with your IoC container, you're telling it what types to use to resolve the dependencies when you compose the application - that's the step you missed.

Back to the Composition Root

_container.RegisterType<ApplicationSettingProvider<Theme>, ApplicationThemeProvider>(new ContainerControlledLifetimeManager());
_container.RegisterType<ApplicationSettingProvider<FontSize>, ApplicationFontSizeProvider>(new ContainerControlledLifetimeManager());
_container.RegisterType<ApplicationSettingProvider<AppVersion>, ApplicationStartupVersionProvider>(new ContainerControlledLifetimeManager());

// again, I don't know how Unity works - this is just for the concept:
_container.RegisterType<MainWindowViewModel,MainWindowViewModel>(new ContainerControlledLifetimeManager());
_container.RegisterType<IView,MainWindow>(new ContainerControlledLifetimeManager());

Now the IoC container knows about MainWindowViewModel, and all of its dependencies. The last thing to do is to resolve them, and get your app started:

var viewModel = _container.Resolve<MainWindowViewModel>();
viewModel.Show();

What just happened here? The container used the configurations you gave it, and constructor-injected all the dependencies of the viewModel - that's why that method is called Resolve: it resolves the entire application's depedency graph with a single call.

The only place you're allowed to do this is in the composition root!

That's why I said in chat, that you cannot do Dependency Injection halfway: you need to go DI all the way, and register all dependencies of every single type; every type needs to have its dependencies constructor-injected, otherwise you're breaking the pattern and you're going to have to new things up manually and thus introduce tight coupling.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.