views:

161

answers:

8

The Wikipedia article on OCP says (emphasis mine):

... the open/closed principle states "software entities (classes, modules, functions, etc.) should be open for extension, but closed for modification"... This is especially valuable in a production environment, where changes to source code may necessitate code reviews, unit tests, and other such procedures to qualify it for use in a product: code obeying the principle doesn't change when it is extended, and therefore needs no such effort.

So, am I reading it correctly that OCP would be valuable if there is no automated unit testing, but not necessarily if there is? Or is the Wikipedia article wrong?

A: 

I think it is still a valuable principle even if you have automated unit testing.

fpmurphy
So, in your opinion, the Wikipedia article is not correct?
Rogerio
A: 

Like most ideas originated by Bertrand Meyer, the Open/Closed Principle is pretty much just flat-out wrong. If your system needs some new functionality, and that functionality belongs in an existing class, change that class. Don't put it somewhere else just to satisfy an arbitrary law.

Tom Anderson
This answer belongs here: http://stackoverflow.com/questions/406760/whats-your-most-controversial-programming-opinion :)
cwap
Have you written about this in a blog or elsewhere? I'd love to see a well thought out critique of Meyer's work. This answer of course's not it.
Vinko Vrsalovic
Yes, that's what in my experience seems to works best in the real world, particularly if you have automated developer tests.I guess some people use principles like the OCP for political gain, by promoting it to managers and "average developers" so they are seen as superior developers that understand the supposed "great principles of OO design". It's certainly easier to throw acronyms in a whiteboard than actually produce great software.
Rogerio
Of course there are lousy consultants that take advantage of "established best-practices" to profit, that doesn't necessarily invalidate all they say. Use your common sense.
Vinko Vrsalovic
Using my common sense is exactly what I do. Problem is, others seem to say that no matter what are my circunstances, I will be wrong in not applying the OCP.
Rogerio
A: 

Making changes to a piece of code might also break the tests, by for example changing the name of a method. This is what the OCP is for - Don't create code where you need to edit previous code in order to make it behave differently. Instead, make it so that if you need it to act differently, you can do so by making extensions.

You can see this kind of design a lot of places: (N)Hibernate Interceptors, WPF data templates, etc. etc.. :)

cwap
How would you rewrite the Wikipedia article then?
Rogerio
I wouldn't..
cwap
A: 

Unit tests help in upholding the open/closed principle: the necessary changes (refactoring of bad code) are validated by a unit test suite to check that no externally visible behavior changes have occurred.

MaxVT
But won't you have violated the principle the moment you start refactoring the bad code? Isn't that exactly what OCP tells us to avoid doing?
Rogerio
+3  A: 

I think you are reading too much into the damned OCP. My interpretation of it is "think thrice before modifying an existing class on whose behavior depend lots of code which is not controlled by you".

If the only users are you and your dog, of course you can modify the guts out of the class while being very efficient and have no problems at all.

If your users (regardless of their being internal or external) are many, you really have to consider all the impact a change inside a working class can have, and, if your user base is huge, you just can't anticipate and will have to:

  • risk breaking something for somebody
  • let them extend it
  • bloat the design by extending it yourself

pick the best for your use case.

As always, understanding the context and the tradeoffs is what makes engineering interesting. Know when to pick the proper tool. There are times when OCP does not apply, but that doesn't ivalidate its helpfulness, if you considered it and rejected it because it doesn't apply to your context for A and B.

Vinko Vrsalovic
OK, agreed. I did not say that OCP never applies. My question is for an specific situation: an application with a full unit test suite, where most or even all code is not published for use by developers outside the team. According to what you say, this would be one of the times the OCP does not apply, right?
Rogerio
As long as you can **validate** that your changes won't break anything (harder than it looks), yes, it might be due to skip OCP. That could mean having a full unit test suite, but then again, you might need either further verifications or less verifications. As with all generic principles, the idea is to understand them and know when and why you can skip them.
Vinko Vrsalovic
Great! I believe we are in agreement then.
Rogerio
+5  A: 

Unit tests, by definition!, are about behavior inside a unit (typically a single class): to do them properly, you try your best to isolate the unit under test from its interactions with other units (e.g. via mocking, dependency injection, and so on).

OCP is about behavior across units ("software entities"): if entity A uses entity B, it can extend it but cannot alter it. (I think the emphasis of the wikipedia article exclusively on source code changes is misplaced: the issue applies to all alterations, whether obtained through source code changes or by other runtime means).

If A did alter B in the process of using it, then unrelated entity C which also uses B might be adversely affected later. Proper unit tests would typically NOT catch the breakage in this case, because it's not confined to a unit: it depends on a subtle, specific sequence of interactions among units, whereby A uses B and then C also tries to use B. Integration, regression or acceptance tests MIGHT catch it, but you can never rely on such tests providing perfect coverage of feasible code paths (it's hard enough even in unit tests to provide perfect coverage within one unit/entity!-).

I think that in some ways the sharpest illustration of this is in the controversial practice of monkey patching, permitted in dynamic languages and popular in some communities of practitioners of such languages (not all!-). Monkey patching (MP) is all about modifying an object's behavior at runtime without changing its source code, so it shows why I think you can't explain OCP exclusively in terms of source code changes.

MP exhibits well the example case I just gave. The unit tests for A and C can each pass with flying colors (even if they both use the real class B rather than mocking it) because each unit, per se, does work fine; even if you test BOTH (so that's already way beyond UNIT testing) but it so happens that you test C before A, everything seems fine. But, say, A monkeypatches B by setting a method, B.foo, to return 23 (as A needs) instead of 45 (as B documentedly supplies and C relies on). Now this breaks the OCP: B should be closed for modification, but A doesn't respect that condition and the language doesn't enforce it. Then, if A uses (and modifies) B, and then it's C's turn, C runs under a condition in which it had never been tested -- one where B.foo, undocumentedly and surprisingly, returns 23 (while it always returned 45 throughout all the testing...!-).

The only issue with using MP as the canonical example of OCP violation is that it might engender a false sense of security among users of languages that don't overtly allow MP; in fact, through configuration files and options, databases (where every SQL implementation allows ALTER TABLE and the like;-), remoting, etc, etc, every sufficiently large and complex project must keep an eye out for OCP violations, even were it written in Eiffel or Haskell (and much more so if the allegedly "static" language actually allows programmers to poke whatever they want anywhere into memory as long as they have the proper cast incantations in place, as C and C++ do -- now THAT's the kind of thing you definitely want to catch in code reviews;-).

"Closed for modification" is a design goal -- it doesn't mean you can't modify an entity's source code to fix bugs, if such bugs are found (and then you'll need code reviews, more testing including regression tests for the bugs being fixed, etc, of course).

The one niche where I've seen "unmodifiable after release" applied widely are interfaces for component models such as Microsoft's good old COM -- no published COM interface is ever allowed to change (so you end up with IWhateverEx, IWhatever2, IWhateverEx2, and the like, when fixes to the interface prove necessary -- never changes to the original IWhatever!-).

Even then, the guaranteed immutability only applies to the interfaces -- the implementations behind those interfaces are always allowed to have bug fixes, performance optimization tweaks, and the like ("do it right the first time" just doesn't work in SW development: if you could release software only when 100% certain that it has 0 bugs and the maximum possible and necessary performance on every platform it will ever be used on, you'd never release anything, the competition would eat your lunch, and you'd go bankrupt;-). Again, such bug fixes and optimizations will need code reviews, tests, and so forth, as usual.

I imagine the debate in your team comes not from bug fixes (is anybody arguing for forbidding those?-), or even performance optimizations, but rather from the issue of where to place new features -- should we add new method foo to existing class A, or rather extend A into B and add foo to B only, so that A stays "closed for modification"? Unit tests, per se, don't yet answer this question, because they might not exercise every existing use of A (A might be mocked out to isolate a different entity when that entity gets tested...), so you need to go one layer deeper and see what foo exactly is, or may be, doing.

If foo is just an accessor, and never modifies the instance of A on which it's called, then adding it is clearly safe; if foo can alter the instance's state, and subsequent behavior as observable from other, existing methods, then you do have a problem. If you respect the OCP and put foo in a separate subclass, your change is very safe and routine; if you want the simplicity of putting foo right into A, then you do need extensive code reviews, light "pairwise component integration" tests probing all uses of A, and so forth. This does not constrain your architectural decision, but it does clearly point at the different costs involved in either choice, so you can plan, estimate and prioritize appropriately.

Meyer's dicta and principles are not a Holy Book, but, with a properly critical attitude, they're very worthwhile to study and ponder in the light of your specific, concrete circumstances, so I commend you for so doing in this case!-)

Alex Martelli
One of official OCP descriptions, http://www.objectmentor.com/resources/articles/ocp.pdf, says:"When requirements change, you extend the behavior of suchmodules by adding new code, not by changing old code that already works."Note it specifically refers to "requirements", not "bugs" or "optimizations".If each developer is free to choose his own interpretation of a published principle, then it becomes useless as guidance for a team.
Rogerio
I am certainly not the one taking such principles as dogma. Others seem to be doing it, though, by saying that the OCP always apply, and trying to induce others into following it blindly.
Rogerio
Wow. Alex wrote a carefully-worded, even-handed response and someone (I wonder who) downvoted it. Perhaps this person would serve the community better by taking their argumentative tone elsewhere.
TrueWill
Better an argumentative tone than personal attacks, though.
Rogerio
@Rogerio, what personal attacks are you talking about? Like all frequent responders I'm getting a callus about crazy, unexplained downvotes (like the ones I got here -- thanks @TruwWill for noticing that!-) -- but what do those anonymous, shrouded "personal attacks" against me have to do with other, open, identified "personal attacks" against somebody somewhere...?!
Alex Martelli
The ones made by "TrueWill" above. I explained my down vote, in the first comment up there. I could have also mentioned that the talk about "monkey patching" is totally irrelevant in the context of this discussion about OCP (which is all about manual changes to source code).
Rogerio
@Rogerio, it makes no sense to describe TrueWill's comment as "personal attacks". And OCP says "closed for modifications", NOT "closed for modifications via source changes but runtime patches are OK", so you're deeply wrong in stating that runtime patches are "totally irrelevant" to OCP: they're a sharp and clear example of a violation thereof (moreover, one that, clearly, unit tests in general cannot ward against).
Alex Martelli
@Alex, OK, lets forget about the supposed attacks; that's not what I am here for.Concerning OCP, however, it's quite clear from the official articles that they consider only manual source code changes. You are attributing additional meaning to OCP that is simply not there. Besides, I never use "runtime patches" anyway, so it's irrelevant to me.
Rogerio
@Alex, check out http://stackoverflow.com/questions/59016/the-open-closed-principle. No answer there gave me the impression that OCP is about anything else than controlling the impact of manual changes to source code.
Rogerio
A: 

This article from the C2 Wiki discusses the tension between OCP and XP.

From some comments in that article, and from this academic dissertation (section B.2), the answer to the question of "with unit tests, must OCP still be applied?" would appear to be no.

The reasoning is that OCP addresses the impact of functional changes to working code through up-front design, where abstractions are created early in the hope that they will provide enough extensibility later when new requirements come in.

Agile development (with XP or just TDD), on the other hand, addresses the impact of change through evolutive design ("Embrace change", anyone?), not by attempting to devise abstractions up-front. And as we all know, up-front design hardly works in practice.

Rogerio
+1  A: 

Good design principles (like the OCP) are not add odds with good development processes (like unit tests and TDD). They're complementary.

The Wikipedia article, IMO, assumes that you're always using good quality processes like unit tests and code reviews (in XP this translates to TDD and pair programming), even when using the OCP. What it goes on to say is that, with the OCP, you better control the scope of your changes, which results in less effort in these quality processes.

Jordão