10

I have a class calculating workers' net yearly income. It has a constant representing a tax percentage. But one day the tax rate has changed, so I need to fix the code.

Does the act of fixing this constant indicate a violation of the Open-Closed Principle, since it postulates that a class should be closed to modification?

11
  • 10
    Software changes because the real world changes. On the other hand making a tax percentage a constant is not so much a violation of Open-Closed Principle as it is just an ignorant thing to do. Tax percentage is an obvious changeable item that should be bound at run time. Dec 7 '17 at 13:37
  • 4
    I completely agree with Richard. If you have to change the code to fix this "constant," OCP is the least of your problems. Dec 7 '17 at 17:38
  • 3
    What constitutes a violation of OCP is highly subjective and the whole thing is somewhat obsolete anyway (since implementation inheritance is not best practice anymore). This is a typical question where you have to guess what the person asking the question thinks. Dec 8 '17 at 8:58
  • 2
    @DocBrown: what constitutes a "new requirement"? You show me some code, I can point out new requirements which will definitely require changing code, regardless of how OCP conform you make it. So back to the question: If the developer asked the business expert about it, and there was no expectation of the tax rate changing more than once every couple of years, there is no point in making it configurable or injectable. Just keep it simple and prepare for what you know. And for those things, sure, make it external to the class. So it depends. Dec 8 '17 at 22:05
  • 1
    @RobertBräutigam: my point is, there is IMHO no such thing as "OCP conform", there is only "OCP conform in context of certain categories of requirements". There can surely be some subjectivity to which categories a component should be "OCP conform". But in the case described in this question, the way I understand it, a changing requirement was already identified, so that this "income calculating class" clearly does not obey the OCP in context of this specific requirement.
    – Doc Brown
    Dec 8 '17 at 22:34
14

The OCP can be better understood when thinking of classes or components provided by a vendor A in some kind of black-box library, for usage by users B, C, and D (note this is just a mental model I am using for clarity, it does not matter if in reality the only user of the class is A himself).

If B, C and D can use or reuse the provided classes for different use cases, without the need for modification of the library's source code, then the component fulfills the OCP (in respect to a category of use cases). There are different means to achieve this, like

  • making the class inheritable (typically in conjunction with the template method pattern or the strategy pattern)

  • by providing "injection points" for dependency injection

  • by providing configuration parameters for the class or component (for example, by having a constructor parameter "tax percentage", as in your case, or by using some other configuration mechanism)

  • maybe other means, depending on the programming language or ecosystem

The typical examples you find in text books are often of the first or second type (I guess because in the eyes of those book's authors, the third type is too trivial to be worth mentioned).

As you see, this has nothing to do with forbidding any change of the source code by vendor A (like for bug fixing, optimization or adding new features in a backwards-compatible manner), that is quite unrelated to the OCP. The OCP is about how A designs the interface and the granularity of the components in the lib, so different reusage scenarios (like resuage with different tax rates) do not automatically induce requirements for change.

So despite what others are told you here, the answer is clearly "yes", it would be a violation of the OCP.

EDIT: seems in between someone wrote a detailed blog post about exact this topic. Though parts of it could have been better worded (as Derek Elkins pointed out), it seems the author generally shares my point of view that "fulfilling the OCP" is no absolut property, but a something which can only be evaluated in context of certain categories of requirement changes.

8
  • OK, so OCP is about providing extensible behavior for different use cases with one of three means you've listed, right? But what if an OP's example implied that something fundamental is about to change? I don't know what country OP's from, but in my country tax rate is something that doesn't change very often. It was a poor example, but maybe it was intentionally extracted in a constant, to stress the point. I'm pretty sure it wasn't meant to be configurable or extendable. So maybe the question was about "this has nothing to do with forbidding any change of the source code by vendor A" part. Dec 8 '17 at 17:49
  • At least I understood it that way. Poor guy who deleted his accepted answer did so either I guess. You've seen it from a bit different angle -- I understand your point and agree with it. But it seems that the most wise comment was given by @Robert Bräutigam. Until now I haven't realized that OCP is THAT subjective. Dec 8 '17 at 17:53
  • 1
    I guess if I ever get asked the same question, there is one question I should ask in reply: "is that behavior supposed to be extended or configured somehow?". If yes -- than direct modification of a class itself is a violation of OCP. If no -- than OCP is simply not applicable in that situation. Dec 8 '17 at 18:00
  • 1
    @Zapadlo: I think if a component fulfills the OCP for a class of requirements is not very subjective - it is pretty clear in most cases if a new requirement needs a modification of the source code of a component, or if the component supports this requirement ". The possible approaches to implement it are not restricted to the first 3 means I listed, see my edit. Your notion of subjectivity might be caused because the OCP just has a misleading name and is pretty bad explained in lots of textbooks.
    – Doc Brown
    Dec 8 '17 at 21:43
  • My notion of subjectivity was caused by the fact that I didn't fully understand what you said -- but I do now, I guess. Thanks much for insightful comments and your answer. Dec 10 '17 at 12:15
4

As others are saying, ideally the worker income class would allow for parameterization of the constant, making this class independent of that value.

Ultimately, the calling application might also allow for parameterization in terms of external configuration (e.g. a file). Once we have external configuration, we can change the tax rate — though consider that if the configuration file is read only once at startup, then the application will have to be restarted for updated tax percentages to take effect, so that is something to keep in mind. We could provide an application feature to reread the configuration when directed to do so, or we might provide a more complicated mechanism that notices when the configuration file changes...

Long term, you may find that the tax issues require more than just a percentage — for example, that one day the tax laws are more complex and requires several percentages and some constants (e.g. the amount under $10k taxed at X%, while the remainder taxed at Y%).

This basically suggests using a strategy pattern, where the main class in question here accepts a strategy object for computing the tax.

The various strategies (and %'s and $ constants) should be choose-able from the configuration file, and now, adding a new strategy requires adding some new code, but not necessarily updates to existing code.

Each strategy might know how to parse/interpret its own external configuration arguments, along with how to compute actual tax.

Dynamically, the tax may further depend on the governing locale, so you might have locale associated with earnings or with employees (or both). In external configuration, we might associate locale with tax strategy.


Also see dependency injection, where we manage these things explictly.

1
  • 1
    The question was not if it is a bad idea to bury something like a tax percentage in the code, I am sure that is obvious to most of us here (including the OP). The question was, "does this violate the OCP?" So I don't see how your answer refers to this question.
    – Doc Brown
    Dec 8 '17 at 9:15
1

If you need to modify the class to change the tax value, then its design is indeed violating OCP. The appropriate design, for what you've described so far, is for the calculator class to take the tax value as a parameter.

If your class is instanced (meaning it's not a static class), by making the tax variable class property, whose value is injected through the constructor, you'd be also improving the class cohesion.

In short, your current design make your class depend on a constant value that's not really a constant (defining constant as a value that'd never change no matter what, like the value of PI). It violates OCP. Change the design to receive the tax value as constructor argument.

0

Totally agree with @Becuzz, and I just want to sum this up: OCP is about finding reused (hence, useful) abstractions that are injected in a class. So the behavior of the class is modified not by changing its code, but by providing it with different implementations. This is made crystal clear in Robert Martin's book "Agile Software Development, Principles, Patterns, and Practices", check the corresponding chapter "The Open-closed principle", "Abstraction is the Key" sub-chapter. It clarifies another misconception that behavior can be modified only with inheritance. It was Bertrand Meyer who proposed that in 1988 in his book “Object Oriented Software Construction”, not Robert Martin.

-2

The way I see it is not a violation of the open closed principle. Yet, the fact that something that is bound to change in time (such as the tax percentage) is a constant is a design flaw: you should not change the value of the constant but how you handle the tax percentage. This should be some type of setting that could be modified without recompiling the whole thing.

1
  • The "design flaw" being that it's violating the open closed principle as you need to recompile the code to change the constant? Dec 8 '17 at 9:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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