What's wrong with returning this?

Question

At the company I work for there's a document describing good practices that we should adhere to in Java. One of them is to avoid methods that return this, like for example in:

class Properties {

  public Properties add(String k, String v) {
   //store (k,v) somewhere
     return this;
  }

}

I would have such a class so that I'm able to write:

 properties.add("name", "john").add("role","swd"). ...

I've seen such idiom many times, like in StringBuilder and don't find anything wrong with it.

Their argumentation is :

... can be the source of synchronization problems or failed expectations about the states of target objects.

I can't think of a situation where this could be true, can any of you give me an example?

EDIT The document doesn't specify anything about mutability, so I don't see the diference between chaining the calls and doing:

properties.add("name", "john");
properties.add("role", "swd");

I'll try to get in touch with the originators, but I wanted to do it with my guns loaded, thats' why I posted the question.

SOLVED: I got to talk with one of the authors, his original intention was apparently to avoid releasing objects that are not yet ready, like in a Builder pattern, and explained that if a context switch happens between calls, the object could be in an invalid state. I argued that this had nothing to do with returning this since you could make the same mistake buy calling the methods one by one and had more to do with synchronizing the building process properly. He admitted the document could be more explicit and will revise it soon. Victory is mine/ours!

"synchronization problems"? I can't imagine how that would come from this (pun not intended, but tolerated). Like pretty much all other patterns it has good uses and mis-uses, but I don't understand the reasoning you've been given. Maybe ask the authors of that document for clarifications. — Joachim Sauer, Apr 17 '13 at 12:48
There is another post about that on http://stackoverflow.com/questions/1345001/is-it-bad-practice-to-make-a-setter-return-this — Walid, Apr 17 '13 at 12:51
That argumentation is the sort of waffly, hollow nonsense I'd use if I wanted my (hypothetical) subordinates to just stop doing something I don't like but don't want to admit my reasons are 95% aesthetic. (See also: every single argument for or against OTBC / K&R style.) — millimoose, Apr 17 '13 at 13:02
If you did `buff.append("Hello, ").append("World\n")` in two threads where `buff` is a `StringBuffer` you could end up with "Hello, Hello, World\nWorld\n". However, there's not much point in `StringBuffer` having all the synchronisation, which tells you about the "good practice". (OTOH, returning `this` does indicate an issue with the language.) — Tom Hawtin - tackline, Apr 17 '13 at 13:07
For what it's worth, my opinion is this is a pattern that's very suitable for mutable objects you're also expected to mutate extensively in common use. (E.g. `StringBuilder`.) Builders that go through a series of immutable internal states are great, but not every single class needs to be made threadsafe that way, and every mutable class is a source of "synchronisation problems" no matter whether its API is fluent. — millimoose, Apr 17 '13 at 13:07
@TomHawtin-tackline That's the basis of my main retort to questions of the form "Is this threadsafe?" Synchronization issues are caused by the interaction of all uses of a single instance of shared state, looking at a single method / class (or a single statement as in this case) is myopic. — millimoose, Apr 17 '13 at 13:13
@millimoose Certainly it looks like a removing-spaces-to-prevent-race-conditions sort of a mindset. :) — Tom Hawtin - tackline, Apr 17 '13 at 13:23
@millimoose Aesthetics are important for maintainable code, so I would submit that aesthetics is a good reason for doing/not doing something. And, instead of chaining Add() methods, why not just have the method take a list of objects? — Bob Horn, Apr 17 '13 at 14:19
@BobHorn The problem with aesthetics is that at some point you're wasting energy on a bikeshed argument. In my experience, what makes a "real world" codebase *unmaintainable* is convoluted architecture, too many interns, workarounds for bugs that had to be fixed during crunch-time all-nighters, having to work with that obscure CMS the client bought ten years ago that comes with documentation that was Google-Translated from French... Purely/mostly aesthethic concerns like are ultimately insignificant and simply do not affect maintainability in practice. It's missing the forest for the trees. — millimoose, Apr 17 '13 at 18:45
@BobHorn As for your latter suggestion: there might be several types of "addable" objects whose implementation you don't control and that don't share a common abstract interface that you could code against. — millimoose, Apr 17 '13 at 18:47
@BobHorn To put my point about maintainability differently: if you *really* care about it, you should do code reviews and all the other hard-time consuming stuff. Being a bully about code style is "maintainability theater", it will do roughly as much for the defect rate and the time needed to fix them as the TSA making people remove their shoes did to prevent terrorist attacks. — millimoose, Apr 17 '13 at 18:55
I think you have some good points. I also think you have a bit of a straw man argument going as it pertains to me. I'm not suggesting debates about where braces go. I simply think that clean code is easier to maintain than ugly code. Anyway, we're off on a different topic. — Bob Horn, Apr 17 '13 at 18:59

score 6 · Answer 1 · answered Apr 17 '13 at 12:47

6

My guess is that they are against mutable state (and often are rightly so). If you are not designing fluent interfaces returning this but rather return a new immutable instance of the object with the changed state, you can avoid synchronization problems or have no "failed expectations about the states of target objects". This might explain their requirement.

answered Apr 17 '13 at 12:47

Kristof Jozsa

6,612
4
28
32

Could you please explain, how returning this from an instance method affects immutability? What's so bad about returning a references to an immutable object? – Mikhail Apr 18 '13 at 14:11

score 5 · Accepted Answer · answered Apr 17 '13 at 12:56

The only serious basis for the practice is avoiding mutable objects; the criticism that it is "confusing" and leads to "failed expectations" is quite weak. One should never use an object without first getting familiar with its semantics, and enforcing constraints on the API just to cater for those who opt out of reading Javadoc is not a good practice at all— especially because, as you note, returning this to achieve a fluent API design is one of the standard approaches in Java, and indeed a very welcome one.

score 1 · Answer 3 · answered Apr 17 '13 at 12:56

I think sometimes this approach can be really useful, for example in 'builder' pattern.

I can say that in my organization this kind of things is controlled by Sonar rules, and we don't have such a rule.

Another guess is that maybe the project was built on top of existing codebase and this is kind of legacy restriction.

So the only thing I can suggest here is to talk to the people who wrote this doc :)

Hope this helps

score 0 · Answer 4 · answered Apr 17 '13 at 14:42

I think it's perfectly acceptable to use that pattern in some situations.

For example, as a Swing developer, I use GridBagLayout fairly frequently for its strengths and flexibility, but anyone who's ever used it (with it's partener in crime GridBagConstraints) knows that it can be quite verbose and not very readable.

A common workaround that I've seen online (and one that I use) is to subclass GridBagConstraints (GBConstraints) that has a setter for each different property, and each setter returns this. This allows for the developer to chain the different properties on an as-needed basis.

The resultant code is about 1/4 the size, and far more readable/maintainable, even to the casual developer who might not be familiar with using GridBagConstaints.

What's wrong with returning this?

4 Answers4