Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question
1  
actually with method 1 you also change the API in a more subtle way –  ratchet freak Mar 29 '13 at 11:41
    
I don't see why having functionA call functionB is a problem. –  Michael Shaw Mar 29 '13 at 13:12
1  
When your question contains "always" or "never" and does not deal with a technical requirement, you can safely assume the answer is "no". Better to phrase your question in terms of understanding general principles more clearly or finding out what the exceptions or criticisms are. –  Sean McSomething Mar 29 '13 at 20:06
    
@Michael Shaw: If functionA had already been implemented and tested for a previous version of the software, it should be extended, at least according to the open / closed principle. –  Giorgio Mar 29 '13 at 22:46
1  
@user86834: I also do not understand the downvotes and the close votes. The question is clear: should one be allowed to modify an existing method or should an existing method be closed for modifications? –  Giorgio Mar 30 '13 at 8:25
show 1 more comment

3 Answers

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.

share|improve this answer
    
This is sound advice thanks. –  user86834 Mar 29 '13 at 23:36
add comment

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 functionA should have always called functionB were 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).

share|improve this answer
    
+1: for a good answer and for pointing out that "bug fixing is an explicitly permitted exception to this principle.". Even if one discovers some possible improvements in the design, changing a tested method always introduces the risk that the software could not work as expected any more, and the need to perform (possibly extensive) tests to check for regression bugs. –  Giorgio Mar 30 '13 at 8:31
add comment

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.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.