Gosh, there are some weird misconceptions on what OCP and LSP and some are due to mismatch of some terminologies and confusing examples. Both principles are only the "same thing" if you implement them the same way. Patterns usually follow the principles in one way or another with few exceptions.
The differences will be explained further down but first let us take a dive into the principles themselves:
Open-Closed Principle (OCP)
According to Uncle Bob:
You should be able to extend a classes behavior, without modifying it.
Note that the word extend in this case doesn't necessarily mean that you should subclass the actual class that needs the new behavior. See how I mentioned at first mismatch of terminology? The keyword extend
only means subclassing in Java, but the principles are older than Java.
The original came from Bertrand Meyer in 1988:
Software entities (classes, modules, functions, etc.) should be open for extension, but closed for modification.
Here it is much clearer that the principle is applied to software entities. A bad example would be override the software entity as you're modifying the code completely instead of providing some point of extension. The behavior of the software entity itself should be extensible and a good example of this is implementation of the Strategy-pattern (because it is the easiest to show of the GoF-patterns bunch IMHO):
// Context is closed for modifications. Meaning you are
// not supposed to change the code here.
public class Context {
// Context is however open for extension through
// this private field
private IBehavior behavior;
// The context calls the behavior in this public
// method. If you want to change this you need
// to implement it in the IBehavior object
public void doStuff() {
if (this.behavior != null)
this.behavior.doStuff();
}
// You can dynamically set a new behavior at will
public void setBehavior(IBehavior behavior) {
this.behavior = behavior;
}
}
// The extension point looks like this and can be
// subclassed/implemented
public interface IBehavior {
public void doStuff();
}
In the example above the Context
is locked for further modifications. Most programmers would probably want to subclass the class in order to extend it but here we don't because it assumes it's behavior can be changed through anything that implements the IBehavior
interface.
I.e. the context class is closed for modification but open for extension. It actually follows another basic principle because we're putting the behavior with object composition instead of inheritance:
"Favor 'object composition' over 'class inheritance'." (Gang of Four 1995:20)
I'll let the reader read up on that principle as it is outside the scope of this question. To continue with the example, say we have the following implementations of the IBehavior interface:
public class HelloWorldBehavior implements IBehavior {
public void doStuff() {
System.println("Hello world!");
}
}
public class GoodByeBehavior implements IBehavior {
public void doStuff() {
System.out.println("Good bye cruel world!");
}
}
Using this pattern we can modify the behavior of the context at runtime, through the setBehavior
method as extension point.
// in your main method
Context c = new Context();
c.setBehavior(new HelloWorldBehavior());
c.doStuff();
// prints out "Hello world!"
c.setBehavior(new GoodByeBehavior());
c.doStuff();
// prints out "Good bye cruel world!"
So whenever you want to extend the "closed" context class, do it by subclassing it's "open" collaborating dependency. This is clearly not the same thing as subclassing the context itself yet it is OCP. LSP makes no mention about this either.
Extending with Mixins Instead of Inheritance
There are other ways to do OCP other than subclassing. One way is to keep your classes open for extension through the use of mixins. This is useful e.g. in languages that are prototype-based rather than class-based. The idea is to amend a dynamic object with more methods or attributes as needed, in other words objects that blends or "mixes in" with other objects.
Here is a javascript example of a mixin that renders a simple HTML template for anchors:
// The mixin, provides a template for anchor HTML elements, i.e. <a>
var LinkMixin = {
render: function() {
return '<a href="' + this.link +'">'
+ this.content
+ '</a>;
}
}
// Constructor for a youtube link
var YoutubeLink = function(content, youtubeId) {
this.content = content;
this.setLink(this.youtubeId);
};
// Methods are added to the prototype
YoutubeLink.prototype = {
setLink: function(youtubeid) {
this.link = 'http://www.youtube.com/watch?v=' + youtubeid;
}
};
// Extend YoutubeLink prototype with the LinkMixin using
// underscore/lodash extend
_.extend(YoutubeLink.protoype, LinkMixin);
// When used:
var ytLink = new YoutubeLink("Cool Movie!", "idOaZpX8lnA");
console.log(ytLink.render());
// will output:
// <a href="http://www.youtube.com/watch?=vidOaZpX8lnA">Cool Movie!</a>
The idea is to extend the objects dynamically and the advantage of this is that objects may share methods even if they are in completely different domains. In the above case you can easily create other kinds of html anchors by extending your specific implementation with the LinkMixin
.
In terms of OCP, the "mixins" are extensions. In the example above the YoutubeLink
is our software entity that is closed for modification, but open for extensions through the use of mixins. The object hierarchy is flattened out which makes it impossible to check for types. However this is not really a bad thing, and I'll explain in further down that checking for types is generally a bad idea and breaks the idea with polymorphism.
Note that it is possible to do multiple inheritance with this method as most extend
implementations can mix-in multiple objects:
_.extend(MyClass, Mixin1, Mixin2 /* [, ...] */);
The only thing you need to keep in mind is to not collide the names, i.e. mixins happen to define the same name of some attributes or methods as they will be overridden. In my humble experience this is a non-issue and if it does happen it is an indication of flawed design.
Liskov's Substitution Principle (LSP)
Uncle Bob defines it simply by:
Derived classes must be substitutable for their base classes.
This principle is old, in fact Uncle Bob's definition doesn't differentiate the principles as that makes LSP still closely related to OCP by the fact that, in the above Strategy example, the same supertype is used (IBehavior
). So lets look at it's original definition by Barbara Liskov and see if we can find out something else about this principle that looks like a mathematical theorem:
What is wanted here is something like the following substitution property: 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 behavior of P
is unchanged when o1
is substituted for o2
then S
is a subtype of T
.
Lets shrug on this for a while, notice as it doesn't mention classes at all. In JavaScript you can actually follow LSP even though it is not explicitly class-based. If your program has a list of at least a couple of JavaScript objects that:
- needs to be computed the same way,
- have the same behavior, and
- are otherwise in some way completely different
...then the objects are regarded as having the same "type" and it doesn't really matter for the program. This is essentially polymorphism. In generic sense; you shouldn't need to know the actual subtype if you're using it's interface. OCP does not say anything explicit about this. It also actually pinpoints a design mistake most novice programmers do:
Whenever you're feeling the urge to check the subtype of an object, you're most likely doing it WRONG.
Okay, so it might not be wrong all the time but if you have the urge to do some type checking with instanceof
or enums, you might be doing the program a bit more convoluted for yourself than it needs to be. But this is not always the case; quick and dirty hacks to get things working is an okay concession to make in my mind if the solution is small enough, and if you practice merciless refactoring, it may get improved once changes demand it.
There are ways around this "design mistake", depending on the actual problem:
- The super class is not calling the prerequisites, forcing the caller to do so instead.
- The super class is missing a generic method that the caller needs.
Both of these are common code design "mistakes". There are a couple of different refactorings you can do, such as pull-up method, or refactor to a pattern such the Visitor pattern.
I actually like the Visitor pattern a lot as it can take care of large if-statement spaghetti and it is simpler to implement than what you'd think on existing code. Say we have the following context:
public class Context {
public void doStuff(string query) {
// outcome no. 1
if (query.Equals("Hello")) {
System.out.println("Hello world!");
}
// outcome no. 2
else if (query.Equals("Bye")) {
System.out.println("Good bye cruel world!");
}
// a change request may require another outcome...
}
}
// usage:
Context c = new Context();
c.doStuff("Hello");
// prints "Hello world"
c.doStuff("Bye");
// prints "Bye"
The outcomes of the if-statement can be translated into their own visitors as each is depending on some decision and some code to run. We can extract these like this:
public interface IVisitor {
public bool canDo(string query);
public void doStuff();
}
// outcome 1
public class HelloVisitor implements IVisitor {
public bool canDo(string query) {
return query.Equals("Hello");
}
public void doStuff() {
System.out.println("Hello World");
}
}
// outcome 2
public class ByeVisitor implements IVisitor {
public bool canDo(string query) {
return query.Equals("Bye");
}
public void doStuff() {
System.out.println("Good bye cruel world");
}
}
At this point, if the programmer did not know about the Visitor pattern, he'd instead implement the Context class to check if it is of some certain type. Because the Visitor classes have a boolean canDo
method, the implementor can use that method call to determine if it is the right object to do the job. The context class can use all visitors (and add new ones) like this:
public class Context {
private ArrayList<IVisitor> visitors = new ArrayList<IVisitor>();
public Context() {
visitors.add(new HelloVisitor());
visitors.add(new ByeVisitor());
}
// instead of if-statements, go through all visitors
// and use the canDo method to determine if the
// visitor object is the right one to "visit"
public void doStuff(string query) {
for(IVisitor visitor : visitors) {
if (visitor.canDo(query)) {
visitor.doStuff();
break;
// or return... it depends if you have logic
// after this foreach loop
}
}
}
// dynamically adds new visitors
public void addVisitor(IVisitor visitor) {
if (visitor != null)
visitors.add(visitor);
}
}
Both patterns follow OCP and LSP, however they are both pinpointing different things about them. So how does code look like if it violates one of the principles?
Violating one principle but following the other
There are ways to break one of the principles but still have the other be followed. The examples below seem contrived, for good reason, but I've actually seen these popping up in production code (and even worser):
Follows OCP but not LSP
Lets say we have the given code:
public interface IPerson {}
public class Boss implements IPerson {
public void doBossStuff() { ... }
}
public class Peon implements IPerson {
public void doPeonStuff() { ... }
}
public class Context {
public Collection<IPerson> getPersons() { ... }
}
This piece of code follows the open-closed principle. If we're calling the context's GetPersons
method, we'll get a bunch of persons all with their own implementations. That means that IPerson is closed for modification, but open for extension. However things take a dark turn when we have to use it:
// in some routine that needs to do stuff with
// a collection of IPerson:
Collection<IPerson> persons = context.getPersons();
for (IPerson person : persons) {
// now we have to check the type... :-P
if (person instanceof Boss) {
((Boss) person).doBossStuff();
}
else if (person instanceof Peon) {
((Peon) person).doPeonStuff();
}
}
You have to do type checking and type conversion! Remember how I mentioned above how type checking is a bad thing? Oh no! But fear not, as also mentioned above either do some pull-up refactoring or implement a Visitor pattern. In this case we can simply do a pull up refactoring after adding a general method:
public class Boss implements IPerson {
// we're adding this general method
public void doStuff() {
// that does the call instead
this.doBossStuff();
}
public void doBossStuff() { ... }
}
public interface IPerson {
// pulled up method from Boss
public void doStuff();
}
// do the same for Peon
The benefit now is that you don't need to know the exact type anymore, following LSP:
// in some routine that needs to do stuff with
// a collection of IPerson:
Collection<IPerson> persons = context.getPersons();
for (IPerson person : persons) {
// yay, no type checking!
person.doStuff();
}
Follows LSP but not OCP
Lets look at some code that follows LSP but not OCP, it is kind of contrived but bear with me on this one it's very subtle mistake:
public class LiskovBase {
public void doStuff() {
System.out.println("My name is Liskov");
}
}
public class LiskovSub extends LiskovBase {
public void doStuff() {
System.out.println("I'm a sub Liskov!");
}
}
public class Context {
private LiskovBase base;
// the good stuff
public void doLiskovyStuff() {
base.doStuff();
}
public void setBase(LiskovBase base) { this.base = base }
}
The code does LSP because the context can use LiskovBase without knowing the actual type. You'd think this code follows OCP as well but look closely, is the class really closed? What if the doStuff
method did more than just print out a line?
The answer if it follows OCP is simply: NO, it isn't because in this object design we're required to override the code completely with something else. This opens up the cut-and-paste can of worms as you have to copy code over from the base class to get things working. The doStuff
method sure is open for extension, but it wasn't completely closed for modification.
We can apply the Template method pattern on this. The template method pattern is so common in frameworks that you might have been using it without knowing it (e.g. java swing components, c# forms and components, etc.). Here is that one way to close the doStuff
method for modification and making sure it stays closed by marking it with java's final
keyword. That keyword prevents anyone from subclassing the class further (in C# you can use sealed
to do the same thing).
public class LiskovBase {
// this is now a template method
// the code that was duplicated
public final void doStuff() {
System.out.println(getStuffString());
}
// extension point, the code that "varies"
// in LiskovBase and it's subclasses
// called by the template method above
// we expect it to be virtual and overridden
public string getStuffString() {
return "My name is Liskov";
}
}
public class LiskovSub extends LiskovBase {
// the extension overridden
// the actual code that varied
public string getStuffString() {
return "I'm sub Liskov!";
}
}
This example follows OCP and seems silly, which it is, but imagine this scaled up with more code to handle. I keep seeing code deployed in production where subclasses completely override everything and the overridden code is mostly cut-n-pasted between implementations. It works, but as with all code duplication is also a set-up for maintenance nightmares.
Conclusion
I hope this all clears out some questions regarding OCP and LSP and the differences/similarities between them. It is easy to dismiss them as the same but the examples above should show that they aren't.
Do note that, gathering from above sample code:
OCP is about locking the working code down but still keep it open somehow with some kind of extension points.
This is to avoid code duplication by encapsulating the code that changes as with the example of Template Method pattern. It also allows for failing fast as breaking changes are painful (i.e. change one place, break it everywhere else). For the sake of maintenance the concept of encapsulating change is a good thing, because changes always happen.
LSP is about letting the user handle different objects that implement a supertype without checking what the actual type they are. This is inherently what polymorphism is about.
This principle provides an alternative to do type-checking and type-conversion, that can get out of hand as the number of types grow, and can be achieved through pull-up refactoring or applying patterns such as Visitor.
First, lets understand what was the issue:
The Staple job knows about many more methods which it doesn't use today, (e.g., the print
method) but which are available for it to use, should it want to.
However, the Job class "thinks" that the Staple class will be "a good citizen" and never use the print method at all.
There are many potential big issues here -
For some reason, the Staple job may start using the print method - by accident or intentionally.
Then down the road, either any changes to the print method may go untested,
OR, any changes to the print method will trigger a regression test in the Staple job also,
AND, any impact analysis for changes to the print job will necessarily involve impact analysis of the staple job too.
This is just the issue of Staple knowing about the print functions. Then there's the case of the Print job knowing all about stapling functions. Same problems.
Very soon, this system would reach a point where any change will require a full blown impact analysis of each module and a full blown regression test.
Another problem is that today, all jobs which can be printed can be stapled, and vice versa on this particular printer.
However, tomorrow, there could be a need to install the same firmware on a device that only prints or only staples. What then? The code already assumes that all Jobs are printable and stapleable. So any further granular breakdown / simplification of responsibilities is impossible.
In more recent terms, imagine a class called "AppleDevice" which has functions for MakePhoneCall as well as PlayMusic. Now your problem is while you can easily use this on an iPhone, you cannot use it for an iPod since the iPod cannot make phone calls.
So, the issue is not that the Job class is all-powerful. In fact, that's how it should be, so that it can act as a common link in the entire "workflow" where someone may scan a job, then print it, then staple it etc.
The problem is that the usage of all its methods is not restricted. Anyone and everyone could use and abuse any method whenever they want to, thus making the maintenance difficult.
Hence, the Dependency Injection approach of only telling users "exactly what they need to know, and nothing more" ensures that calling modules only use the code that they are meant to.
A sample implementation would look like:
interface IStapleableJob { void stapleYourself(); }
interface IPrintableJob { void printYourself(); }
class Job implements IStapleableJob, IPrintableJob {
....
}
class Staple {
public static void stapleAllJobs(ArrayList<IStapleableJob> jobs) {
for(IStapleableJob job : jobs) job.stapleYourself();
}
}
class Print {
public static void stapleAllJobs(ArrayList<IPrintableJob> jobs) {
for(IPrintableJob job : jobs) job.printYourself();
}
}
Here, even if you pass a Job object to the Staple and Print methods, they dont know that its a Job, so they cannot use any methods that they are not supposed to. Thus, when you make any changes to a module, your scope of impact analysis and regression testing is restricted. That's the problem that ISP solves.
Best Answer
Interface vis-a-vis SOLID
These are not mutually exclusive. The interface should express the nature of your business model ideally in business model terms. The SOLID principles is a Koan for maximizing Object Oriented code maintainability (and I mean "maintainability" in the broadest sense). The former supports the use and manipulation of your business model and the latter optimizes code maintenance.
Open/Closed Principle
"don't touch that!" is too simplistic an interpretation. And assuming we mean "the class" is arbitrary, and not necessarily right. Rather, OCP means that you have designed your code such that modifying it's behavior does not (should not) require you to directly modify existing, working code. Further, not touching the code in the first place is the ideal way to preserve the integrity of existing interfaces; this is a significant corollary of OCP in my view.
Finally I see OCP as an indicator of existing design quality. If I find myself cracking open classes (or methods) too often, and/or without really solid (ha, ha) reasons for doing so then this may be telling me I've got some bad design (and/or I don't know how to code OO).
Give it your best shot, we have a team of doctors standing by
If your requirements analysis tells you that you need to express the Product-Review relationship from both perspectives, then do so.
Therefore, Wolfgang, you may have a good reason for modifying those existing classes. Given the new requirements, if a Review is now a fundamental part of a Product, if every extension of Product needs Review, if doing so makes client code appropriately expressive, then integrate it into the Product.