How do I correct feature envy in this case?

Question

I have some code that looks like:

class Parent {
 private Intermediate intermediateContainer;
 public Intermediate getIntermediate();
}

class Intermediate {
 private Child child;
 public Child getChild() {...}
 public void intermediateOp();
}

class Child {
 public void something();
 public void somethingElse();
}

class Client {
 private Parent parent;

 public void something() {
  parent.getIntermediate().getChild().something();
 }

 public void somethingElse() {
  parent.getIntermediate().getChild().somethingElse();
 }

 public void intermediate() {
  parent.getIntermediate().intermediateOp();
 }
}

I understand that is an example of the "feature envy" code smell. The question is, what's the best way to fix it? My first instinct is to put the three methods on parent:

parent.something();
parent.somethingElse();
parent.intermediateOp();

...but I feel like this duplicates code, and clutters the API of the Parent class (which is already rather busy).

Do I want to store the result of getIntermediate(), and/or getChild(), and keep my own references to these objects?

Answer 1

In my experience, doing what you suggest (make all of the methods only call "the next level") will help to illuminate changes to your API that you should make. So yes, make those changes despite it cluttering your API and you may discover the appropriate way to deal with the issues.

The "fact" is that something has a Client when it should have (or wants to have) something else. This implies that either the functionality of that other thing is in the wrong place or the thing requesting that functionality is in the wrong place. Unfortunately, it's hard to give concrete examples without much more comprehensive code.

When you finally realize what the "problems" are, however, the solutions may not be so easy to implement. That's a bummer, but keep up the good fight and refactor toward that solution anyway!

Answer 2

This isn't exactly feature envy, but more of a high coupling issue between these classes and a definite violation of the law of demeter http://www.ccs.neu.edu/research/demeter/demeter-method/LawOfDemeter/general-formulation.html. Your first though of having the methods directly on the parent is a good though. Clients should not need to call through the stack like that.

If you're concerned about the parent being too busy, maybe you have too much functionality in there and it should be split into some smaller classes. Classes should be single purposed.

Answer 3

Who is accessing Client? Do you need to keep its parent private, or should you expose that so that calling code can navigate to where the needed features are?

If you're worried about exposing parent completely, another option is to extract an interface from Parent and expose that from Client.