Do you leverage the benefits of the open-closed principle?

Question

The open-closed principle (OCP) states that an object should be open for extension but closed for modification. I believe I understand it and use it in conjunction with SRP to create classes that do only one thing. And, I try to create many small methods that make it possible to extract out all the behavior controls into methods that may be extended or overridden in some subclass. Thus, I end up with classes that have many extension points, be it through: dependency injection and composition, events, delegation, etc.

Consider the following a simple, extendable class:

class PaycheckCalculator {
    // ...
    protected decimal GetOvertimeFactor() { return 2.0M; }
}

Now say, for example, that the OvertimeFactor changes to 1.5. Since the above class was designed to be extended, I can easily subclass and return a different OvertimeFactor.

But... despite the class being designed for extension and adhering to OCP, I'll modify the single method in question, rather than subclassing and overridding the method in question and then re-wiring my objects in my IoC container.

As a result I've violated part of what OCP attempts to accomplish. It feels like I'm just being lazy because the above is a bit easier. Am I misunderstanding OCP? Should I really be doing something different? Do you leverage the benefits of OCP differently?

Update: based on the answers it looks like this contrived example is a poor one for a number of different reasons. The main intent of the example was to demonstrate that the class was designed to be extended by providing methods that when overridden would alter the behavior of public methods without the need for changing internal or private code. Still, I definitely misunderstood OCP.

Answer 1

If you are modifying the base class then it is not really closed is it!

Think of the situation where you have released the library to the world. If you go and change the behavior of your base class by modifying the overtime factor to 1.5 then you have violated all the people that use your code assuming that the class was closed.

Really to make the class closed but open you should be retrieving the overtime factor from an alternative source (config file maybe) or proving a virtual method that can be overridden?

If the class was truly closed then after your change no test cases would fail (assuming you have 100% coverage with all your test cases) and I would assume that there is a test case that checks GetOvertimeFactor() == 2.0M.

Don't over Engineer

But don't take this open-close principle to the logical conclusion and have everything configurable from the start (that is over engineering). Only define the bits you currently need.

The closed principle does not preclude you from re-engineering the object. It just pre-cludes you from changing the currently defined public interface to your object (protected members are part of the public interface). You can still add more functionality as long as the old functionality is not broken.

Answer 2

So the Open Closed Principle is a gotcha...especially if you try to apply it at the same time as YAGNI. How do I adhere to both at the same time? Apply the rule of three. The first time you make a change, make it directly. And the second time as well. The third time, it's time to abstract that change out.

Another approach is "fool me once...", when you have to make a change, apply OCP to protect against that change in the future. I would almost go so far as to propose that changing the overtime rate is a new story. "As a payroll administrator I want to change the overtime rate so that I can be in compliance with applicable labor laws". Now you have a new UI to change the overtime rate, a way to store it, and GetOvertimeFactor() just asks its repository what the overtime rate is.

Answer 3

In the example you've posted, the overtime factor should be a variable or a constant. *(Java example)

class PaycheckCalculator {
   float overtimeFactor;

   protected float setOvertimeFactor(float overtimeFactor) {
      this.overtimeFactor = overtimeFactor;
   }

   protected float getOvertimeFactor() {
      return overtimeFactor;
   }
}

OR

class PaycheckCalculator {
   public static final float OVERTIME_FACTOR = 1.5f;
}

Then when you extend the class, set or override the factor. "Magic numbers" should only appear once. This is much more in the style of OCP and DRY(Don't Repeat Yourself), because it is not necessary to make a whole new class for a different factor if using the first method, and only having to change the constant in one idiomatic place in the second.

I would use the first in cases where there would be multiple types of calculator, each needing different constant values. An example would be the Chain of Responsibility pattern, which is is usually implemented using inherited types. An object that can only see the interface (i.e. getOvertimeFactor()) uses it to get all the information it needs, while the subtypes worry about the actual information to provide.

The second is useful in cases where the constant is not likely to be changed, but is used in multiple locations. Having one constant to change (in the unlikely event that it does) is much easier than setting it all over the place or getting it from a property file.

The Open-closed principle is less a call to not modify existing object than a caution to leave the interface to them unchanged. If you need some slightly different behavior from a class, or added functionality for a specific case, extend and override. But if the requirements for the class itself change (like changing the factor), you need to change the class. There is no point in a huge class hierarchy, most of which is never used.

Answer 4

I don't really see your example as a great representation of OCP. I think what the rule really means is this:

When you want to add a feature, you should only have to add one class and you should not need to modify any other class (but possibly a config file).

A poor implementation below. Every time you add a game you would need to modify the GamePlayer class.

class GamePlayer
{
   public void PlayGame(string game)
   {
      switch(game)
      {
          case "Poker":
              PlayPoker();
              break;

          case "Gin": 
              PlayGin();
              break;

          ...
      }
   }

   ...
}

The GamePlayer class should never need to be modified

class GamePlayer
{
    ...

    public void PlayGame(string game)
    {
        Game g = GameFactory.GetByName(game); 
        g.Play();   
    }

    ...
}

Now assuming my GameFactory also abides by OCP, when I want to add another game, I would just need to build a new class that inherits from the Game class and everything should just work.

All too often classes like the first get built up after years of "extensions" and just never refactored correctly from the original version (or worse, what should be multiple classes remains one big class).

The example you provide is OCP-ish. In my opinion, the correct way to handle overtime rate changes would be in a database with historical rates kept so data could be reprocessed. The code should still be closed for modification because it would always load the appropriate value from the lookup.

As a real world example, I've used a variant of my example and the Open-Closed Principle really shines. Functionality is real easy to add because I just have to derive from an abstract base class and my "factory" picks it up automatically and the "player" doesn't care what concrete implementation the factory returns.

Answer 5

In this particular example, you have what is known as a "Magic Value". Essentially a hard coded value that may or may not change over time. I'm going to attempt to address the conundrum you express generically, but this is an example of the type of thing where creating a subclass is more work than changing a value in a class.

More than likely, you have specified behavior too early in your class hierarchy.

Let's say we have the PaycheckCalculator. The OvertimeFactor would more than likely be keyed off of information about the employee. An hourly employee may enjoy an overtime bonus, while a salaried employee wouldn't get payed anything. Still, some salaried employees will get straight time because of the contract they were working on. You may decide that there are certain known classes of pay scenarios, and that's how you would build up your logic.

In the base PaycheckCalculator class you make it abstract, and specify the methods you expect. The core calculations are the same, it's just that certain factors are calculated differently. Your HourlyPaycheckCalculator would then implement the getOvertimeFactor method and return 1.5 or 2.0 as your case may be. Your StraightTimePaycheckCalculator would implement the getOvertimeFactor to return 1.0. Finally a third implementation would be a NoOvertimePaycheckCalculator that would implement the getOvertimeFactor to return 0.

The key is to only describe behavior in the base class that is intended to be extended. The details of parts of the overall algorithm or specific values would be filled in by subclasses. The fact that you included a default value for the getOvertimeFactor leads to the quick and easy "fix" to the one line instead of extending the class as you intended. It also highlights the fact that there is effort involved with extending classes. There is also effort involved in understanding the hierarchy of classes in your application. You want to design your classes in such a way as to minimize the need to create subclasses yet provide the flexibility you do need.

Food for thought: When our classes encapsulate certain data factors like the OvertimeFactor in your example, you might need a way to pull that information from some other source. For example, a properties file (since this looks like Java) or a database would hold the value, and your PaycheckCalculator would use a data access object to pull your values. This allows the right people to change the behavior of the system without requiring a code rewrite.