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 violating "Open closed principle". Is it true?

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?

Answer 4

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.

Answer 5

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.