Coding Efficiency

SOLID is a combination of five important design principles:
S – Single Responsibility Principle
O – Open Closed Principle
L – Liskov Substitution Principle
I – Interface Segregation Principle
D – Dependency Inversion Principle

This post is about the Dependency Inversion Principle.

Definition:

The Dependency Inversion Principle states that higher level components shouldn’t depend on lower level ones, instead they should depend on abstractions.

Or:
Abstractions shouldn’t depend on details. Details should depend on abstractions.

Benefits:

Unit testing is much easier as you can really test the class you want to test in isolation. If done right, the dependencies of your class under test can simply be faked and therefore, the implementations of it’s dependencies don’t effect your tests.

You can easily create a new implementation, it just has to implement an interface.

Some kind of code duplication can be removed as applying the dependency inversion principle enables you to consolidate mutiple rather equal algorythms into a single generic one.

A bad Example:

Let’s say you work for a company that has three different crms and wants to export some customers from crm 1 to crm 2, crm 1 is capable of exporting its customers as a csv file. We’ll import that.

public class CustomerFileImporter
{
    private string _filePath;

    public CustomerFileImporter(string filePath)
    {
        if (!File.Exists(filePath))
        {
            throw new ArgumentException("File does not exist.");
        }

        _filePath = filePath;
    }

    public void Import()
    {
        var customersFromFile = LoadFromFile();
        var existingCustomers = LoadExistingCustomersFromDb(customersFromFile);

        var updated = HandleCustomerUpdate(existingCustomers, customersFromFile);
        var created = HandleCustomerCreation(existingCustomers, customersFromFile);

        WriteChangesToDb(updated, created);
    }

    private void WriteChangesToDb(IList<Customer> updated,
                                  IList<Customer> created)
    {
        // ....
    }

    private IList<Customer> HandleCustomerCreation(IList<Customer> existing,
                                                   IList<Customer> imported)
    {
        // .....
    }

    private IList<Customer> HandleCustomerUpdate(IList<Customer> existing,
                                                 IList<Customer> imported)
    {
        // .....
    }

    private IList<Customer> LoadFromFile()
    {
        // import the file and
        // return the customers
    }

    private IList<Customer> LoadExistingCustomersFromDb(IList<Customer> imported)
    {
        // do some fancy db stuff
        // and return the customers
    }
}

What’s the problem?

Well, right now, nothing. It does exactly what it has to do. But as we all know “Change is the only constant”, it will only be a matter of time until the requirements change because “The import worked so great!”.

And now, a change is requested, crm 3 should be a possible source too. Sadly, crm 3 doesn’t provide any way to export the customers as a csv file.

How to fix it?

The lazy approach would be to simply copy and paste the CustomerFileImporter, rename it CustomerDbImporter and do the necessary changes.

The better approach would be to rely more on abstractions. Just create an interface, let’s say ICustomerImportSource, with the method LoadCustomers() that returns an IList<Customer>. All we have to change in the above code is to accept an source as constructor argument instead of a filepath and change the call from LoadFromFile to source.LoadCustomers().

All the logic that was in the LoadFromFile method is now in the LoadCustomers method of the ICustomerImportSource’s implementation that might be called Crm1CustomerImportSource. The now requested change is just another implementation of ICustomerImportSource.

As the import is no longer restricted to files, we should rename the CustomerFileImporter to just CustomerImporter.

More changes

I said Change is the only constant right? Well, the next request is “just make it possible to import from any crm into any other”.

As you might have expected, just let the CustomerImporter take an ICustomerImportTarget too, implement the necessary logic for all three crms there, implement the ICustomerImportSource for crm 2 and be done.

The result

public class CustomerImporter
{
    private ICustomerImportSource _source;
    private ICustomerImportTarget _target;

    public CustomerImporter(ICustomerImportSource source,
                            ICustomerImportTarget target)
    {
        _source = source;
        _target = target;
    }

    public void Import()
    {
        var customersFromSource = _source.Load();
        var existingCustomers = _target.LoadExisting(customersFromSource);

        var updated = HandleCustomerUpdate(existingCustomers, customersFromSource);
        var created = HandleCustomerCreation(existingCustomers, customersFromSource);

        _target.HandleChanges(updated, created);
    }

    private IList<Customer> HandleCustomerCreation(IList<Customer> existing,
                                                   IList<Customer> imported)
    {
        // .....
    }

    private IList<Customer> HandleCustomerUpdate(IList<Customer> existing,
                                                 IList<Customer> imported)
    {
        // .....
    }
}

public interface ICustomerImportSource
{
    IList<Customer> Load();
}

public interface ICustomerImportTarget
{
    IList<Customer> LoadExisting(IList<Customer> imported);
    void HandleChanges(IList<Customer> updated, IList<Customer> created);
}

Conclusion

In most cases, only very little effort is required to convert a static solution that depends on concrete implementations into something more generic that needs only abstractions to fulfill its duty. The dependency inversion principle makes it possible.

Would I do such a thing upfront? Well, in most cases yes, its almost zero effort and clearly separates the responsibilities of each class.

Actually, I’d go one step further and almost always use interfaces. The reason is simple, easier unit testing with very little effort and if you use an IoC container there is no pain with the instantiation and the like. More on that in a later post.

SOLID is a combination of five important design principles:
S – Single Responsibility Principle
O – Open Closed Principle
L – Liskov Substitution Principle
I – Interface Segregation Principle
D – Dependency Inversion Principle

This post is about the Interface Segregation Principle.

Definition:

The Interface Segregation Principle states that client’s shouldn’t depend on methods that they do not use. In other words, a class should only depend on the smallest possible interface of other classes.

Bad Example:

Let’s say you have an IEntityReporter in your application, while you created it, the only required report was a revenue report for customers, yet you wanted it to be generic and called the method therefore CreateReportFrom.

public interface IEntityReporter<T> where T : Entity
{
    IReport CreateReportFrom(T entity);
}

Now you are asked to create revenue reports from products too, as it’s no problem, you just create a ProductReporter that implements the IEntityReporter.

Later, you are asked to create two more reports, an recommendation report for customers and an inventory report for products . The former shows what the particular customer might be interested in, the latter contains stock data of the product.

You might implement it this way:

public interface IEntityReporter<T> where T : Entity
{
    IReport CreateReportFrom(T entity);
    IReport CreateRecommendationReportFrom(T entity);
    IReport CreateInventoryReportFrom(T entity);
}

(It would be way worse if you want to add methods like CreateReport2, CreateReport3 etc.)

It will work but you can’t create an inventory report for customers, so you’ll have to throw a NotImplementedException / NotSupportedException.

As there are just two entities involved and thereby only two implementations of the interface, you might think that it’s not that bad as there is almost no effort required to deal with these additional methods.

Although that is true, what happens if you have to add more reports for other entities like Location, Supplier etc? Do you want to add all those reports to the EntityReporter interface as well?

What do you get if you call CreateReport in the LocationReporter? A revenue report about all customers contained within it? A report about all customers / suppliers in that location?

Even if you create an abstract EntityReporter that implements the interface with empty, virtual methods, do you really want all the IntelliSense clutter if you deal with an EntityReporter, how do you tell other developers what reports are valid for each entity?

Another argument might be that you have to redeploy each and every assembly that contains an IEntityReporter implementation if you change the interface.

How it could be done

I’d say it would be better to get rid of your IEntityReporter and create specific interfaces:

public interface IRevenueReporter<T> where T : Entity
{
    IReport CreateRevenueReportFrom(T entity);
}

public interface ICustomerReporter : IRevenueReporter<Customer>
{
    IReport CreateRecommendationReportFrom(Customer customer);
}

public interface IProductReporter : IRevenueReporter<Product>
{
    IReport CreateInventoryReportFrom(Product product);
}

SOLID is a combination of five important design principles:
S – Single Responsibility Principle
O – Open Closed Principle
L – Liskov Substitution Principle
I – Interface Segregation Principle
D – Dependency Inversion Principle

Definition:

If for each object o1 of type S there is an object o2 of type T such that for all programs P defined in terms of T, the behaviour of P is unchanged when o1 is substituted for o2 then S is a subtype of T.

In other words, if your method accepts an animal, it shouldn’t care whether it’s a cat, a dog or whatever.

Further, the Liskov Substitution Principle states that by inheriting you shouldn’t weaken the post conditions nor should you strengthen the pre conditions and you also shouldn’t change expected behavior.

Bad Example #1

If you have a hirachy of animals, like Animal -> Cat | Dog | Horse and Animal provides the method void Feed(Food food), you’d actually expect the animal to be fed afterwards, but neither cats nor dogs eat grass (well, you know what I mean ) and horses don’t eat meat.

public abstract class Food { }
public class Grass : Food { }
public class Meat : Food { }

public abstract class Animal
{
    public abstract void Feed(Food food);
    public bool IsFed { get; protected set; }
}

public class Dog : Animal
{
    public override void Feed(Food food)
    {
        if(food is Meat)
        {
            IsFed = true;
        }
    }
}

public class Cat : Animal
{
    public override void Feed(Food food)
    {
        if(food is Meat)
        {
            IsFed = true;
        }
    }
}

public class Horse : Animal
{
    public override void Feed(Food food)
    {
        if(food is Grass)
        {
            IsFed = true;
        }
    }
}

As you can see, Meat and Grass both inherit from Food, yet dog, cat and horse make a difference between the types of food.

If we have a method that relies on the fact that an animal is fed after feeding it and feed grass to a cat, the method will break.

This is also a violation of the OCP, if you’d add dried cat food, the cat class would have to be changed in order to accept it too.

Bad Example #2

This is another example of altering the behavior, it should be a little more familiar.

If you implement the IList interface, everyone will expect that calling the Add method increases Count and that the added object can actually be found within the list.

public class MyList<T> : IList<T>
{
    private readonly List<T> _innerList = new List<T>();

    public void Add(T item)
    {
        if (_innerList.Contains(item))
        {
            return;
        }
        _innerList.Add(item);
    }

    public int Count
    {
        get { return _innerList.Count}
    }
    // . . .
}

Although the interface is implemented correctly, it compiles, it’s actually wrong as it’s not following the expected behavior. If you’d rename the class to UniqueList, it might be a different case – at least as long as you deal with this specific implementation. The “correct” way would be to create a new interface, like ISet.

A last word

Although the Liskov Substitution Principle is very important, there are a few cases where a violation, or at least bending is ok. Take my message handler example from the last post, the LSP is bended as it’d throw an exception if it would have to handle a message type where no handler exists. I’d say that is ok, I don’t want to handle unknown messages and throw an exception instead.

SOLID is a combination of five important design principles:
S – Single Responsibility Principle
O – Open Closed Principle
L – Liskov Substitution Principle
I – Interface Segregation Principle
D – Dependency Inversion Principle

This post is about the Open Closed Principle.

Definition

The open closed principle states that classes should be open for extension but closed for modification.
You shouldn’t touch written code, you should instead add new. Whether it’s done by sub classing and overriding or otherwise.

What are the benefits?

Stability – If you don’t change existing code, you can’t break it.
Error Reduction – If you always have to make more or less the same changes over and over again you might make a mistake.
Clarity and Readability – Switch statements aren’t that great for readability / clarity.

When should you apply it?

If you see large if and else if blocks or switch statements, you know you could and maybe should refactor to the open closed principle. Using an IoC container makes it child’s play.

A bad Example

Lets say you have a system that deals with messages, therefore, you have different kinds of messages that all inherit from IMessage. Usually you’d have some kind of MessageHandler that has a method like void Handle(IMessage message), without the OCP you’d just make an large switch statement for each message type you’d like to handle.

public interface IMessage
{
    Guid Id { get; }
    DateTime SendingTime { get; set; }
}

public interface IMessageHandler
{
    void Handle(IMessage message);
}

public class MessageHandler : IMessageHandler
{
    public void Handle(IMessage message)
    {
        if (message is ChatMessage)
        {
            HandleChatMessage((ChatMessage)message);
        }
        else if(message is PrivateChatMessage)
        {
            HandlePrivateChatMessage((PrivateChatMessage)message);
        }
        else if(message is UserConnectedMessage)
        {
            HandleUserConnectedMessage((UserConnectedMessage) message);
        }

    }
}

The Handle method might quickly span multiple screens and the MessageHandler itself might easily span many hundreds of lines as contains every handling method.

By the way: this is also a classical violation of the Liskov Substitution Principle, the L in SOLID.

With the help of an IoC container we can easily refactor it and thereby make it follow the OCP and the LSP.

The better Way

public interface IMessageHandler
{
    Type Handles { get; }
    void Handle(IMessage message);
}

public interface IMessageHandler<T> : IMessageHandler where T : IMessage
{
    void Handle(T message);
}

public abstract class AbstractMessageHandler<T> : AbstractMessageHandler,
    IMessageHandler<T> where T : IMessage
{
    public override Type Handles
    {
        get { return typeof (T); }
    }

    public abstract void Handle(T message);

    public override void Handle(IMessage message)
    {
        Handle((T) message);
    }
}

public abstract class AbstractMessageHandler : IMessageHandler
{
    public abstract Type Handles { get; }

    public abstract void Handle(IMessage message);

    protected IMessageTargetChoice SendMessage(IMessage message)
    {
        return new MessageTargetChoice(message);
    }
}

public class PrivateChatMessageHandler : AbstractMessageHandler<PrivateChatMessage>
{
    public override void Handle(PrivateChatMessage message)
    {
        SendMessage(message).To(message.ReceiverId);
    }
}

In order to handle a different method you just subclass the AbstractMessageHandler. No clutter in any other place.

On a side note: the PrivateChatMessageHandler in this case just forwards the received message to its destination.

The Dispatcher

public class MessageDispatcher
{
    private readonly Dictionary<Type, IMessageHandler> _handlers;

    public MessageDispatcher(IMessageHandler[] handlers)
    {
        _handlers = new Dictionary<Type, IMessageHandler>();

        foreach (var handler in handlers)
        {
            _handlers.Add(handler.Handles, handler);
        }
    }

    public void Dispatch(IMessage message)
    {
        new Thread(() =>
                       {
                           var type = message.GetType();

                           if (!_handlers.ContainsKey(type))
                           {
                               throw new ArgumentException();
                           }

                           _handlers[type].Handle(message);
                       });
    }
}

As you can see, the dispatcher takes an array of IMessageHandler as its constructor argument, every good IoC container will happily fill it when the MessageDispatcher is requested. In StructureMap for example, you’d just say that it should take all classes that implement IMessageHandler, then all you have to do is sub class the AbstractMessageHandler.

Critics about the OCP

Some state that it is in conflict with YAGNI (you ain’t gonna need it) or KISS (keep it simple, stupid). I’d say it depends, if you do it upfront with the vague expectation you might need it, then yes, you at least might violate those two principles. If on the other hand, you have something like a message handler, I’d say its more than fine to implement it in such a way upfront as a message handler handles different message types by design.
Anyway, it’s usually easy to refactor those cases.

SOLID is a combination of five important design principles:
S – Single Responsibility Principle
O – Open Closed Principle
L – Liskov Substitution Principle
I – Interface Segregation Principle
D – Dependency Inversion Principle

This post is about the Single Responsibility Principle.

Definition:

The single responsibility principle states that there should only be one reason for a class to change. To achieve that, every class should have a single responsibility. Thereby, only if something with this one responsibility changes, the class should have to change.

What are the benefits?

Reuse - If all your classes follow the srp, you are more likely to reuse some of them
Clarity - Your code is cleaner as your classes don’t do unexpected things
Naming - As all your classes have a single responsibility, choosing a good name is easy
Readability - Due to the clarity, better names and shorter files the readability is improved greatly.

What is a responsibility?

That is actually the part most have a problem with. By definition, every responsibility is a reason to change.
A responsibility can literally be everything, take for example a customer class, its responsibility is to represent a customer but not how it is loaded from a db or written to it nor is it its responsibility to print a revenue report.

A bad example:

public class Customer
{
    private Guid _id;

    private IList<Order> _orders;

    public Customer(Guid id)
    {
        LoadFromDb(id);
        _id = id;
    }

    public Customer() { }

    public Guid Id
    {
        get { return _id; }
    }

    public string GivenName { get; set; }
    public string LastName { get; set; }

    public IEnumerable<Order> Orders
    {
        get { return _orders; }
    }

    private void LoadFromDb(Guid id)
    {
        using (var con = new SqlConnection())
        {
            using (var cmd = con.CreateCommand())
            {
            	//......
            }
        }
    }

    public void Save()
    {
        if (_id == Guid.Empty)
        {
            Insert();
        }
        else
        {
            Update();
        }
    }

    private void Update()
    {
        using(var con = new SqlConnection())
        {
            using(var cmd = con.CreateCommand())
            {
                //Update
            }
        }
    }

    private void Insert()
    {
        using(var con = new SqlConnection())
        {
            using(var cmd = con.CreateCommand())
            {
                //Insert
            }
        }
    }

    private void PrintRevenueReport()
    {
        // Reportlogic....
    }

    public override string ToString()
    {
        return string.Format("{0}, {1}", LastName, GivenName);
    }
}

This class has to change if either the representation of a customer changes, the database is changed or the layout of a report is changed.

Extracting the additional responsibilites

If we remove the parts that have nothing to do with the representation of a customer we are left with that:

public class Customer
{
    private Guid _id;

    private IList<Order> _orders;

    public Customer(Guid id)
    {
        _id = id;
    }

    public Customer() { }

    public Guid Id
    {
        get { return _id; }
    }

    public string GivenName { get; set; }
    public string LastName { get; set; }

    public IEnumerable<Order> Orders
    {
        get { return _orders; }
    }

    public override string ToString()
    {
        return string.Format("{0}, {1}", LastName, GivenName);
    }
}

As we have to change the Id after an insert and that is no longer inside this class, it shouldn’t be read-only anymore. Further, we can remove the Constructor that requires the id as the class doesn’t care about it any more than about its other properties.

What is left

public class Customer
{
   private IList<Order> _orders;

    public Guid Id { get;set; }
    public string GivenName { get; set; }
    public string LastName { get; set; }

    public IEnumerable<Order> Orders
    {
        get { return _orders; }
    }

    public override string ToString()
    {
        return string.Format("{0}, {1}", LastName, GivenName);
    }
}

Now, it only has to change if the representation of a customer in our application changes. An likely example would be the requirement for an address.

What about the missing parts?

We simply create four interfaces, ICustomerPersister and ICustomerLoader, ICustomerReporter, IReportPrinter:

public interface ICustomerPersister
{
    void Persist(Customer customer);
}

public interface ICustomerLoader
{
    Customer LoadById(Guid id);
}

public interface ICustomerReporter
{
    CustomerReport CreateReportFrom(Customer customer);
}

Public interface IReportPrinter
{
    void PrintReport(Report report);
}

If updating or inserting has additional tasks (notification mail on registration etc.) you shouldn’t implement it all in the persister, instead you should make updating and inserting separate responsibilities and thereby classes. In that case, the persister would just become a facade.

Conclusion

Testing in isolation is now possible as is testing without having to hit any external resources like databases or printers. Additionally, by allowing multiple customers to be passed to the persistence method performance could be increased.

We do now have 5 classes and 4 interfaces for what a single class did before. Some fear that it’s now more complicated to use it as we have to instantiate way more classes. But you shouldn’t argue that way if you take the advantages into consideration. In the last part of this series, you will see that there are many IoC containers that happily take the responsibility of object instantiation away from you.

What about maintenance / code evolution?

Be aware that you have to have an eye on your classes, as time goes by, your classes will collect additional responsibilities that should immediately be refactored.

Developers that are not used to the srp might need some help if they first encounter such a project. They usually feel overwhelmed by the amount of classes / files .

This is part four of my refactoring series, you find the index here

Part one was a short success story, it can be found here.
Part two was a introduction to refactoring, available here.
In part three I talked about the requirements, you can read it here
You can read about the very basic refactoring methods in Part four of my series.

As I said in the last post, I’ll refactor some random open source projects, in this post it’s how updating the database schema is handled in Personal Task Manager.

If you have any suggestions what project to refactor, please leave a comment.

What the update method was before

public static void UpdateDataBase()
{
	bool findNextUpdate = true;
	while (findNextUpdate)
	{
		Configuration oldVersion = ConfigurationHelper.GetConfiguration(ConfigurationKey.DataBaseVersion);
		if (UpdateFromV00ToV09(oldVersion))
			continue;
		if (UpdateFromV09ToV091(oldVersion))
			continue;
		// SNIP (10 more updates)
		if (UpdateFromV_0_9_10_ToV_1_0_0(oldVersion))
			continue;
		if (UpdateFromV_1_0_0ToV_1_0_1(oldVersion))
			continue;
		if (UpdateFromV_1_0_1ToV_1_0_2(oldVersion))
			continue;
		findNextUpdate = false;
		RegisterAddins();
	}
}

Wrong:

As you can see, for every update you create, you have to touch this method, though it’s not complicated, you might simply forget it.

The continues are somewhat useless as applying a update results in the next version.

The call to RegisterAddins has nothing to do with the db updates, it should thereby be removed.

The version is a string, contained in the oldVersion variable as oldVersion.Value.ToString().

What the updates were before

private static bool UpdateFromV_1_0_1ToV_1_0_2(Configuration oldVersion)
{
    if (string.Compare(oldVersion.Value.ToString().Trim(), "1.0.1") == 0)
    {
        try
        {
            DbHelper.AddColumn("ApplicationsLog", "Caption", "VarChar(255)");
            ConfigurationHelper.SaveConfiguration(new Configuration(ConfigurationKey.DataBaseVersion, "1.0.2"));
            return true;
        }
        catch (OleDbException ex)
        {
            Logger.WriteException(ex);
            return false;
        }
    }
    return false;
}

private static bool UpdateFromV_0_9_10_ToV_1_0_0(Configuration oldVersion)
{
    if (string.Compare(oldVersion.Value.ToString().Trim(), "0.9.10") == 0)
    {
        try
        {
            DbHelper.AddColumn("Tasks", "Hidden", "Bit");
            DbHelper.AddColumn("Tasks", "Priority", "Integer");
            DbHelper.AddColumn("Tasks", "Notes", "VarChar(255)");
            ConfigurationHelper.SaveConfiguration(new Configuration(ConfigurationKey.DataBaseVersion, "1.0.0"));
            return true;
        }
        catch (OleDbException ex)
        {
            Logger.WriteException(ex);
            return false;
        }
    }
    return false;
}

Wrong:

The version string, actually object, is always manipulated in the same way

The try / catch block is always the same

Both violates DRY (don’t repeat yourself)

What the updates could look like

public abstract class DbUpdate
{
    public bool CanUpgradeFrom(Version currentVersion)
    {
        return currentVersion == UpgradeableVersion;
    }

    public abstract Version UpgradeableVersion { get; }
    public abstract Version NewVersion { get; }

    public bool Apply()
    {
        try
        {
            ApplyDbUpdate();
            return true;
        }
        catch (Exception ex)
        {
            Logger.WriteException(ex);
            return false;
        }
    }

    protected abstract void ApplyDbUpdate();

    public override string ToString()
    {
        return UpgradeableVersion + " To " + NewVersion;
    }
}

Changes:

The Version strings were compatible with the Version class, therefore the Version class is now used.

The Repeative code is encapsulated in the CanUpgradeFrom and Apply methods. DRY is now followed.

The ToString override is simply for convenience, if we have those DbUpdate instances in a collection and look at them in the debugger, you don’t have to step into them.

Encapsulating the update method into its own class

public class DbUpdater
{
    public void ApplyUpdatesAsNeeded()
    {
        var updates = OrderByVersion(GatherAllUpdates());
        var currentVersion = GetCurrentVersion();

        if (IsLatestVersion(currentVersion, updates))
        {
            return;
        }

        foreach (var update in updates)
        {
            if (!update.CanUpgradeFrom(currentVersion))
            {
                continue;
            }

            if (!update.Apply())
                break;

            currentVersion = update.NewVersion;
            SaveNewDbVersion(currentVersion);
        }
    }

    private bool IsLatestVersion(Version currentVersion, IList<DbUpdate> updates)
    {
        if (updates.Count == 0)
        {
            return true;
        }

        return updates[updates.Count - 1].NewVersion == currentVersion;
    }

    private void SaveNewDbVersion(Version version)
    {
        ConfigurationHelper.SaveConfiguration(new Configuration(ConfigurationKey.DataBaseVersion, version.ToString()));
    }

    private IList<DbUpdate> OrderByVersion(IEnumerable<DbUpdate> updates)
    {
        var result = new List<DbUpdate>(updates);
        result.Sort(delegate(DbUpdate a, DbUpdate b)
        {
            return Comparer<Version>.Default.Compare(a.NewVersion, b.NewVersion);
        });
        return result;
    }

    private Version GetCurrentVersion()
    {
        var versionString = ConfigurationHelper.GetConfiguration(ConfigurationKey.DataBaseVersion)
            .Value.ToString().Trim();

        return new Version(versionString);
    }

    private IList<DbUpdate> GatherAllUpdates()
    {
        var updates = new List<DbUpdate>();

        var types = typeof (DbUpdate).Assembly.GetTypes();
        foreach (var type in types)
        {
            if (!type.IsSubclassOf(typeof(DbUpdate)))
            {
                continue;
            }

            updates.Add((DbUpdate)Activator.CreateInstance(type));
        }

        return updates;
    }
}

Changes:

If the update failed, no other updates will be applied as the version is still the same and it would thereby be useless.

With the IsLatestVersion method the load time was reduced, although the updates are obtained via reflection.

The use of reflection was necessary as no IoC container was used and I didn’t want to introduce one for just this case nor did I want to hardcode each update.

Individual updates in their new form

public class Update101To102 : DbUpdate
{
    public override Version UpgradeableVersion
    {
        get { return new Version(1, 0, 1); }
    }

    public override Version NewVersion
    {
        get { return new Version(1, 0, 2); }
    }

    protected override void ApplyDbUpdate()
    {
        DbHelper.AddColumn("ApplicationsLog", "Caption", "VarChar(255)");
    }
}

public class Update0910To100 : DbUpdate
{
    public override Version UpgradeableVersion
    {
        get { return new Version(0, 9, 10); }
    }

    public override Version NewVersion
    {
        get { return new Version(1, 0, 0); }
    }

    protected override void ApplyDbUpdate()
    {
        DbHelper.AddColumn("Tasks", "Hidden", "Bit");
        DbHelper.AddColumn("Tasks", "Priority", "Integer");
        DbHelper.AddColumn("Tasks", "Notes", "VarChar(255)");
    }
}

The Result:

The db updating part of the application now follows the open closed principle, you don’t have to touch any of the written code, you just add new.

The DbUpdate class follows the single responsibility principle, you only have to change the DbUpdate implementation that represents the current update if you make changes to the db schema.

If you take single responsibility a little broader as in “updating the schema”, the DbUpdater class also follows that principle. (It gathers the updates, orders and applies them, if anything of that changes it has to change too, thereby, if you take the srp strict, it actually has three responsibilities.)

You can’t forget to add the new update as you don’t have to add it anywhere, you just create the new class.

Typos are much less likely as the version is now represented with the Version class.

Updating the db schema is child’s play now.

By the way, the updating method that was used before is a typical example of organically growth, you start by adding the first update method, later a second comes in an you “just add a conditional” but before you realize it, you have an abomination waiting for copy and paste errors. Try to refactor those methods the very moment you want to add this conditional and it’s likely that more will follow. Doing it before might violate YAGNI (You Ain’t Gonna Need It) and / or KISS(Keep It Simple Stupid)

A word to testing: I didn’t create any tests as there was no way to do it without really great effort. Almost everything in that application is static thereby stubbing / mocking alone would have required some serious changes and hard coding a test would just verify that “yes you can upgrade from specific version X to specific version Y”, not that useful compared to the required effort.

This is part four of my refactoring series, you find the index here

Part one was a short success story, it can be found here.
Part two was a introduction to refactoring, available here.
In part three I talked about the requirements, you can read it here

In this part I’ll talk about very basic refactoring methods, something you can start with right away, without further knowledge and the like.

The following parts will contain “real world” refactoring from random open source projects.

Naming:

It’s very important to name your classes, methods and variables properly. If you have trouble to find a good name for a method or class, chances are high that it is doing too much. In such a case, you should split it up.

I don’t think that I should give you yet another example of bad naming, every developer knows the problems that arise from bad naming.

Conditionals:

Try to avoid unnecessary nesting, use guard clauses instead.

You might also try to consolidate the conditionals but be careful with it as readability might suffer. The favorable way is to encapsulate the whole condition in its own method.

Nested:

if (string.IsNullOrEmpty(input) == false))
{
    if (input.StartsWith("ABC"))
    {
        if (input.EndsWith("123"))
        {
            DoSomething(input);
            DoSomethingMore(input);
        }
    }
}

With guard clauses:

if (string.IsNullOrEmpty(input))
{
    return;
}

if (!input.StartsWith("ABC"))
{
    return;
}

if (!input.EndsWith("123"))
{
    return;
}

DoSomething(input);
DoSomethingMore(input);

Consolidated:

if (string.IsNullOrEmpty(input) || !input.StartsWith("ABC")
    || !input.EndsWith("123"))
{
    return;
}

DoSomething(input);
DoSomethingMore(input);

Encapsulated:

if (InputIsNotWellFormatted(input))
{
    return;
}

DoSomething(input);
DoSomethingMore(input);

Method Extraction:

Long methods should be split up, now we have to define what long means, some say your methods shouldn’t have more than 10 lines, I’d say, it depends on it’s readability. Yet I’d try to stay blow 20 lines.

If you have trouble to name a method properly, it’s actually a no brainer to refactor it because you should know what you want it to do, thereby you can simply extract what doesn’t belong there. It’s a little more difficult if the method is already dealing with just a single problem, yet the method covers multiple screens. In those cases you have to think how to split it up properly. More on that in the next parts.
Another reason for method extraction is pure readability.

Example:
If you want to process a customer registration, you validate it and only if it’s valid, it’s a real customer. The next steps would be to save it and send an activation mail out.

You could do it this way:

public void ProcessCustomerRegistration(CustomerRegistration registration)
{
    if (registration == null)
    {
        throw new ArgumentException();
    }

    if (registration.Adress == null)
    {
        throw new ArgumentException();
    }

    if (registration.ImportantProperty == null)
    {
        throw new ArgumentException();
    }

    if (registration.AnotherImportantProperty == null)
    {
        throw new ArgumentException();
    }

    var customer = new Customer();
    customer.Adress = registration.Adress;
    customer.Username = registration.Username;
    customer.OtherProperty = registration.OtherProperty;

    using (var con = new SqlConnection(_conStr))
    {
        using (var cmd = con.CreateCommand())
        {
            cmd.CommandText = "Insert Command";
            //parameter.....

            con.Open();
            cmd.ExecuteNonQuery();
        }
    }

    using (var mailClient = new MailClient())
    {
        var mail = new Mail()
        //.......
        //.......
        //.......
        //.......
        //.......
        mailClient.Send(mail);
    }
}

Or you could make your method reflect what you want to do:

public void ProcessCustomerRegistration(CustomerRegistration registration)
{
   if(IsInvalid(registration))
       throw new ArgumentException();

    var customer = ConvertToCustomer(registration);
    Persist(customer);
    SendActivationEMailTo(customer);
}

This is part three of my refactoring series, you find the index here

Part one was a short success story, it can be found here.
Part two was a introduction to refactoring, available here.

In this part I’ll talk about what is required to start refactoring and what tools can help with it.

You have to care about the code:

This is the single most important point. If you don’t care about your code, refactoring is useless as you won’t gain anything from it because after a very short time you will stop refactoring it and thereby allowing it’s quality to degrade again. The good news is, as you are reading this, you obviously care about your code.

Do it continuously:

Although refactoring is still very great if done together with scheduled code reviews, the real power lies in continuous refactoring. Whenever you find something you don’t like, just change it right away. Code is not static, you won’t hurt anyone by changing it, quite the opposite is the case. If you made an awkward piece of code into something everyone understands without looking anything up, you saved those involved time. This is something you’d miss if you’d only refactor while doing scheduled code reviews. Although you’ll save time afterwards, you already lost some because those who worked with it had to look something up, read it twice or anything similar.

Testing:

Unit testing is not a fix requirement for refactoring, yet it gives you the confidence that you didn’t break anything with your changes. Further, unit testing will enable other changes as well, still for the same reason, you know when you broke something. If you are working without tests, you’ll most likely hesitate to change something that already works. Though you should remember, if the code is bad, maintenance hell is one step away. If you have to fix a bug in bad code, it’ll take longer and won’t be any fun at all.
If you have no experience with unit tests, I’d advice you to learn about it. Refactoring and unit testing are a perfect fit. This, this and this would be good starting points.

Don’t fight alone:

Try to win your team, all of you will have it easier in the end. Refactoring in a pair is not only more efficient, as four eyes see more than two, it’s also fun to listen to you coworker, especially if he wrote the code you are refactoring.
If no one besides you wants to do it, you could just start to refactor the parts you are working on and convince you team later by showing your results.
But be warned, refactoring alone is not easy, others might destroy your work by just getting their work done. It’s also important that you don’t refactor too much, you could fall behind with your tasks and thereby get in trouble. Try to meet schedules and refactor only when there is time.
The good news is that usually just a little effort is enough to win a few team members and if they start refactoring too, it’ll be easier for all of you.

Tools

Visual Studio AddIns:
JetBrains ReSharper: I just can’t work without it anymore, really great.
DevExpress CodeRush / Refactor: Although I never used it, many speak very highly about it.

Unit testing and mocking
MbUnit: My unit testing framework of choice
NUnit: Port of the popular JUnit.
Rhino.Mocks: Simply the best mocking framework I know.
Moq: Another great mocking framework.

This is part two of my refactoring series, you can find the index here.

Part one was a short success story, it can be found here.

In this part I’ll introduce refactoring, what it is, what it can do for you and why you should do it.

What is Refactoring?

Refactoring is changing the appearance and structure of your code but not its behavior. Refactoring is an answer to ever changing requirements and the resulting modifications to your code. It won’t solve all problems but it will make your life way easier by improving the code quality.

What can it do if it is just a cosmetic change?

Have you ever struggled with reviewing someone else’s code or even your own after a month or two? Refactoring will help in these situations and more. If applied correctly, the code will be much easier to work with.

Does it really work?

Yes. Take for example the following snippet:

public IEnumerable<ContentInformation> GetContentFor(IEnumerable<MatrixKey> selection)
{
    var lowX = selection.OrderBy(x => x.X).First().X;
    var lowY = selection.OrderBy(x => x.Y).First().Y;

    foreach (var key in selection)
    {
        var x = key.X-lowX;
        while(x > _upperXBound)
        {
            if(_upperXBound == 0)
            {
                x = 0;
            }
            else
            {
                x -= _upperXBound + 1;
            }
        }
        var y = key.Y-lowY;
        while(y > _upperYBound)
        {
            if(_upperYBound == 0)
            {
                y = 0;
            }
            else
            {
                y -= _upperYBound+1;
            }
        }

        var internalKey = new MatrixKey(x, y);

        if (_content.ContainsKey(internalKey))
        {
            yield return _content[internalKey].CloneTo(key);
        }
        else
        {
            yield return new NullContentInformation(key);
        }
    }
}

And now, compare it to its refactored version:

public IEnumerable<ContentInformation> GetContentFor(IEnumerable<MatrixKey> selection)
{
    var lowX = selection.OrderBy(x => x.X).First().X;
    var lowY = selection.OrderBy(x => x.Y).First().Y;

    foreach (var key in selection)
    {
        var x = Transform(key.X, lowX, _upperXBound);
        var y = Transform(key.Y, lowY, _upperYBound);

        var internalKey = new MatrixKey(x, y);

        if (_content.ContainsKey(internalKey))
        {
            yield return _content[internalKey].CloneTo(key);
        }
        else
        {
            yield return new NullContentInformation(key);
        }
    }
}

private int Transform(int source, int lowerBound, int upperBound)
{
    if(upperBound == 0)
    {
        return 0;
    }

    var transformed = source - lowerBound;

    while(transformed > upperBound)
    {
        transformed -= upperBound + 1;
    }

    return transformed;
}

Why should you do it?

For one, it will make your life easier, you won’t get as much headaches from two screens long methods with nesting up to five levels deep. Simply because they just won’t exist anymore.
It will save money as the time to feature as well as time to fix a bug will reduce. New developers will be able to adopt your code earlier too.

This is part one of my refactoring series, you can find the index here.

In this part I’ll tell a short story about refactoring and give a conclusion.

The Story

In my company we have a legacy VB6 application, a trainee started it about 6 years ago. As the requirements were very low, it took only a week to finish it as it. The expected lifetime was 3 month as it should just last the time that was required to evaluate proprietary solutions.

As it was just a quick and dirty solution by design, all ui elements still had their default name (command1, command2, label1, label2 etc.). Everything was done in the event handling methods (command1_click).

As time passed, it was decided to extend the application a little and as the trainee writing it in the first place left, it was done by new trainee. This trainee started the habit of using copy and paste. Code was simply copied and pasted whenever it was needed.

Three months later, the application has already seen 4 different trainees as it was the companies way of “testing new trainees”. It had about 5000 lines of code, the main form contained 3000 of those. The control names reached the 100, structure was not existent. But still, management decided, it was better than the ready and proven solutions and so it came that it kept being used. The only good thing was, it was no longer used to terrify new trainees.

From time to time, new features were requested and implemented, again without any structure, it was just kept as dirty as it was, everyone involved just wanted to get his work done. By all means.

One year ago, I started in the company. As I was mostly responsible for C# and our SQL Server, I had very little to do with our legacy applications but 2 month ago, my coworker who’s main task is to maintain the VB6 stuff, approached me and asked for help. It turned out, he was trying to implement a new feature and that since two weeks already.

This was the first time I encountered this mess, until then, I just heard complaints of my coworker and thought he was over exaggerating, it turned out he did not. In fact, it was much worse. By then, it consisted of 20000 lines of code and was just a mess with no structure at all. Almost no useful ui element names (my co worker named the elements he worked with properly, but as it seems, he was the first to do so.). Most event handling methods spanned multiple screens, nesting was up to 10 levels deep and that mostly without any indentation. The only used pattern was copy and paste.

I suggested to implement the new feature in isolation, therefore we created a new module and wrote it there. My coworker asked me now how we could make it work together with the rest of the code, my answer was that we would refactor the affected parts. As he never heard about refactoring, I had to explain it to him. Although he was a little skeptical about it, we began to change the existing methods but instead of just making it work together with the new module, I encouraged him to get all touched methods refactored as well.

After five days, the new feature worked nicely and was easy to maintain and change. We encapsulated about ten more distinct features into their own module. All of this lead to way better code. My coworker was so fond of it that we started to refactor more and more parts of the application and in just 2 month, we managed to reduce the 20000 lines of pure horror to just above 8000 lines of self explaining code. But the reduction was not the only benefit, the time to new feature was reduced drastically as well, from weeks to just days or less.

The Conclusion

You should always consider that you are not the only one who will touch the code. Even if that is not the case, in two month you are “someone else” too, at least in terms of code knowledge. Therefore, code should be written in a self explaining way.

Writing good code in the beginning has also the benefit of protecting it against getting bad in the near future. You can observe it in other areas too, if you have for example a clean room, it is likely to stay clean, dirty rooms on the other hand tend to get dirtier. The reason for that is, we think that someone cares about the clean room and therefore try to keep it clean but as no one seems to care about the dirty room, it is much easier for us to be lazy and just let it get dirtier.

If you didn’t create the mess, you shouldn’t just “deal with it”, you should improve it. That does not mean that you should stop to do anything else, but you should improve those areas you touch while maintaining the mess. After some time, the application as a whole will improve.

Bad code is not only a cosmetic problem, the time to feature and thereby cost of new features increase, bug fixes affected by this too. If the code quality is too bad, some suggest a rewrite. But that is not only expensive, it won’t fix the problem at all, it is just matter of time until the code quality is that bad quality again.