Taking baby-steps in applying a better design

Question

In wanting to get some hands-on experience of good OO design I've decided to try to apply separation of concerns on a legacy app.

I decided that I wasn't comfortable with these calls being scattered all over the code base.

ConfigurationManager.AppSettings["key"]

While I've already tackled this before by writing a helper class to encapsulate those calls into static methods I thought it could be an opportunity to go a bit further.

I realise that ultimately I should be aiming to use dependency injection and always be 'coding to interfaces'. But I don't want to take what seems like too big a step. In the meantime I'd like to take smaller steps towards that ultimate goal.

Can anyone enumerate the steps they would recommend?

Here are some that come to mind:

• Have client code depend on an interface not a concrete implementation

• Manually inject dependencies into an interface via constructor or property?

• Before going to the effort of choosing and applying an IoC container how do I keep the code running?

• In order to fulfil a dependency the default constructor of any class that needs a configuration value could use a Factory (with a static CreateObject() method)?

Surely I'll still have a concrete dependency on the Factory?...

I've dipped into Michael Feathers' book so I know that I need to introduce seams but I'm struggling to know when I've introduced enough or too many!

Update

Imagine that Client calls methods on WidgetLoader passing it the required dependencies (such as an IConfigReader)

WidgetLoader reads config to find out what Widgets to load and asks WidgetFactory to create each in turn

WidgetFactory reads config to know what state to put the Widgets into by default

WidgetFactory delegates to WidgetRepository to do the data access, which reads config to decide what diagnostics it should log

In each case above should the IConfigReader be passed like a hot potato between each member in the call chain?

Is a Factory the answer?

To clarify following some comments:

My primary aim is to gradually migrate some app settings out of the config file and into some other form of persistence. While I realise that with an injected dependency I can Extract and Override to get some unit testing goodness, my primary concern is not testing so much as to encapsulate enough to begin being ignorant of where the settings actually get persisted.

Answer 1

Usually its very difficult to clean a legacy application is small steps, because they are not designed to be changed in this way. If the code is completely intermingled and you have no SoC it is difficult to change on thing without being forced to change everything else... Also it is often very hard to unit test anything.

But in general you have to: 1) Find the simplest (smallest) class not refactored yet 2) Write unit tests for this class so that you have confidence that your refactoring didn't break anything 3) Do the smallest possible change (this depends on the project and your common sense) 4) Make sure all the tests pass 5) Commit and goto 1

I would like to recommend "Refactoring" by Martin Fowler to give you more ideas: http://www.amazon.com/exec/obidos/ASIN/0201485672

Answer 2

When refactoring a legacy code-base you want to iteratively make small changes over time. Here is one approach:

• Create a new static class (i.e. MyConfigManager) with a method to get the app setting (i.e. GetAppSettingString( string key )

• Do a global search and replace of "ConfigurationManager.AppSettings["key"] and replace instances with "MyConfigManager.GetAppSettingsString("key")"

• Test and check-in

Now your dependency on the ConfigurationManager is in one place. You can store your settings in a database or wherever, without having to change tons of code. Down side is that you still have a static dependency.

Next step would be to change MyConfigManager into a regular instance class and inject it into classes where it is used. Best approach here is to do it incrementally.

• Create an instance class (and an interface) alongside the static class.

• Now that you have both, you can refactor the using classes slowly until they are all using the instance class. Inject the instance into the constructor (using the interface). Don't try for the big bang check-in if there are lots of usages. Just do it slowly and carefully over time.

• Then just delete the static class.

Answer 3

For your example, the first thing I'd do is to create an interface exposing the functionality you need to read config e.g.

public interface IConfigReader
{
    string GetAppSetting(string key);
    ...
}

and then create an implementation which delegates to the static ConfigurationManager class:

public class StaticConfigReader : IConfigReader
{
    public string Get(string key)
    {
     return ConfigurationManager.AppSetting[key];
    }
}

Then for a particular class with a dependency on the configuration you can create a seam which initially just returns an instance of the static config reader:

public class ClassRequiringConfig
{
    public void MethodUsingConfig()
    {
     string setting = this.GetConfigReader().GetAppSetting("key");
    }
    protected virtual IConfigReader GetConfigReader()
    {
     return new StaticConfigReader();
    }
}

And replace all references to ConfigManager with usages of your interface. Then for testing purposes you can subclass this class and override the GetConfigReader method to inject fakes so you don't need any actual config file:

public class TestClassRequiringConfig : ClassRequiringConfig
{
    public IConfigReader ConfigReader { get; set; }
    protected override IConfigReader GetConfigReader()
    {
     return this.ConfigReader;
    }
}

[Test]
public void TestMethodUsingConfig()
{
    ClassRequiringConfig sut = new TestClassRequiringConfig { ConfigReader = fakeConfigReader };
    sut.MethodUsingConfig();

    //Assertions
}

Then eventually you will be able to replace this with property/constructor injection when you add an IoC container.

EDIT: If you're not happy with injecting instances into individual classes like this (which would be quite tedious if many classes depend on configuration) then you could create a static configuration class, and then allow temporary changes to the config reader for testing:

    public static class Configuration
{
    private static Func<IConfigReader> _configReaderFunc = () => new StaticConfigReader;

    public static Func<IConfigReader> GetConfiguration
    {
     get { return _configReaderFunc; }
    }

    public static IDisposable CreateConfigScope(IConfigReader reader)
    {
     return new ConfigReaderScope(() => reader);
    }

    private class ConfigReaderScope : IDisposable
    {
     private readonly Func<IConfigReader> _oldReaderFunc;

     public ConfigReaderScope(Func<IConfigReader> newReaderFunc)
     {
      this._oldReaderFunc = _configReaderFunc;
      _configReaderFunc = newReaderFunc;
     }

     public void Dispose()
     {
      _configReaderFunc = this._oldReaderFunc;
     }
    }
}

Then your classes just access the config through the static class:

public void MethodUsingConfig()
{
    string value = Configuration.GetConfigReader().GetAppSetting("key");
}

and your tests can use a fake through a temporary scope:

[Test]
public void TestMethodUsingConfig()
{
    using(var scope = Configuration.CreateConfigScope(fakeReader))
    {
        new ClassUsingConfig().MethodUsingConfig();
        //Assertions
    }
}