10

I have code that looks like this:

tr.t.findIndexSmoothed(arg0.getX(), arg0.getY());

"tr" and "t" are objects. Is it bad practice to reach all the way down the object hierarchy to call methods? The only reason I can think of is that is breaks encapsulation, and if that's the case, can anybody tell me why that poses a problem? Also, does this code structure inhibit performance in any way?

Improve this question
3
  • 1
    If the t member inside the tr object is reachable (public) then you are not breaking encapsulation: the purpose of making a member (field or method) public is to allow direct access to it. Commented Aug 18, 2013 at 18:40
  • Yes it is correct. Though if you abuse it, it can hinder readability and debugging (you get a NullPointerException, which is null, tr, tr.tor arg0?) Commented Aug 18, 2013 at 19:58
  • The answers here, as well as the answers on the question this is considered a duplicate of, overlook one key point: if tr.t's signature is changed from whatever class is currently used, to a newly defined interface, then the "over-coupling" is broken. Clients would know what they can do. tr can make any needed changes in future. It is the public exposure of a property as a class rather than a more limited interface, that is the root of the problem. (Typically: make t private, perhaps as _t, then add a getter that exposes t, but using an interface type.) Commented Feb 4, 2017 at 0:33

2 Answers 2

Reset to default
12

The Law of Demeter suggests that you shouldn't call methods on objects two layers down like that.

The idea is that, in your case, you should have a method findIndexSmoothed on tr which in turn calls findIndexSmoothed on t.

What this does is makes your code adaptable. It means that the nature of tr.findIndexSmoothed can change without necessarily changing the nature of t.findIndexSmoothed, which may be called by other code you don't intend to change.

However, it is commonly noted that it's pretty rare you really need that adaptability, and you'll have put yourself in a position where maintenance of t.findIndexSmoothed now requires maintenance of tr.findIndexSmoothed so, in some cases, you will increase your workload.

So, the answer to your question, as so many style questions in programming, is: Use your common sense. Keep the Law of Demeter in mind, but never make it an inflexible truth.

Improve this answer
12
  • Also you dont really have a choice when using apis and frameworks. It is just not worth it. So sometimes you will have chains of getX().getY().getZ() Commented Aug 19, 2013 at 6:32
  • The linked article about LOD seems to ignore a fundamental issue: ownership. If a piece of code owns an object (typically by holding the only reference to it anywhere in the universe) and the object provides a place for its owner to store something, that place would be owned by the owner of the object containing it, and that owner would be entitled to use it just as it would one of its own fields. Commented Dec 17, 2014 at 22:55
  • @supercat: If I'm understanding what you mean, that's exactly what LoD advises you not to do. Each object should have only one owner, which should pass it to child objects as method parameters or expose methods that allow parent objects to access the "grandchild" indirectly. Again, I'm not arguing for the LoD as a hard-and-fast rule here, I'm explaining why there's no mention of multiple ownership in an article about the Law of Demeter -- because the two things are orthogonal. Commented Dec 18, 2014 at 18:46
  • @pdr: My my understanding, the main purpose of the Law of Demeter is to ensure that any change to some aspect of an object's state should go through the object in question. If a List<Car> holds one element--a reference to the 592nd object created since startup, which is a Car whose Color is "blue", then the state encapsulated by that List<Car> is the fact that item #0 holds a reference to Object #592. Code which owns the List<Car> might care about the fact that Object #592 has a Color of "blue", but that information is not part of the state of the List<Car>. Commented Dec 18, 2014 at 19:04
  • 1
    @pdr: Feel free. IMHO one of the great weaknesses of Java is that it only has one type of reference and does not distinguish whether any given reference encapsulates identity, mutable state, both, or neither. While it would be excessively expensive for a framework to support C++'s full litany of copy constructors etc. I think a good language should distinguish between fields which identify things whose state is part of that of the object holding the field, versus those which identify things whose state is owned by someone else. If the language used . for one and -> for the other, then... Commented Dec 19, 2014 at 16:55
0

I think there is nothing wrong with it as long as it makes sense from the design point of view. Just ask yourself those questions:

  1. Is it sensible for tr to contain and expose t?
  2. Is findIndexSmoothed() the responsibility of t?

If you answer yes to both of the questions then it is a way to go. Only thing I would do differently is to make t private and access it via getter.

Also I think it actually makes sense with the single responsibility principle. What if t had 5 different methods? would you delegate all of those from tr? how would you name them? findIndexSmoothedInTr()?

Improve this answer

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.