Pragmatic SOLID Part 2 – The Open/Closed Principle

Software entities (classes, modules, functions etc.)ย  should be open for extension but closed for modification- Robert C. Martin February 1997

TL;DR

  • Let the act of changing code be the catalyst for protecting against that kind of change in future.
  • Not every change can be protected against, but the action of closing methods allows easy extension later.
  • Use programming features (such as abstraction and interfaces) to enable addition or removal of leaf classes without impacting existing code.
  • Avoid distributing RunTime Type Information across your code-base, it makes your code brittle.
  • Classes within a derivation should know nothing of other concrete classes in that derivation – Try and keep ‘set’ logic away from consuming code and concretions.
  • And remember – “Resisting premature abstraction is as important as abstraction itself” Robert C Martin.

What’s the business value?

The ideal outcome of applying the Open/Closed Principle (OCP) is to reduce costs through designing classes and interfaces which withstand a degree of expected change, such that it will have little impact on existing code. This can reduce consequential errors and increase future velocity. Protecting against changes which never materialize is a poor return on investment.

Come again?

The Open aspect of the principle means preparing code to be extensible in future. That could be through abstraction and extracting type-detection logic from consuming classes

E.g. Code immediately takes advantage of a new leaf class in an existing class hierarchy without impacting existing code.

The Closed aspect of the principle means protecting code from the risks of change.

E.g. Adding or removing a leaf class in an existing class hierarchy has a low impact on existing code.

This is simply applying principles of polymorphism, encapsulation, information hiding and abstraction appropriately.

Naturally, if you add a method to a class it is likely to require review and modification of previously Closed code. While you are making those changes ask yourself “if I make this kind of change again, can I protect against having to do so much work?”. That’s the attitude the principle is trying to promote.

Show me an example

Example 1 – Common OCP application
This example uses the concept of a Graph and Printer class from my previous post – The Single Responsibility Principle, but ignores the Single Responsibility Principle for a while.

Pretend a new junior programmer called Bob is introducing a new printer class for a 3d printer, to be used by the MyGraph class. Being a complete idiot, Bob passes in two printer classes and conditionally calls print on each as the code below shows.

namespace OCP1
{
    class Printer2d
    {
        public void Print(){ }
    }

    class Printer3d
    {
        public void Print() { }
    }

    class MyGraph
    {
        public MyGraph() { ; }
        public void Print(Printer2d printer2d, Printer3d printer3d)
        {
            if (printer2d != null)
            {
                printer2d.Print();
            }
            else if (printer3d != null)
            {
                printer3d.Print();
            }
            else
            {
                // must be a new type of printer
            }
        }
    }

    class TestClass
    {
        public void Test()
        {
            MyGraph graph = new MyGraph();
            graph.Print(null, new Printer3d());
        }
    }
}

Bob’s co-worker, Dave, slaps Bob around the head and tells him to create an abstract base class. He also asks Bob “What happens when we pass both printers, do you really intend to print twice?”. Bob’s review might be awkward if he continues to be an idiot. By introducing an abstract base class (or an interface) we’ve closed off the Print function to that type of change in future. Here’s how that looks.

namespace OCP2
{
    abstract class Printer
    {
        public abstract void Print();
    }

    class Printer2d : Printer
    {
        override public void Print() { }
    }

    class Printer3d : Printer
    {
        override public void Print() { }
    }

    class MyGraph
    {
        public MyGraph() { ; }
        public void Print(Printer anyPrinter)
        {
            anyPrinter.Print();
        }
    }

    class TestClass
    {
        public void Test()
        {
            MyGraph graph = new MyGraph();
            graph.Print(new Printer3d());
            graph.Print(new Printer2d());
        }
    }
}

This example is trivial, in the real world there’s likely to be some property one printer supports that the other does not. Imagine the 3d Printer has a SetDepth Function, which moves coordinates further down the Z order. This is useful for aesthetics when printed. Let’s see how Bob manages this change.

namespace OCP3
{
    abstract class Printer
    {
        public abstract void Print();
    }

    class Printer2d : Printer
    {
        override public void Print() { } 
    }

    class Printer3d : Printer
    {
        override public void Print() { }
        public void SetDepth(int depth) { }
    }

    class MyGraph
    {
        public MyGraph() { ; }
        public void Print(Printer anyPrinter)
        {
            if (anyPrinter is Printer3d)
            {
                Printer3d printer3d = (Printer3d)anyPrinter;
                printer3d.SetDepth(2);
            }
            anyPrinter.Print();
        }
    }

    class TestClass
    {
        public void Test()
        {
            MyGraph graph = new MyGraph();
            graph.Print(new Printer3d());
            graph.Print(new Printer2d());
        }
    }
}

Well, Bob’s implementation is not wrong. If that is the only code in the whole system dealing with printers and graphs it will be fine to leave it alone and move on. But if we know we will be adding or removing a printer, especially the 3d printer, this code is too rigid. If you can avoid branching logic based on type, your code will be more closed to change and open to extension.
Let’s imagine there’s a 4d Printer, which also supports the depth method.

namespace OCP4
{
    abstract class Printer
    {
        public abstract void Print();
    }

    class Printer2d : Printer
    {
        override public void Print() { }
    }

    class Printer3d : Printer
    {
        override public void Print() { }
        public void SetDepth(int depth) { }
    }

    class Printer4d : Printer
    {
        override public void Print() { }
        public void SetDepth(int depth) { }
    }

    class MyGraph
    {
        public MyGraph() { ; }
        public void Print(Printer anyPrinter)
        {
            if (anyPrinter is Printer3d)
            {
                Printer3d printer3d = (Printer3d)anyPrinter;
                printer3d.SetDepth(2);
            }
            if (anyPrinter is Printer4d)
            {
                Printer4d printer4d = (Printer4d)anyPrinter;
                printer4d.SetDepth(2);
            }
            anyPrinter.Print();
        }
    }

    class TestClass
    {
        public void Test()
        {
            MyGraph graph = new MyGraph();
            graph.Print(new Printer3d());
            graph.Print(new Printer2d());
            graph.Print(new Printer4d());
        }
    }
}

We are now building logic into every graph, for every printer. Madness. So how do we close the MyGraph.Print method from the types of changes we’ve experienced so far? The OCP does not dictate how you achieve this, but just that you should try and achieve it.

Use an interface to represent capability
๐Ÿ™‚ Keeps base class clean
๐Ÿ™‚ Only derivations expose the functionality if truly supported
๐Ÿ™‚ Callers can’t use methods without casting the interface. This means it is less error prone for consumers who might not realize this is not supported on all printers.
๐Ÿ˜ฆ Unless you maintain good documentation in your base class comments, a new derivation doesn’t know what interfaces could be implemented.
My View: The preferred approach. It keeps the code to a minimum, makes it obvious that this is specific functionality only a set of classes support and is unambiguous.

namespace OCPUsingInterfaces
{
    abstract class Printer
    {
        public abstract void Print(); 
    }

    interface IPrinterDepthCapable
    {
        void SetDepth(int depth);
    }

    class Printer2d : Printer
    {
        public override void Print() { }
    }

    class Printer3d : Printer, IPrinterDepthCapable
    {
        public override void Print() { }

        void IPrinterDepthCapable.SetDepth(int depth) { }
    }

    class Printer4d : Printer, IPrinterDepthCapable
    {
        public override void Print() { }

        void IPrinterDepthCapable.SetDepth(int depth) { }
    }

    class MyGraph
    {
        public MyGraph() { ; }
        public void Print(Printer anyPrinter)
        {
            // it's obvious SetDepth is only supported by some classes
            if( anyPrinter is IPrinterDepthCapable)
                ((IPrinterDepthCapable)anyPrinter).SetDepth(2);

            anyPrinter.Print();
        }
    }

    class TestClass
    {
        public void Test()
        {
            MyGraph graph = new MyGraph();
            graph.Print(new Printer3d());
            graph.Print(new Printer2d());
            graph.Print(new Printer4d());
        }
    }
}

๐Ÿ™‚ In the example above we have demonstrated how to protected the Print() function from addition or removal of similar printer classes.
๐Ÿ™‚ We’ve also protected the Print() function from classes which do or do not implement SetDepth() without complicating the base class interface.
๐Ÿ™‚ No concrete implementation of a Graph knows about any concrete implementation of a Printer class.
๐Ÿ˜ฆ We haven’t truly protected against adding another capability or another function to theย IPrinterDepthCapable interface. But it’s not possible to protect classes against every change.
๐Ÿ˜ฆ We still have to search across all Graph classes to determine how they may be adapted if an interface is added.

To improve closure of the Print() function further we could apply the Single Responsibility Principle and move the printing logic into a MyGraphPrinter class. This can just move the problem elsewhere, but would ensure that a change to printing logic only affected classes like MyGraphPrinter only, and not MyGraph.

Example 2- Where to store relational logic
Sometimes features force developers to develop meta-knowledge about concretions, to rank one class over another. For a quality such as speed of printing, the implementation is trivial – a property on each printer using an abstract base class member will do. The code will remain nicely closed.

But if the quality is relative to other concretions it is much more likely that a poor implementation will result. For example, how do we choose the most capable printer installed on the system? We know a 4d Printer is more capable than a 3d printer and that is more capable than 2d printer. Where does that code live?

It’s been a few weeks since Bob has shown his mentor Dave some code, let’s see if Bob has some new tricks.

namespace OCPMostDesirablePrinter
{
    abstract class Printer : IComparable
    {
        public abstract void Print();
        public abstract bool IsInstalled { get; }
        public abstract int CompareTo(object o);
    }

    interface IPrinterDepthCapability
    {
        void SetDepth(int depth);
    }

    class Printer2d : Printer
    {
        public override void Print() { }
        public override bool IsInstalled { get { return true; } }// hard coded for test purposes
        public override int CompareTo(object o)
        {
            if (o is Printer3d)
                return 1; // 2d should come after 3d
            if (o is Printer4d)
                return 1; // 2d should come after 4d
            return 0;
        }
    }

    class Printer3d : Printer, IPrinterDepthCapability
    {
        public override void Print() { }

        void IPrinterDepthCapability.SetDepth(int depth) { }
        public override bool IsInstalled { get { return true; } }// hard coded for test purposes
        public override int CompareTo(object o)
        {
            if (o is Printer2d)
                return -1; // 3d should come before 2d
            if (o is Printer4d)
                return 1; // 3d should come after 4d 
            return 0;
        }
    }

    class Printer4d : Printer, IPrinterDepthCapability
    {
        public override void Print() { }

        void IPrinterDepthCapability.SetDepth(int depth) { }
        public override bool IsInstalled { get { return false; } } // hard coded for test purposes
        public override int CompareTo(object o)
        {
            if (o is Printer2d)
                return -1; // 4d should come before 2d
            if (o is Printer3d)
                return -1; // 4d should come before 3d
            return 0;
        }
    }

    class MyGraph
    {
        public MyGraph() { ; }
        public void Print(Printer anyPrinter)
        {
            // it's obvious SetDepth might not work for all
            if (anyPrinter is IPrinterDepthCapability)
                ((IPrinterDepthCapability)anyPrinter).SetDepth(2);

            anyPrinter.Print();
        }
    }

    class TestClass
    {
        public void Test()
        {
            List allPrinters = new List(){ new Printer2d(), new Printer3d(), new Printer4d() };

            List installedPrinters = allPrinters.Where(x => x.IsInstalled).ToList();

            if (installedPrinters.Count > 0)
            {
                installedPrinters.Sort();

                Printer bestPrinter = installedPrinters[0];

                MyGraph graph = new MyGraph();
                graph.Print(bestPrinter);
                graph.Print(bestPrinter);
                graph.Print(bestPrinter);
            }            
        }
    }
}

Wow. Bob has learned some new tricks. List initialization, lambda expressions. He’s getting some snazzy skills there. However…
๐Ÿ™‚ Using sort was quite clever, but what does that really mean to a consumer, they might expect it sorts by paper available, speed, name of printer.
๐Ÿ˜ฆ Bob has introduced a terrible burden on each concrete printer class – every printer class has knowledge of every printer class.
๐Ÿ˜ฆ The CompareTo() indicates what should come first in the list, not really what’s ‘greater’ than each other
๐Ÿ˜ฆ What does it really mean to compare a printer against each other – it doesn’t even make sense.
๐Ÿ˜ฆ It feels like we have copied and pasted code three times. Something is wrong!

Dave resorts to more head slapping, and nearly knocks Bob unconscious. It’s a brutal world.
Here’s what they came up with to make sure that client code was not exposed to the printers concrete classes, the printers do not know about each other, and that the code could be read easily. This code is closed to change and open to extension.

namespace OCPMostDesirablePrinterFactory
{
    abstract class Printer
    {
        public abstract void Print();
        public abstract bool IsInstalled { get; }
    }

    interface IPrinterDepthCapability
    {
        void SetDepth(int depth);
    }

    class Printer2d : Printer
    {
        public override void Print() { }
        public override bool IsInstalled { get { return true; } }// hard coded for test purposes
    }

    class Printer3d : Printer, IPrinterDepthCapability
    {
        public override void Print() { }

        void IPrinterDepthCapability.SetDepth(int depth) { }
        public override bool IsInstalled { get { return true; } }// hard coded for test purposes
    }

    class Printer4d : Printer, IPrinterDepthCapability
    {
        public override void Print() { }

        void IPrinterDepthCapability.SetDepth(int depth) { }
        public override bool IsInstalled { get { return false; } } // hard coded for test purposes
    }

    class MyGraph
    {
        public MyGraph() { ; }
        public void Print(Printer anyPrinter)
        {
            // it's obvious SetDepth might not work for all
            if (anyPrinter is IPrinterDepthCapability)
                ((IPrinterDepthCapability)anyPrinter).SetDepth(2);

            anyPrinter.Print();
        }
    }

    class PrinterFactory
    {
        public static Printer FindMostDesirablePrinterInstalled()
        {
            // keep this list in order of most desirable first
            List allPrinters = new List(){
                new Printer4d(),
                new Printer3d(),
                new Printer2d() };

            return allPrinters.FirstOrDefault(x => x.IsInstalled);
        }
    }

    class TestClass
    {
        public void Test()
        {
            Printer bestPrinter = PrinterFactory.FindMostDesirablePrinterInstalled();

            if (bestPrinter!=null)
            {
                MyGraph graph = new MyGraph();
                graph.Print(bestPrinter);
                graph.Print(bestPrinter);
                graph.Print(bestPrinter);
            }
        }            
    }
}

๐Ÿ™‚ The code is much much much cleaner.

๐Ÿ™‚ The ordering logic of desirable printers is trivial to maintain.

๐Ÿ™‚ The the logic is easy to locate.

When should I retrospectively apply the principle to a class?

There will always be a change that a design is not closed to, thus the application of the OCP needs to be strategic. The designer must predict the most probable changes the system will endure, or react to them as they happen. The time taken to refactor and design code to meet OCP can be expensive. Over-enthusiastic application of OCP can increase maintenance costs through increased code complexity. Thus, choosing the proportionate amount of effort to expend is important.

Robert C. Martin talks about taking ‘the bullet’ when the first change request comes in – letting that change request show you the likely pattern of change. When a bullet comes in, make your code bullet proof to those kinds of bullets.

The Middleton test โ€“ to maximise ROI:

  • If you are changing some code, try and protect the code from this type of change if you can.
  • If you have doubts about the forthcoming changes materializing, do not do the work now.

References

Full Article –ย  Robert C. Martin

Wikipedia

Further examples for Example 1 – Common OCP application

The Interface pattern is not the only way of solving the problem in Example 1. Languages such as C# and C++ give us many different ropes to hang ourselves with. Each has pros and cons, and some are just plain stupid to even try.

Inject an intermediate abstract class
๐Ÿ™‚ can test for the capability rather than the object.
๐Ÿ™‚ It is now immune (Closed) to other printers which may or may not implement this capability.
๐Ÿ˜ฆ You have to choose unique method names for each abstract class you implement.
๐Ÿ˜ฆ If we add another capability to printers, where does it sit in the inheritance hierarchy?
๐Ÿ˜ฆ This is an abuse of inheritance – Is this really a ‘type of’ graph? Your colleagues won’t respect you!

My view: I would not recommend this approach.

namespace OCPIntermediateAbstractBaseClass
{
    abstract class Printer
    {
        public abstract void Print();
    }

    abstract class PrinterWhichSupportsDepth : Printer
    {
        public abstract void SetDepth(int depth);
    }

    class Printer2d : Printer
    {
        override public void Print() { }
    }

    class Printer3d : PrinterWhichSupportsDepth
    {
        override public void Print() { }
        override public void SetDepth(int depth) { }
    }

    class Printer4d : PrinterWhichSupportsDepth
    {
        override public void Print() { }
        override public void SetDepth(int depth) { }
    }

    class MyGraph
    {
        public MyGraph() { ; }
        public void Print(Printer anyPrinter)
        {
            // much simpler than before
            if (anyPrinter is PrinterWhichSupportsDepth)
            {
                PrinterWhichSupportsDepth printerDepth = (PrinterWhichSupportsDepth)anyPrinter;
                printerDepth.SetDepth(2);
            }
            anyPrinter.Print();
        }
    }

    class TestClass
    {
        public void Test()
        {
            MyGraph graph = new MyGraph();
            graph.Print(new Printer3d());
            graph.Print(new Printer2d());
            graph.Print(new Printer4d());
        }
    }
}

Use multiple inheritance
๐Ÿ˜ฆ This isn’t supported in C#, but is in C++.
๐Ÿ˜ Is not a terrible approach in C++ (non .NET) if you do not want to introduce COM Interfaces. It would require a spin doctor to call these ‘is a’ relationships.

Provide a SetDepth method in the base class which is ignored by some.
๐Ÿ™‚ Can call without worry.
๐Ÿ™‚ Minimal code
๐Ÿ™‚ Smaller public interface
๐Ÿ˜ฆ Callers might expect the method to do something
๐Ÿ˜ฆ Every derivation has to implement something they may never use. If you do not make it abstract in the base class, derivatives have to guess that they should implement the function.
My view: Not so terrible, and quite a common thing you’ll see – but that doesn’t make it right.

namespace OCPSetDepthInBaseClass
{
    abstract class Printer
    {
        public abstract void Print();
        public abstract void SetDepth(int depth);
    }

    class Printer2d : Printer
    {
        override public void Print() { }
        // really does nothing, I have to implement this because of another printer ๐Ÿ˜ฆ
        override public void SetDepth(int depth) { }
    }

    class Printer3d : Printer
    {
        override public void Print() { }
        override public void SetDepth(int depth) { }
    }

    class Printer4d : Printer
    {
        override public void Print() { }
        override public void SetDepth(int depth) { }
    }

    class MyGraph
    {
        public MyGraph() { ; }
        public void Print(Printer anyPrinter)
        {
            // MUCH SIMPLER!!!
            anyPrinter.SetDepth(2);
            anyPrinter.Print();
        }
    }

    class TestClass
    {
        public void Test()
        {
            MyGraph graph = new MyGraph();
            graph.Print(new Printer3d());
            graph.Print(new Printer2d());
            graph.Print(new Printer4d());
        }
    }
}

Introduce a capability method, and make SetDepth non abstract
๐Ÿ™‚ We know that when we call SetDepth that it will do something.
๐Ÿ™‚ Derivatives do not have to implement SetDepth() when it makes no sense.
๐Ÿ™‚ Derivatives have to implement IsSetDepthSupported()
๐Ÿ™‚ Code is simple and predictable
๐Ÿ˜ฆ Two methods are exposed in the base class
๐Ÿ˜ฆ Public interface gets bigger per capability
๐Ÿ˜ฆ Callers have to know to call IsSetDepthSupported() before SetDepth() – error prone.
My View: Maybe suitable for smaller sets of functionality, the benefit is that it does make the capability appear optional and available without hiding capabilities as interfaces.

namespace OCPIsDepthSupportedCapability
{
    abstract class Printer
    {
        public abstract void Print();

        public abstract bool IsDepthSupported();
        public virtual void SetDepth(int depth) {  /* do nothing by default*/ }
    }

    class Printer2d : Printer
    {
        public override void Print() { }
        public override bool IsDepthSupported() { return false; }

        // don't have to implement SetDepth
    }

    class Printer3d : Printer
    {
        public override void Print() { }

        public override bool IsDepthSupported() { return true; }
        public override void SetDepth(int depth) { }
    }

    class Printer4d : Printer
    {
        public override void Print() { }

        public override bool IsDepthSupported() { return true; }
        public override void SetDepth(int depth) { }
    }

    class MyGraph
    {
        public MyGraph() { ; }
        public void Print(Printer anyPrinter)
        {
            // it's obvious SetDepth might not work for all
            if( anyPrinter.IsDepthSupported() )
                anyPrinter.SetDepth(2);

            anyPrinter.Print();
        }
    }

    class TestClass
    {
        public void Test()
        {
            MyGraph graph = new MyGraph();
            graph.Print(new Printer3d());
            graph.Print(new Printer2d());
            graph.Print(new Printer4d());
        }
    }
}

(C) Giles Middletonย  2013

Advertisements

2 thoughts on “Pragmatic SOLID Part 2 – The Open/Closed Principle

    • That’s a great question. And no, it’s not entirely clear to me either – It may have been clear when I wrote this article 2 years ago, but not right now!

      I could make some hooey reason up on the spot, but I’d rather admit when I’m not so confident than mislead others.

      I do however remember a time before interfaces. Interface constructs were introduced into the major languages to provide a more explicit and strict contract of behaviour, and also to firm up the trend of using COM. COM required a strict set of types that could be marshalled between different binary formats/languages and provided a set of rules (like the transmutation of interfaces, reference counting, IUnknown). If you need that behaviour, then interfaces makes a lot of sense.

      So if you’re already in a code base where interfaces or COM is used, then you’ll have to have a good reason to move away from it.

      I’m also guessing that tools like Moq won’t work well with abstract classes (but I could be wrong if it’s just another ‘type’).

      Do you have any thoughts yourself?

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s