Say I have a method called ‘functionA’ that is called by a service and carries out a single piece of functionality, it is easy to unit test as it is does one thing only.
If a few months later an new requirement comes in that requires another method ‘functionB’ to be called directly after ‘functionA’ has executed. What is the better approach to add functionB to my code.
to call ‘functionB’ at the end of the ‘functionA’ method:
functionA() {
//do something..
functionB();
}
functionB() {
//do somehting...
}
Problem with this is that when I unit test functionA it will call functionB that will also be part of the test (so now it is more of an integration test).
Or refactor the code with a new method ‘functionX’ that calls ‘functionA’ and then ‘functionB’?
functionX() {
functionA();
functionB();
}
functionA() {
//do something..
}
functionB() {
//do somehting...
}
I can unit test both methods individually but now I have changed the API.
I see the first approach so much in legacy code, is it best practice to always make methods closed for modification or are there examples when it is OK to use the first approach?
6
My rule of thumb is that when adapting a design, you should aim to make the design as you would have were you starting from scratch. Don’t make your design sub-optimal now because of the characteristics of some previous design. The only result of that will be a big mess of a code base.
The fact that you need to modify this function says there was a problem with your original design. It did a poor job of adapting to requirements. Ideally, you shouldn’t have to modify your code in order to adapt to requirements. (Its a bit of an unattainable goal, but it’s the goal nonetheless).
So you’ve learned something about how requirements tend to change. Using that knowledge, how would have designed this piece of code to make it easier to make those sorts of changes in future? How can you avoid breaking unit tests, ease implementation?
Of course, your example is too vague to know what the best approach is. But I’d say: don’t be afraid to modify the function, but keep in mind that you want to produce designs that won’t require you to modify it for future uses.
The situation is somewhat different if you are writing code that other projects will use. For them any change of functionality, even just fixing bugs, can cause problems. There care needs to be taken. However, if its just your code, I don’t think thats a concern.
0
Here are some situations where the first approach would have been sufficient.
-
If the “new requirement” is a change in implementation detail, that is, caused by a response to another code change or code-contract change somewhere else.
- In this case, your change should be “billed” to the original source of that change.
-
If it is actually a bug fix, that is, if
functionAshould have always calledfunctionBwere they to be re-written from scratch.- Bug fixing is an explicitly permitted exception to this principle.
Either way, it is helpful to remember the original motivation for this principle:
- To avoid causing behavior changes in existing clients that don’t need/expect the new behavior.
- To reduce the effort (via testing) of re-establishing that the code change does not introduce bugs into the overall system.
And also remember that OCP is typically applied during class design.
Typically, OCP influences class design by encouraging the designers to think about the extension points of a set of classes, and make them explicit in the code contract or interface.
From this thinking, one could ask: Is it possible that some clients would have preferred functionA to call functionB and meanwhile some other clients preferred otherwise? This would have required a toggle switch (in the simplest sense), or an extension point (in the OOP sense).
1
A strict adherence to OCP would imply that a new object with the functionA() interface that has the correct behavior be created and substituted for this object. Whether inheritance or interfaces are used to ensure substitutability (LSP) depends on the flavor of OCP you prefer, but the ideas are the same.
Your unit tests for the old object would not change and you would write new unit tests for the new object.
If you’re finding difficult to create a new object that has all the behavior necessary to substitute it for this object, that speaks to your objects having too many responsibilities (SRP).
How strictly you want to apply OCP (and the other SOLID principles) here is a decision for you to make.
Your question focusses on the implementation view of your methods. But methods are a way to create the application-specific language you want to use for the next-higher abstraction level.
IMHO, you get a better guidance by looking at the abstractions (or contracts) that the methods are meant to provide. Of course, we’d need more information than just arbitraty method names like functionA or functionB to give specific hints.
What is the contract of functionA? What abstraction does it provide? And, from this abstract point of view, is calling functionB() something that
- has to be done now inside
functionA, because without it it would no longer do its job correctly? - has nothing to do with the original contract of
functionA, and just happens to be needed quite often (always?) in conjunction withfunctionA?
So, I see three possibilities:
- Keep the abstract meaning (contract) of
functionAunchanged, and callingfunctionBnow is a necessity to fulfill that contract. ThenfunctionBgoes insidefunctionA. - Keep the contract of
functionAunchanged, butfunctionBis not part of that contract. Then callfunctionBafterfunctionAwherever needed. - Find a new abstract meaning for the combination of
functionAandfunctionB. As most probably, the oldfunctionAnaming will not convey the new meaning, give that combination a new namefunctionC.
“Closed for modification etc.” is a rule for libraries that are used from multiple places. Or even by multiple independent customers. If two dozen customers need to change their code because of a library change, that is obviously bad.
But if you have a project that is the only user of a class, then you are absolutely free to do some refactoring. You want the whole project to change its behaviour, so you make these changes in the most appropriate places.
1