Is "avoid feature envy" violating "open closed principle"?

Question

After reading

What is a" feature envy" code and why is it considered a code smell?"

I know the following code is suffering from "feature envy":

 public class Garden{
 public float width;
 public float height;
 }
 public class Owner{
 Garden garden;
 public float getAddLawnCost(){
 return this.garden.width*this.garden.height*LAWN_PRICE_PER_1M2;
 }
 }

and area of garden should be calculated inside Garden:

 public class Garden{
 private float width;
 private float height;
 public float getArea(){
 this.width*this.height;
 }
 }
 public class Owner{
 Garden garden;
 public float getAddLawnCost(){
 return this.garden.getArea()*LAWN_PRICE_PER_1M2;
 }
 }

Suddenly a new requirement is added : calculate the cost of adding fence to the garden:

"Feature envy" version, class to modify : Owner

 public class Garden{
 public float width;
 public float height;
 }
 public class Owner{
 Garden garden;
 public float getAddLawnCost(){
 return this.garden.width*this.garden.height*LAWN_PRICE_PER_1M2;
 }
 public float getAddFenceCost(){
 return (this.garden.width+this.garden.height)*2*FENCE_PRICE_PER_1M;
 }
 }

"non-feature envy" version, classes to modify : Owner, Garden:

 public class Garden{
 private float width;
 private float height;
 public float getArea(){
 this.width*this.height;
 }
 public float getPerimeter(){
 (this.width+this.height)*2;
 }
 }
 public class Owner{
 Garden garden;
 public float getAddLawnCost(){
 return this.garden.getArea()*LAWN_PRICE_PER_1M2;
 }
 public float getAddFenceCost(){
 return this.garden.getPerimeter()*FENCE_PRICE_PER_1M;
 }
 }

which the "non-feature envy" version has one extra class to modify : Garden, and hence I think it is violating "Open closed principle", is it true?

• JonasH: Keep in mind that all programming "principles" are general recommendations, not dogma to slavishly follow. In this specific case I think your "non feature envy" example is easier to read, so I would not care that an extra class where modified.
• gnasher729: What JonasH said, plus consider who is affected. Changing a class used inside one project only by you and your colleagues: Change it if you can make all changes needed elsewhere (easy here because no changes elsewhere are required). Inside a library downloaded by 10,000 developers: Think very hard before you make changes.
• Filip Milovanović: This is not really a good toy example to understand feature envy. Your garden class is essentially just a general-purpose rectangle data structure with the wrong name, that doesn't have a lot of its own behavior - it could serve as a parameter to getAddLawnCost() and getAddFenceCost(). Feature envy would be something more like - suppose you had some code that had an instance of Owner, and that needed to orchestrate some calculation - but instead of Owner having these two methods, it would just have a Garden getter, and then you'd do calculations in the calling code.
• Filip Milovanović: P.S. I guess it helps if you think of having two different kinds of classes - one kind serves as just mostly anemic data structures that are more general in purpose, and the other kind proper OOP objects that encode within them domain-relevant behavior - it's more important to apply these principles to this second group. Then when you're writing your classes, decide which role each class will fill, and try and apply the principles based on that.
• candied_orange: @MartinMaat closed to modification means closed to modification. This isn't just an interface thing. This means you don't touch the binary. This is why Java stuck us with type erasure. They didn't want to touch their binaries. OCP is about planning for and dealing with untouchable code.

Answer 1

The difference is when.

Yes, resolving Feature Envy involves making modifications not extensions. Which is exactly what the Open Closed Principle tells you not to do. And that's exactly why you should resolve Feature Envy before you let anyone else see the class.

The cost of violating OCP, and performing a modification, goes up as the class you're considering modifying becomes more well known. So the best time to resolve Feature Envy is when you're creating a new class. That's when it's the least painful.

The principles should not be dismissed just because you find they conflict. They are lessons that point out costly choices. Carefully consider what you're willing to pay before you decide these costs are not worth avoiding.

Answer 2

Fighting feature-envyness (as well as other design principles like keeping methods small, the DRY principle, or simply the usage of descriptive names) requires the ability to refactor code constantly. This works best when all classes of a certain object model or library are under control of a single team, and no external libs depend on them directly.

The OCP is sometimes misunderstood as a principle which forbids to make changes - and if that would be true, it would indeed be add odds with the former principles, since it would disallow refactorings. However, that is IMHO not what the OCP is about(*). The OCP is about designing classes and libraries in a way they can be reused without the necessity of making changes. It's goal is to allow a team or organization A to design and implement a library in a way so a team B can reuse it without asking A for changes, even when the reusage requires some kind of feature extension. Hence, the OCP does in no way forbid "feature-rich" classes (like a Garden class with methods getArea and getPerimeter), quite the opposite.

Of course, once a class library is published by team A, certain refactorings, especially ones involving the public methods of that class, become inherently harder. Moreover, a Garden class designed with the OCP in mind may provide public getters for width and height, because having direct access to those values will allow a variety of unforeseen requirements to be implemented. That, however, may encourage some features to be implemented completely outside of that class - maybe by some team B, which is responsible for the Owner class. And that can indeed lead to feature-envyness.

Hence, not those two principles contradict each other - only the situations when and where to use them are opposing:

• when the goal is to optimize the "inner design" of a library or code base (for example, by minimizing feature envyness), constant and regular refactorings are necessary.

• when the goal is to create a library which can be reused by someone else in a black-box fashion, then refacorings to the public API will require more organizational effort. That is the situation where the OCP makes sense, which means to design the public API in a way less refactorings will be necessary.

_{(*)Side note: even Bob Martin admitted in 2013 that his original definition of the OCP from 1996 was poorly worded and could lead readers to mistakenly believe that its intent was to prohibit changes, see https://blog.cleancoder.com/uncle-bob/2013/03/08/AnOpenAndClosedCase.html}

Answer 3

Your example is super basic and begs the question can an object with no methods be considered OOP at all.

Rather than changing your "feature envy" style class, with its public fields to a "non feature envy" encapsulated version, we can achieve the same lack of "feature envy" by simply adding a intermediate class:

GardenWithArea : Garden
{
 float Area() => width * height
}

Or..

CircularGarden : GardenWithArea
{
 override float Area() => //complicated maths I have forgotten! GCSE++
}

Or if we are doing the best OOP, ADM

AreaCalculator : IAreaCalculator
{
 float GetArea(Garden garden) => //complicated code I need to ask ChatGTP about
}

Can we really say that Garden is "closed for extension" if we can do all these?

• Doc Brown: Your example where GardenWithArea inherits from Garden will still require width and height to be made protected, as long as those attributes are private the Garden class is not following the OCP.
• Ewan: erm. I think that would be encapsulation. But yeah its so simple I dont think it matters. its just a struct
• Doc Brown: Maybe I was not clear. If you want Garden to be OCP-compliant, assume it is compiled into a binary lib and you have no access to the source code. Then the version with those private fields width and height will not allow you to implement GardenWithArea as shown in this answer. And yes, that's encapsulation - too much encapsulation will conflict the OCP.
• Ewan: ok, but the "feature envy" version has public fields
• Ewan: ahh i see what you mean. No, my point is you can "avoid feature envy" by extending the class, rather than writing it like the "non feature envy" example

Answer 4

The problem is not solvable if you want to follow the open-closed principle strictly.

According to the original definition of the open-closed principle⁽¹⁾, any change to the public interface of a "published" class is a violation of the principle. Which mean both solutions you present violate the principle, since both change the public interface of at least one class.

(The principle is a bit ambiguous about what it means to "publish" as class - obviously you change a class continuously while developing it. But the principle assumes that at one point you "publish" the class to make it available for use in production code. From this moment on, any further change will run the risk of introducing errors and should be avoided.)

According to the principle, new functionality should instead be added through subclasses, e.g. by adding a GardenWithFence class inheriting from Garden. But the problem is, width and height are private in Garden which means you cannot access them in a subclass to perform the perimeter calculation. Which means you cannot solve the problem.

The principle is not in conflict "avoid feature envy" in particular, it is in conflict with any change or refactoring with affect the public interface of published objects.

The principle might seem overly strict, but consider that the principle is intended for object-libraries which are published for others to use, possibly in complex application. In this scenario it is prudent to avoid breaking changes. If you have full control over every use of a class, then you don't need to follow the principle, since you can just change the class and update any dependent code as necessary. E.g. if Garden and Owner classes are only used in your own code and not part of a published library, then there is no reason to follow the principle. It would be much better to refactor freely and instead use unit testing to ensure no bugs are introduced.

---

⁽¹⁾ See Object Oriented Software Construction by Bertrand Meyer, page 57-61.

• Basilevs: GardenWithFence should not inherit. Instead all producers should have an alternative factory for gardens, producing a new type. Unrealistic still, but possible.
• JacquesB: @Basilevs Are you suggesting copy-pasting code over into a new type and then add the new feature?
• Basilevs: Yes. That's how "closed for modification" problem is solved.
• JacquesB: @Basilevs So if you need to add yet another feature you copy again? You will get exponential amounts of duplicated code by that approach.
• Basilevs: Yes, that's what happens when API is ill-designed. My point was that it is not impossible, just impractical.

Answer 5

The open-closed principle doesn’t mean you can’t change any source code. It doesn’t mean that as all.

What it means: If you have a class whose instances have to behave in different ways, then it is much nicer if you can change the behaviour of an instance from the outside, instead of having to change the source code.

Say I create a class whose instances draw green buttons. But you want a red button. So you modify the source code. If you are slightly clever you now have two classes "Green button" and "red button". Which is still awful. Especially if someone else wants a blue button. Much better if your instances have a property "button Color" and people can change that and have buttons of any Color they want.

Now if you don’t have that, what do you do? Not change the class and live with green buttons? Of course not. You now have an idea what kind of variation people want, so you add the "button Color" property. This is a small change if the Color must be chosen when the button is created. More work if the Color property can be changed while the button is visible on the screen.

So you should have the goal to make your class flexible if needed. Never changing it is not the goal. Reducing the number of situations where you have to change it, that is a goal that needs weighing against other goals.

Answer 6

A debate was started in the comments about the true meaning of the open-closed principle (OCP). I argued it is an OO principle and as such refers to inheritance exclusively. Others regard it as a more general guideline. Please allow me to elaborate.

@DocBrown mentioned a Jon Skeet blog post on the matter.

Skeet mainly advocates that OCP is not a helpful clear guide line. We can probably all agree on that. Open for interpretation, closed for conclusion. However, he also acknowledges Bertrand Meyer's original definition definitely hints towards implementation inheritance.

The quotes on the Wikipedia page about Bertrand Meyer's Open-closed principle also leave little room for a wider interpretation. Extension is implementation inheritance.

Mind that Bertrand Meyer's book "Object Oriented Software Construction" was published in 1988. Object orientation in those days was largely an academic concept. Current programming languages at the time did not support OO in a way we are used to today. Extension was quite literally "make a data structure longer to add some fields". He was not thinking about virtual methods or interface implementations.

Then Robert Martin adopted the principle in his SOLID collection and had his notoriously controversial and confusing way with it. So now we argue over its true meaning.

We may want to settle on the idea that it's outdated and better avoided as a basis to make a case for anything.

Answer 7

That could be true, yes, it is going around the open close principle, it also goes around object oriented principles - abstraction, encapsulation, inheritance, polymorphism. The indicator of going around the open close principle is whether a class has to be modified or not rather than the number of classes to modify that is a sign of how far away from the principle the implementation is.

With the proper implementation the code snippet could be referred by decorator pattern, aggregate root, IS-A / HAS-A relation terms just to name a few without going vastly wide otherwise it could end up with a CISC or RISC architecture argument.