Coding Efficiency

This is in reply to Ayendes post.

For those who haven’t followed the discussion, it all began with the release of NHibernate 2.1.

Shortly thereafter, Patrick Smacchia made this post where he analyzed what had changed in NHibernates code base and measured its quality with NCover. He came to the conclusion that it must be a daily pain to maintain the code.

In reply to that, Ayende said that it is structured, although not by name spaces and the like (Patrick’s analyses made that assumption) but different. To underline that NHibernate is indeed maintainable, he pointed out that several new committers joined last year, that there were many new features while at the same time fewer issues.

Frans Bouma made a comment where he said that even a mess looks like something acceptable to those who work with it. This lead to Ayendes post to which I respond.

I have to say, I agree with Ayende. Rating how maintainable a code base is can only be done by those who actually maintain it.

If you have to deal with an application written in a language you don’t know, would you say it’s unmaintainable just because you’re not familiar with the language? Code bases are to some extent like that as many, especially larger ones, have their very own conventions and those conventions aren’t limited to naming conventions and the like but can span the structural or architectural level too.

The VB6 application I talked about in my refactoring series was very easy to learn as the only used pattern was copy and paste. You’d simply go in, copy what you need and paste it into its new place, everyone could do it, everyone did it. However, the result was an unmaintainable mess.

Therefore I’d say that although learning a code base is required to effectively maintain it, learning and maintaining it are two different metrics that should be looked at independently. The learning cost is a one time cost, fixing a bug or adding a feature isn’t.

Yet another story is ongoing learning, this has directly to do with maintainability as no one can remember every line of an entire code base that has a sufficient size.

If a code base is consistent with its conventions, no matter how strange they may first seem to outsiders, there shouldn’t be a problem as this ongoing learning cost will stay at a low level.

Please note that I’m not saying “that’s a convention” excuses everything. There has to be a structure and concepts have to be followed but this structure or these concepts don’t necessarily have to jump the outsider right in the eye as it is the case in the classical three tier architecture for example.

A very good explanation of what dependency injection is can be found here.

Very short: Dependency injection allows you to decouple your code and to make it more unit test friendly.

Another commonly used phrase for dependency injection (DI) is inversion of control (IoC).

What is a DI / IoC container?

A DI or IoC container is basically just a type map that returns a specific implementation for a requested type. To do so, it has to be told in some way what it should return if it is asked for a given type.

Most, if not all, DI Containers automatically resolve dependencies. You’ll find an example below.

The benefits are less code duplication, you specify a concrete implementation at most once and easier unit testing because you can test each component in isolation.

The only downside is the little upfront effort that is required to configure the container.

What is StructureMap?

StructureMap is a popular DI Container with a very easy to use api for both, normal use and configuration.

The mainly used method of StructureMap is in the static ObjectFactory, the methods signature is T GetInstance<T>(). This method is enough for most use cases.

StructureMaps configuration capabilities are also very interesting as they are powerful while easy to use.

You can get it and further information here.

A very basic example:

We have these interfaces:

public interface IA
{
    IB B { get;}
}
public interface IB
{
    IC C { get; }
}
public interface IC { }

With these implementations:

public class A : IA
{
    private readonly IB _b;

    public A(IB b)
    {
        _b = b;
    }

    public IB B
    {
        get { return _b; }
    }
}

public class B : IB
{
    private readonly IC _c;

    public B(IC c)
    {
        _c = c;
    }

    public IC C
    {
        get { return _c; }
    }
}

public class C : IC { }

Such a construct has a long and ugly instantiation, new A(new B(new C()))). Unit testing the class that makes this call is limited as it directly depends on A, B and C.

The other option would be to create B in A’s constructor and C in B’s but doing so would harm unit testing even more as testing A without B and B without C is impossible.

Before I show you how to configure StructureMap, you should know that StructureMap uses so called Registries for in-code configuration and that you’d normally configure it as early as possible.

In WinForms “as early as possible” might be before calling Application.Run. If you name the registry ClientRegistry, it could look like this:

public static void Main(params string[] args)
{
    ObjectFactory.Initialize(x => x.AddRegistry(new ClientRegistry());
    Application.Run(ObjectFactory.GetInstance<Form1>());
}

And here are two ClientRegistry implementations that would do the job for the above case:

public class ClientRegistry : Registry
{
    public ClientRegistry()
    {
        ForRequestedType<IA>()
            .TheDefaultIsConcreteType<A>();

        ForRequestedType<IB>()
            .TheDefaultIsConcreteType<B>();

        ForRequestedType<IC>()
            .TheDefaultIsConcreteType<C>();
    }
}

public ClientRegistry()
{
    Scan(x =>
             {
                 x.TheCallingAssembly();
                 x.WithDefaultConventions();
             });
}

Both would map IA to A, IB to B and IC to C, in implementation 1 it’s obvious but it looks like code duplication. In implementation 2 its not obvious at all, basically all it does is looking through all types in the calling assembly and to map those types that conform to the default conventions. The default convention is mapping interfaces to classes with the same name just without the leading I.

All you have to do now is call ObjectFactory.GetInstance<IA>(), in this case you’ll get an instance of A.

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.

Nothing is more frustrating than an application crash, well nothing except a crash without any information.

There are always conditions where errors occur, even if only unbelievable stupidity can lead to such an error, your application should never crash without a word.

Don’t say nothing

Something like “Argument Exception has been thrown. Shutting down” is as good as nothing, no one knows what failed, how bad it was etc. Your error messages should be understandable but on the other hand, they shouldn’t fill entire pages.

Your users aren’t developers

Well, as long as you don’t write IDE plug-ins etc. this should be true

Although “Db call returned 0, expected 1” is the internal error and most developers will know “Ah, an insert/update/delete etc was tried but it failed as there was no row affected.”, the average user won’t understand it.

Provide that there was an error, where it occurred and what the consequences are. Let’s say, the user tried to update something and that resulted in the message above, the message could be written as: “We apologize but there was an error in the database. Your changes have not been saved.”, developers still know “Ah, there was an error in the database” and the average user knows “Oh, my changes weren’t saved, I have to do them again”.

Error recovery

Even if a terrible error occurred, as long as the application was able to correct it, you shouldn’t announce it as the user wouldn’t care anyway. Logging is a different story though.

If you can offer useful tips about what to do after what ever just happened but the application can’t do it alone, show them right away. Your average user will be more than happy if he knows what to do now.

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 .

As I was looking for another open source project for my refactoring series, I saw both, very good and very bad code.

As many projects, this one just wanted to implement three tiers, with the data and the business layer having their own data containers. Although they are equal by content, they are different classes.

I really don’t understand why anyone would do such a thing:

// In some method that needs an "Category" object
Category category = DBMapping(dbItem);

// Somewhere else, revealing what the dbItem is....
private static Category DBMapping(DBCategory dbItem)
{
    if (dbItem == null)
        return null;

    Category item = new Category();
    item.CategoryID = dbItem.CategoryID;
    item.Name = dbItem.Name;
    item.Description = dbItem.Description;
    item.TemplateID = dbItem.TemplateID;
    item.MetaKeywords = dbItem.MetaKeywords;
    item.MetaDescription = dbItem.MetaDescription;
    item.MetaTitle = dbItem.MetaTitle;
    item.SEName = dbItem.SEName;
    item.ParentCategoryID = dbItem.ParentCategoryID;
    item.PictureID = dbItem.PictureID;
    item.PageSize = dbItem.PageSize;
    item.PriceRanges = dbItem.PriceRanges;
    item.Published = dbItem.Published;
    item.Deleted = dbItem.Deleted;
    item.DisplayOrder = dbItem.DisplayOrder;
    item.CreatedOn = dbItem.CreatedOn;
    item.UpdatedOn = dbItem.UpdatedOn;

    return item;
}

Doing so provides nothing, you just violate DRY and thereby create your self some great amount of repetive work. Further, human errors like forgetting a property will arise.

Another thing I encountered rather often is the following:

private IList<Category> GetAllCategories()
{
    return DataLayer.CategoryManager.GetAllCategories();
}

The so called business layer is nothing more than a very thin proxy to the data layer. If you used interfaces for decoupling in both, the business and the data layer, you have to change 3 places in exactly the same way, doesn’t that feel just wrong?

If all you want to do is to allow different data stores, then just do that. There is absolutely no reason to duplicate your data containers in such a case.

You have at least three ways:

Let the different data layer implementations depend on the business layer. Define the data containers and the interfaces to retrieve those in the business layer and let your IoC container do the rest.

Create a separate assembly that contains the data containers. You can still let the business layer depend on the data layer, yet you don’t have to duplicate anything.

Use an ORM, like NHibernate.

If you ever created a project with just 2 parts, usually named ui/gui/presentation and core/logic/service, it’s actually the same. The only difference is that you implement the interfaces by which you retrieve your entities / business object, or however you want to call them, in a separate assembly. Done. Data stores of all kind can be implemented.

By doing so, you can unit test, without having to hit your real data store. Further, you can switch databases, just load a different data layer implementation and its done.

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.