Previous post in this series:
SOLID – SRP – Ignoring the rules on the paint can

SOFTWARE ENTITIES (CLASSES, MODULES, FUNCTIONS, ETC.) SHOULD BE OPEN FOR EXTENSION, BUT CLOSED FOR MODIFICATION.

When a single change to a program results in a cascade of changes to dependent modules, that program exhibits the undesirable attributes that we have to come to associate with “bad” design. The program becomes fragile, rigid, unpredictable and unreusable. The open-closed principle attacks this in a very straightforward way. It says that you should design modules that never change. When requirements change, you extend the behavior of such modules by adding new code, not by changing old code that already works.

The Open-Closed Principle, Robert C. Martin

Again, like all the SOLID principles, this is straight-forward in its definition but more complex in its implementation. Let’s look at each of the areas this relates to so we can get a more through understanding of the issues.

Abstraction

First off we have to abstract any specific implementations away from the base class into the child classes.

Example 1: The “Shape” Abstraction done wrong (based loosely on the source reference)

public enum ShapeType { Circle, Square }
public abstract class Shape
{
	ShapeType _type;
	public ShapeType Type { get { return _type; } }

	protected Shape(ShapeType Type)
	{
		this._type = Type;
	}
}

public class Circle : Shape
{
	double _radius;
	public double Radius { get { return _radius; } }

	Point _center;
	public Point Center { get { return _center; } }

	public Circle(double Radius, Point Center)
		: base(ShapeType.Circle)
	{
		this._radius = Radius;
		this._center = Center;
	}
}

public class Square : Shape
{
	double _side;
	public double Side { get { return _side; } }

	Point _topLeft;
	public Point TopLeft { get { return _topLeft; } }

	public Square(double Side, Point TopLeft)
		: base(ShapeType.Square)
	{
		this._side = Side;
		this._topLeft = TopLeft;
	}
}

public class Canvas
{
	public void DrawAllShapes(List<Shape> Shapes)
	{
		foreach (Shape shape in Shapes)
		{
			switch (shape.Type)
			{
				case ShapeType.Circle:
					DrawCircle((Circle)shape);
					break;
				case ShapeType.Square:
					DrawSquare((Square)shape);
					break;
			}
		}
	}

	void DrawCircle(Circle Circle) { throw new NotImplementedException(); }
	void DrawSquare(Square Square) { throw new NotImplementedException(); }
}

The most obvious problem here is that you can’t extend Shape without making major modifications to Canvas to support other shape types. Not only would you have to make changes to “DrawAllShapes” but you would have to add new methods for each new type to draw it. Remember too that this is a fairly contrived case. In real code you would have switch cases all over the place trying to deal with each ShapeType that would end up being a serious case of spaghetti code. You shouldn’t have to make changes to dependent modules anytime you want to extend an existing class. In the same vein is the ShapeType, where anytime we add a new shape we would have to add a new ShapeType.

Example 2: OOD solution to Square/Circle problem. (based loosely on the source reference)

public abstract class Shape
{
	abstract public void Draw();
}

public class Circle : Shape
{
	double _radius;
	public double Radius { get { return _radius; } }

	Point _center;
	public Point Center { get { return _center; } }

	public Circle(double Radius, Point Center)
	{
		this._radius = Radius;
		this._center = Center;
	}

	public override void Draw() {/* Draw ourself */}
}

public class Square : Shape
{
	double _side;
	public double Side { get { return _side; } }

	Point _topLeft;
	public Point TopLeft { get { return _topLeft; } }

	public Square(double Side, Point TopLeft)
	{
		this._side = Side;
		this._topLeft = TopLeft;
	}

	public override void Draw() {/* Draw ourself */}
}

public class Canvas
{
	public void DrawAllShapes(List<Shape> Shapes)
	{
		foreach (Shape shape in Shapes)
		{
			shape.Draw();
		}
	}
}

We’ve removed the need to define a shape type and abstracted “Draw” away from canvas and into each class. This means that each class can be extended with its own draw method without having to modify DrawAllShapes. There a problem here however. What if you may need to define a property/member variable or method in such a way that is should be done as an abstract in the base class? Well, you suck it up, make changes to your unit tests to cover this new case and modify any and all classes affected by this change assuming you can. When you do this you risk braking functionality any clients are dependent on. Case in point, my sample model from my series on the TPL and MVVM. When I initially wrote my TPL sampler there was no need for a “ImageRequired” property as all samples I had envisioned used an image and there was no reason not to reset and reload the image before each sample was run. The problem arose that reloading the image each time was a resource heavy and IO blocking operation, especially on large files and I quickly came to samples where they didn’t need an image but instead did other operations to show how their functionality worked.

The first solution some developers might think to do is simply modify the top-level call where it resets and reloads the image with a case/switch such that if it is one type of class, reload the image, if it is another then don’t reload it. But this is very bad and actually violates another part of this principle I’ll discuss later. The solution was to add an abstract ImageRequired property to the base model and then go back to each of my models and add in this property so that it could be used without having to modify the code where the image for the samples was reset and reloaded.

Referring back to the source material it goes into further ways of achieving closure that I’ll leave as an exercise to the reader. The primary sample it contains is, “What if we have to draw shapes in a paticular order on our canvas?” and goes into how to achieve this. The end result is that, probably the best way to do this is order the classes based on a “OrderTable” where the classes are ordered by how they should be drawn. This way type is still abstracted away from the classes themselves and dependent modules don’t have to change but can rely on the “OrderTable”. OrderTable is still dependent on the class types but not on the classes themselves and so can operated independently. I know it sounds like a lot of handwaving to try and make this all work for functionality like this but having a single place that defines stuff like this that is independent can make maintenance a lot easier.

Heuristics and Conventions

There are a lot of “Good Dev Practices” that are done just to facilitate the OCP and Uncle Bob mentions a few of the more important ones. I’ll touch briefly on these but I would recommend reading the source material for a more in-depth coverage of each one.

Make all Member Variables Private

This where the argument that member variables should only be accessed via getters/setters comes from. This way the class that implements the member variable has control over it. We don’t want the potential that the meaning of what a variable is changes by the classes that use it external to the class that implements it. The source material provides an example to this end. In .NET this is made rather simple for us by using properties, though using “PropertyName{get;set;}” just exposes the member variable as if it were public. I argued in a post from over 6 years ago that we should strive for immutability. If you look at the classes in the Shape example above you will see that the classes are immutable with private member variables and public getters. It’s important to note as I’ve mentioned that just implementing properties with the default {get;set;} violates this principle as it just exposes the private member variable with no thought to how it is used or accessed.

No Global Variables — Ever

I have seen pretty consistently where the meaning of global variables changes over time. This means that early code uses a global variable in one manner but future code uses it in another manner. Not only that but we run into the problem where one class or module could change the variable in a way that is unexpected from other modules using it. It violates the OCP because by exposing the global we allow for it to be changed over time. This means that code that relies on the global in one manner will have to be changed if the meaning of the global changes over time. Uncle Bob does mention a case for Global Variables. Where the definition of those variables can’t change, like in cout and cin (or in C#, Console.Write() or Console.Read()). These are static global instances where they perform a functionality but we can’t change them.

The most consistent violation of this is the use of the Singleton pattern where there is but a single global reference to a resource (like Console). This is done intentionally with the idea that control over the resource is needed and more important (or more convenient). In the case of a singleton, the object should not be changed and therefore adheres to the OCP. I’ll cover the good and bad of Singleton when I do coverage on patterns.

RTTI is Dangerous

RTTI is run time type identification (RTTI). In C# it is the use of the “is” keyword or using “as” and checking for null.

Example 3: Using RTTI in Example 1 for our Canvas.DrawAllShapes()

public class Canvas
{
	public void DrawAllShapes(List<Shape> Shapes)
	{
		foreach (Shape shape in Shapes)
		{
			if (shape is Circle)
			{
				DrawCircle((Circle)shape);
			}
			else if (shape is Square)
			{
				DrawSquare((Square)shape);
			}
		}
	}
	
	void DrawCircle(Circle Circle) { throw new NotImplementedException(); }
	void DrawSquare(Square Square) { throw new NotImplementedException(); }
}

This is great because we eliminate the dependency on ShapeType. We’ve cleaned up the code a bit and made maintenance easier. So why is this bad? Because you will still have to modify Canvas for any new shapes that need to be added. Example 1 was an example of what not to do for a reason and by using RTTI all we’ve done is make it look like it should be okay to implement things poorly. So when is it okay to use RTTI?

Exmaple 4: Using RTTI in Example 2 where it does not violate the OCP (based loosely on the source reference)

public class Canvas
{
	public void DrawAllShapes(List<Shape> Shapes)
	{
		foreach (Shape shape in Shapes)
		{
			shape.Draw();
		}
	}

	public void DrawSquares(List<Shape> Shapes)
	{
		foreach (Shape shape in Shapes)
		{
			if(shape is Square)
				shape.Draw();
		}
	}
}

Here we’ve added “DrawSquares”. All we’re doing is drawing squares. It makes sense to verify that we have a square before we try and draw it. This doesn’t violate the OCP because even if the new shapes are added we don’t have to go back and modify “DrawSquares”. It will always do just what it says it will do.

That’s it on the OCP. As with all the principles there are times where you have to balance adhering to the rules with other considerations but when in doubt it’s generally better to follow the rules. Up next is the Liskov Substitution Principle.

Thanks,
Brian