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?
Best Answer
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 methodsgetArea
andgetPerimeter
), 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 forwidth
andheight
, 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 theOwner
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