Essential Developer Principles #4 – Open Closed Principle

The “Open Closed Principle” is usually summarised as code should be “open to extension” but “closed to modification”. The way I often express it is that when I am adding a new feature to an application, I want to as much as possible be writing new code, rather than changing existing code.

However, I noticed there has been some pushback on this concept from none other than the legendary Jon Skeet. His objection seems to be based on the understanding that OCP dictates that you should never change existing code. And I agree; that would be ridiculous. It would encourage an approach to writing code where you added extensibility points at every conceivable juncture – all methods virtual, events firing before and after everything, XML configuration allowing any class to be swapped out, etc, etc. Clearly this would lead to code so flexible that no one could work out what it was supposed to do. It would also violate another well-established principle – YAGNI (You ain’t gonna need it). I (usually) don’t know in advance in what way I’ll need to extend my system, so why complicate matters by adding numerous extensibility points that will never be used (or more likely, still need to be modified before they can be used)?

So in a nutshell, here’s my take on OCP. When I’m writing the initial version of my code, I simply focus on writing clean maintainable code, and don’t add extensibility points unless I know for sure they are needed for an upcoming feature. (so yes, I write code that doesn’t yet adhere to OCP).

But when I add a new feature that requires a modification to that original code, instead of just sticking all the new code in there alongside the old, I refactor the original class to add the extensibility points I need. Then the new feature can be added in an isolated way, without adding additional responsibilities to the original class. The benefit of this approach is that you get extensibility points that are actually useful (because they are being used), and they are more likely to enable further new features in the future.

OCP encourages you to make your classes extensible, but doesn’t stipulate how you do so. Here’s some of the most common techniques:

• Pass dependencies as interfaces into your class allowing callers to provide their own implementations
• Add events to your class to allow people to hook in and insert steps into the process
• Make your class suitable as a base class with appropriate virtual methods and protected fields
• Create a “plug-in” architecture which can discover plugins using reflection or configuration files

It is clear that OCP is in fact very closely related to SRP (Single Responsibility Principle). Violations of OCP result in violations of SRP. If you can’t extend the class from outside, you will end up sticking more and more code inside the class, resulting in an ever-growing list of responsibilities.

In summary, for me OCP shouldn’t mean you’re not allowed to change any code after writing it. Rather, it’s about how you change it when a new feature comes along. First, refactor to make it extensible, then extend it. Or to put it another way that I’ve said before on this blog, “the only real reasons to change the existing code are to fix bugs, and to make it more extensible”.

SOLID Code is Mergeable Code

Not every developer I speak to is convinced of the importance of adhering to the “SOLID” principles. They can seem a bit too abstract and “computer sciency” and disconnected from the immediate needs of delivering working software on time. “SOLID makes writing unit tests easier? So what, the customer didn’t ask for unit tests, they asked for an order billing system”.

But one of the very concrete benefits of adhering to SOLID is that it eases merging for projects that require parallel development on multiple branches. Many of the commercial products I have worked on have required old versions to be supported with bug fixes and new features for many years. It is not uncommon to have more than 10 active branches of code. As nice as it would be to simply require every customer to upgrade to the latest version, that is not possible in all commercial environments.

One customer took well over a year to roll out our product to all their numerous sites. During that time we released two new major versions of our software. Another customer requires a rigorous and lengthy approval testing phase before they will accept anything new at all. The result is that features and bug fixes often have to be merged through multiple branches, some of which have diverged significantly over the years.

So how do the SOLID principles aid merging?

The Single Responsibility Principle states that each class should only have a single reason to change. If you have a class that constantly requires changing on every branch, creating regular merge conflicts, the chances are it violates SRP. If each class has only a single responsibility there is only a merge conflict if both branches genuinely need to modify that single responsibility.

Classes that adhere to the Open Closed Principle don’t need to be changed at all (except to fix bugs). Instead, they can be extended from the outside. Two branches can therefore extend the same class in completely different ways without any merge conflict at all – each branch simply adds a new class to the project.

Designs that violate the Liskov Substitution Principle result in lots of switch and if statements littered throughout the code, that must be modified every time we introduce a new subclass into the system. In the canonical ‘shapes’ example, when the Triangle class is introduced on one branch, and the Hexagon class is created on another, the merge is trivial if the code abides by LSP. If the code doesn’t, you can expect a lot of merge conflicts as every place in the code that uses the Shape base class is likely to have been modified in both branches.

The Interface Segregation Principle is in some ways the SRP for interfaces. If you adhere to ISP, you have many small, focused interfaces instead of few large, bloated interfaces. And the more your codebase is broken down into smaller parts, the lower the chances of two branches both needing to change the same files.

Finally, the Dependency Inversion Principle might not at first glance seem relevant to merges. But every application of DIP is in fact also a separation of concerns. The concern of creating your dependencies is separated away from actually using them. So applying DIP is always a step in the direction of SRP, which means you have smaller classes with more focused responsibilities. But DIP has another power that turns out to be very relevant for merging.

Code that adheres to the Dependency Inversion Principle is much easier to unit test. And a good suite of unit tests is an invaluable tool when merging lots of changes between parallel branches. This is because it is quite easily possible for merged code to compile and yet still be broken. When merges between branches are taking place on a weekly basis, there is simply not enough time to run a full manual regression test each time. Unit tests can step into the breach and give a high level of confidence that a merge has not broken existing features. This benefit alone repays the time invested in creating unit tests.

In summary, SOLID code is mergeable code. SOLID code is also unit testable code, and merges verified by unit tests are the safest kind of merges. If your development process involves working on parallel branches, you will save yourself a lot of pain by mastering the SOLID principles, and protecting legacy features with good unit test coverage.

Merge-Friendly Code

If you are working a large application where you have to simultaneously support hotfixes, service packs and new features for customers running several different versions, while developing new versions of your software, chances are you have some kind of branching structure in your source control system.

And before too long, you will experience the joy of merging features from one branch into another. Here’s my top six tips for writing code that is easy to merge…

1. Little and Often

The first principle is that it is better to make many small, focused check-ins, and merge them early, rather than checking a vast collection of changes in one hit and attempting a gigantic merge after several months of development. Sadly, this is not always possible, as sometimes a major change has to be kept out of a branch for a long time.

2. Get Your Branch Strategy Right

It is worth spending some time making sure you choose the correct branching strategy. A lot could be said about this, but here’s just two things to consider.

Don’t put two major features into one branch unless you are willing to deploy them together. Although you can “cherry-pick” changesets to merge, the reality is that this only works if your changes are completely isolated from one another. In other words if changeset B depends on something that was in changeset A, then you can’t just merge changeset B into another branch, changeset A has to come along for the ride too.

Also, avoid getting into the situation where you need to merge between two branches that are not directly related. In TFS, these are called “baseless merges”, and they can result in deleted code getting re-inserted.

3. Check in Clean Code

Nothing messes up a merge more quickly than someone making widespread “cosmetic” changes – introducing or deleting whitespace, renaming things, moving methods to a different part of the file, etc. Sweeping changes like this have a high probability of conflicting with someone else’s change.

The solution is of course, to reduce the need for this kind of change by making sure that what you check in is formatted correctly, and follows the appropriate coding standards and naming conventions. Tools like StyleCop, Resharper, and FxCop are all able to help here.

4. Single Responsibility Principle (SRP)

Simply put, the Single Responsibility Principle dictates that every class should have one and only one responsibility. If it has two or more, you should extract functionality into additional classes. Similarly every method should perform one and only one task.

Adhering to this straightforward principle results in many classes, each composed of short methods. Very often merge conflicts are due to two people working on the same file or method, but changing it for very different reasons. But if a class or method has only “one reason to change”, then the chances of two developers working on different features needing to simultaneously change it are greatly reduced.

5. Open Closed Principle (OCP)

The Open Closed Principle states that classes should be open for extension but closed for modification. Or to say it another way, it should be possible to add new features and capabilities to your codebase simply by creating new classes, rather than having to mess with the internals of existing classes. And if you use technologies like MEF, it really is possible to add whole new features without touching a single line of your existing codebase.

Obviously, in any large real-world application, there will always be the need to make some changes to legacy code. But this should be the exception rather than the norm. In fact, the only real reasons to change the existing code are to fix bugs, and to make it more extensible.

If commits to source control of new features consist mostly of changes to existing files rather than the creation of new ones, maybe you need to do some research into how you can better design your classes to conform to OCP.

6. Unit Tests

After performing a merge, it is vital that you are able to determine as quickly as possible if you made any mistakes in the conflict resolution process. A unit test suite with good code coverage is invaluable in helping with this task. Obviously this may need to be complemented with further integration testing and manual testing, but the quicker you can identify problems with a merge, the better.

So those are my top recommendations for avoiding merging nightmares. Anyone got any other suggestions?

Where are you going to put that code?

Often we want to modify existing software by inserting an additional step. Before we do operation X, we want to do operation Y. Consider a simple example of a LabelPrinter class, with a Print method. Suppose a new requirement has come in that before it prints a label for a customer in Sweden, it needs to do some kind of postal code transformation.

Approach 1 – Last Possible Moment

Most developers would immediately gravitate towards going to the Print method, and putting their new code in there. This seems sensible – we run the new code at the last possible moment before performing the original task.

public void Print(LabelDetails labelDetails)
{
    if (labelDetails.Country == "Sweden")
    {
        FixPostalCodeForSweden(labelDetails);
    }
    // do the actual printing....
}

Despite the fact that it works, this approach has several problems. We have broken the Single Responsibility Principle. Our LabelPrinter class now has an extra responsibility. If we allow ourselves to keep coding this way, before long, the Print method will become a magnet for special case features:

public void Print(LabelDetails labelDetails)
{
    if (labelDetails.Country == "Sweden")
    {
        FixPostalCodeForSweden(labelDetails);
    }
    if (printerType == "Serial Port Printer")
    {
        if (!CanConnectToSerialPrinter())
        {
            MessageBox.Show("Please attach the printer to COM1");
            return;
        }
    }
    // do the actual printing....
}

And before we know it, we have a class where the original functionality is swamped with miscellaneous concerns. Worse still, it tends to become untestable, as bits of GUI code or hard dependencies on the file system etc creep in.

Approach 2 – Remember to Call This First

Having seen that the LabelPrinter class was not really the best place for our new code to be added, the fallback approach is typically to put the new code in the calling class before it calls into the original method:

private void DoPrint()
{
    LabelDetails details = GetLabelDetails();
    // remember to call this first before calling Print
    DoImportantStuffBeforePrinting(details);
    // now we are ready to print
    labelPrinter.Print(details);
}

This approach keeps our LabelPrinter class free from picking up any extra responsibilities, but it comes at a cost. Now we have to remember to always call our DoImportantStuffBeforePrinting method before anyone calls the LabelPrinter.Print method. We have lost the guarantee we had with approach 1 that no one call call Print without the pre-requisite tasks getting called.

Approach 3 – Open – Closed Principle

So where should our new code go? The answer is found in what is known as the “Open Closed Principle”, which states that classes should be closed for modification, but open for extension. In other words, we want to make LabelPrinter extensible, but not for it to change every time we come up with some new task that needs to be done before printing.

There are several ways this can be done including inheritance or the use of the facade pattern. I will just describe one of the simplest – using an event. In the LabelPrinter class, we create a new BeforePrint event. This will fire as soon as the Print function is called. As part of its event arguments, it will have a CancelPrint boolean flag to allow event handlers to request that the print is cancelled:

public void Print(LabelDetails labelDetails)
{
    if (BeforePrint != null)
    {
        var args = new BeforePrintEventArgs();
        BeforePrint(this, args);
        if (args.CancelPrint)
        {
            return;
        }
    }
    // do the actual printing....
}

This approach means that our LabelPrinter class keeps to its single responsibility of printing labels (and thus remains maintainable and testable). It is now open for any future enhancements that require an action to take place before printing.

There are a couple of things to watch out for with this approach. First, you would want to make sure that whenever a LabelPrinter is created, all the appropriate events were hooked up (otherwise you run into the same problems as with approach 2). One solution would be to put a LabelPrinter into your IoC container ready set up.

Another potential problem is the ordering of the event handlers. For example, checking if you have permission to print would make sense as the first operation. The simplest approach is to add the handlers in the right order.

Conclusion

Always think about where you are putting the new code you write. Does it really belong there? Can you make a small modification to the class you want to change so that it is extensible, and then implement your new feature as an extension to that class? If you do, you will not only keep the original code maintainable, but your new code is protected from accidental breakage, as it is isolated off in a class of its own.