3

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;
        }
    }
111
Which the "non-feature envy" version has one extra class to modify : Garden, and hence violating "Open closed principle". Is it true?
Rohit Gupta
  • 203
  • 2
  • 3
  • 12
wcminipgasker2023
  • 721
  • 1
  • 3
  • 10
  • 1
    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. – JonasH Aug 08 '23 at 09:07
  • 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. – gnasher729 Aug 08 '23 at 09:39
  • 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ć Aug 08 '23 at 13:44
  • 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. – Filip Milovanović Aug 08 '23 at 14:14
  • You are misrepresenting the open-closed principle. The OCP is specifically about implementing features in a base class that should have been implemented in a descending class, thus about inheritance. Since you are not demonstrating inheritance, the OCP does not apply. – Martin Maat Aug 08 '23 at 14:57
  • @MartinMaat: the OP might have misunderstood the OCP, but your comment shows also a common misconception about the OCP. Though the OCP is often demonstrated with extension points implemented using virtual methods, it is not restricted to that special case (see my [former answer here](https://softwareengineering.stackexchange.com/a/362033) for more details.) – Doc Brown Aug 08 '23 at 18:08
  • ... in this specific case, it the `Garden` class with the private fields violates the OCP because a certain feature cannot be implemented without modifying the class (and it is not the programmer who violates the OCP at the point in time when adding new functions to it, it is the programmer who designed the class by not providing at least public getters for width and height). – Doc Brown Aug 08 '23 at 18:19
  • 1
    @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. – candied_orange Aug 08 '23 at 20:36
  • @candied_orange Every SOLID principle pertains to object oriented programming specifically. Making it some broad, general rule about not needing to modify a library in order to add functionality is incorrect. If you feel the need for a broader term you should make up your own. Open for extension means open for inheritance, because that is the OO way of extending. Considering its context I'm pretty sure the broader interpretation peddled here is not what the original author(s) meant by it. – Martin Maat Aug 09 '23 at 09:57
  • @MartinMaat: I clearly share candied_orange's point of view. The OCP is a design *principle*, not just a pattern or programming idiom restricted to the usage of inheritance. My understanding here is that the OCP is defined by its goals - and keeping a library "closed for modification" is a goal which is not necessarily achieved by providing virtual, overridable methods exclusively. Concerning OO: class design by deliberately making some properties public is also a well known OO technique. It is just so simple that most OCP "explainers" at the web don't think it is worth mentioning it. – Doc Brown Aug 15 '23 at 05:18
  • ... I would also recommend to have a look at [Jon Skeet's assessment of the OCP](https://codeblog.jonskeet.uk/2013/03/15/the-open-closed-principle-in-review/). – Doc Brown Aug 15 '23 at 05:34

5 Answers5

5

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.

candied_orange
  • 102,279
  • 24
  • 197
  • 315
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. 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

Doc Brown
  • 199,015
  • 33
  • 367
  • 565
1

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?

Ewan
  • 70,664
  • 5
  • 76
  • 161
  • 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. – Doc Brown Aug 08 '23 at 20:58
  • erm. I think that would be encapsulation. But yeah its so simple I dont think it matters. its just a struct – Ewan Aug 08 '23 at 21:05
  • 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. – Doc Brown Aug 08 '23 at 21:18
  • ok, but the "feature envy" version has public fields – Ewan Aug 08 '23 at 21:35
  • 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 – Ewan Aug 08 '23 at 21:40
1

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.

Martin Maat
  • 18,218
  • 3
  • 30
  • 57
0

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.