Skip to content

The Amazing Adventures of a Sexy Software Engineer Posts

The Open-Closed Principle

Today we are going to go through one of the five tools that every Object Oriented Designer, or just anyone not wanting his code to become a loaded gun pointing at her head, should have in her toolset: the Open/Closed Principle.

The Open/Closed Principle is one of the most mentioned, but yet most misunderstood, of the five SOLID principles. I am sure you are fed up with reading the definition all around the interwebs, but here it is anyway:

Software entities (classes, modules, functions, etc.) should be open for extension, but closed for modification.

Say what!?

Open for extension, but closed for modification. Neat. But, what does that mean? Let’s rephrase the Principle a little bit:

We should be able to modify the behaviour of a piece of code without changing the actual code that implements it.

In other words, we should have magic powers, or superpowers, or be able to bend the space-time continuum.

Having superpowers would be neat. Can I have some?

Sure, it would be neat. But it is definitely out of the scope of this post.

Let me rephrase. How can my code be open for extension but closed for modification?

Let’s go through a real life example. Real life, like in “code extracted from an actual codebase my team has to maintain”. Let’s pretend that the fact that I personally wrote the offending piece of code does not matter.

The problem

I mostly work with OVPs (that’s Online Video Platforms). I build those pesky apps that some TV providers deliver to their subscribers, and allow said subscribers to watch TV on their devices.

So, I needed to display a Program Guide. An Electronic Program Guide, actually.

An Electronic Program Guide is a list of what is going to be broadcasted during a particular range of dates/times. Let’s pretend this is what I need to display:

Screen Shot 2015-04-03 at 8.29.42 pm

Bear with me on this one, please.

As you can see, the list needs to display Movies, TV Shows, and Live events. When the user taps a particular Program, the app needs to transition to a “details” view, which is, obviously, different (different content and different layout, and even different business logic) according to the type of Program the user selected.

For historical reasons, and some practical reasons, the model object that was provided with in order to build that listing looks similar to this:

Yes, you can argue than all my troubles will end if I looked at the right abstraction, and if I did not need to rely on that “type” property. And I would agree with you: remember, there are historical reasons behind this. Anyway, all this “look at the right abstraction” thing is probably matter of another post.

But, on the other hand, relaying on the right abstraction does not change the fact that I need to navigate to different views according to the type of content I need to display.

The simplest possible solution.

I am a big fan of keeping things simple. When you keep things simple, code smells, bad designs, and bad solutions just popup in front of you when you look at the code, jumping out of the monitor, waving their arms, screaming “refactor me, refactor me!”

So, the simplest implementation that would get the job done would be:

Riiiiiiiight, a switch.

Switch is bad

No matter what they tell you, no matter what you see, switch clauses are usually a code smell. And in this particular case, ermahgawd, it is just bad. I wrote that code, and I still want to punch myself in the face overtime I see it. But why?

Well, this will not escalate well. First, and most obvious, Program is poorly designed, it is the wrong abstraction, and to workaround that bad design, it exposes a type to help identify what it actually represents. That is clearly meant to change.

But, the worst is about to come. What if I need to deal with a new type of program? I would need to update that switch. There is no way to extend this code that does not imply editing the navigateToDetails function.

This code is not open for extension and closed for modification.

Again, the simplest possible solution clearly exposes two code smells: first, Program is the wrong abstraction, or at least it is poorly designed, and second, the initial design is not easy to extend.

The better solution, open for extension and closed for modification

Let’s assume that Program is not going to change now. It will, but not at this precise moment.

If you look at the first implementation of the navigateToDetails function, you will see how all the business logic that deals with navigation is in there. That function checks the program type, and launches the command needed to navigate to the desired destination. What I want, is find a way to delay those decisions as much as possible. That way, I can deal with the problem in a generic way here.

The first step to fix my problem (remember, I want to be able to navigate to different views according to the type of Program the user selects) starts with declaring a protocol that abstracts the details of how I want to perform that navigation.

Then, I will have different implementations of that protocol. Each of those concrete implementations will deal with the details of how to navigate to the desired destination, and more importantly, they will provide a way to decide if that particular implementation is supposed to handle a Program or not.

That last part is the key of the whole refactoring. Notice how I can inject (or in less fancy words, pass as a parameter) a list of all the available implementations of the Router. My navigateToDetails function will loop those, passing them the Program. If it finds any implementation that can deal with the Program passed as parameter, it will ask the implementation found to proceed and navigate to… where? That’s the thing! The navigateToDetails function does not know where, because it doesn’t need to know.

 

Delay all decisions!

I recall reading, some time ago, that good Object Oriented Design consisted in finding ways to delay decisions. I only agree with that up to a point (I guess I have been a victim of over-engineered systems more than once), but I think it makes a good rule of thumb. If a particular part of your system knows too much about others, maybe it is time to rethink it.

But I digress. The whole point of this post was implementing a solution to a problem in a way that allows easy maintenance, and simplifies scalability.

Summary.

Object Oriented Design is all about trade-offs. Simple code usually gets the job done, but sometimes simplicity in the code brings along poor maintainability. Sometimes, ten lines of code get the work done, but sometimes you need to substitute those ten lines by a hundred lines, as long as those hundred lines provide a solid, clean and maintainable solution.

And, did I mention that unit testing the second solution is way easier?

Code samples.

You can get a couple of Swift playgrounds containing the source code to this post on Github

Leave a Comment

Code reviews are awesome, when done properly

Code reviews are awesome. Seriously, if you are not doing code reviews you should start doing them right away. There are many good reasons for doing so.

Code reviews are a learning opportunity

When done properly, code reviews are awesome learning opportunities for junior developers. There is no better way of growing as a developer, than having someone well seasoned in front of you, explaining to you, line by line, how to make your code better.

But wait! Code reviews are awesome learning opportunities for senior developers as well. You know that saying “an old dog does not learn new tricks”, don’t you? Well, there is nothing better for a senior developer than a newcomer to the craft asking all kinds of questions about a codebase. If makes you reconsider each and everyone of your decisions, and most times, it makes you realise how many things you do that you should probably not be doing. And how you do those things without any particular reason.

Code reviews help improve the code quality

This should be obvious, but apparently it is not. The main goal of a code review is being sure that the code quality is up to the team’s standards. Now, what are those standards might be a theme for another post (or a thousand posts), but the key concept here is that opening up the code for review, everybody gets to read it, discuss it, and improve it through suggestions and comments. Which leads to…

Code reviews enforce collective ownership of the code

You have probably heard it a thousand times: code should be owned by the whole team. There is no better way to make the team feel like the code is owned by everyone than making everyone actually own the code. And that happens when everyone’s suggestions and comments are incorporated into the code base. More importantly, opening up the code for review avoids what I call dark zones, chunks of code that none has actually seen, and that no one knows what they actually do or how they actually do it. Again, collective ownership FTW!

Code reviews help share knowledge about the code base

Here comes Captain Obvious! Yes, if everybody is supposed to read everybody else’s code, understand it, and being able to comment about it and suggests improvements to it, as a side effect, that means that everybody understand the whole code base. And that is awesome!

Code reviews avoid nasty surprises.

We have all been there. James did work on the new feature, and his code was complete. Until we tried to integrate Jame’s feature into the new release. That’s when we found that, apparently, James has a different definition of “code complete”. Code reviews will rise to the surface issues with the code as early as possible. If the feature is not implemented as it is supposed to, a good code review will catch that.

Code reviews motivate developers to write their best code

If you know that everyone else is going to read your code, you are going to find that extra pinch of motivation to avoid duplication, or to come up with better names for your methods and variables, or to break down that very long method into smaller methods.

Code reviews create a team’s culture

Yeah, the company culture. That thing every company on the face of earth wants to create, that thing that is supposed to act as a mesh, as a framework, that supports all the engineers, that binds them together, that makes easier for new hires to catch up and blend into their teams. A healthy system of code reviews makes wonders when it comes to create that. There is no better mentoring for a new developer that his first pull request open for review. There is no better way to make that new hire feel as he is growing as a developer than receiving continuous feedback about his code and being able to provide feedback to his colleagues about their code.

All that is cool, and everything, but how can you make sure that everybody understand code reviews as all of the above?

Well, I am very sceptic about this whole Scrum and Agile Cult. But there is something in that process that I really like: the idea that teams should be self-organised. Give a team the opportunity to figure out how and why they want to do their code reviews, and they will come up with something good. Good, I say, because it will be something they all will stand by, and thy will all abide by.

That is what my team did, And this is what we came out with (edited to remove some company-specific items)

Code reviews: rules and regulations

Rule #1

Code reviews are a learning opportunity, both for the person opening up his code for review, and the persons doing the actual review, and they should be approached as such.

Rule #2

Code reviews are done for one reason: deliver the best possible code.

Rule #3

Comments in the code reviews shall be performed with rules #1 and #2 in mind.
Corollary 1: Comments in the code reviews should be backed up by arguments and facts, not by personal preferences.
Corollary 2: Refrain from adding comments that provide no value. If a single comment can achieve the same result of 10 comments, it is better to make that one comment vs. 10 different comments in 10 different places.

Rule #4

The result of a good review should provide:
– A list of things to update. There should be a clear definition of what to update.
– A list of things that require further discussion.

Rule #5

In order of importance (descending) a review should focus on:
– Global intent of the code (e.g. does it do what it is supposed to be doing?)
– Architecture (e.g., does it follow the technical design?)
– Gaps in the unit tests (e.g., does it cover all the requirements?)
– Code readability (e.g., does it conform to code clean guidelines?)

Corollary: While code style is also an integral part of code readability, it is not considered an important aspect of the code review unless it blatantly violates the agreed upon standard.

Leave a Comment

Quote

Confidence is a valuable, but fragile, resource. If it’s not fragile, it’s no longer valuable.

Kent Beck

Leave a Comment

Hello world!

Not bad, but not exactly object oriented. Improve all the code!

That’s better. That string is nagging me, though. It should be declared as a constant. Improve all the code!

That’s better. However, there is still something wrong. I don’t like how my code expects an instance of Greeting, what if I need a different implementation of the greet method?. Improve all the code!

But wait! Now there is a hardcoded dependency in there. I expect an abstraction, but create an instance of a specific class. This clearly calls for a factory. Improve all the code!

Welcome to my blog.

60922204

Leave a Comment