Is it bad practice to make a setter return “this”? [java]


It's not bad practice. It's an increasingly common practice. Most languages don't require you to deal with the returned object if you don't want to so it doesn't change "normal" setter usage syntax but allows you to chain setters together.

This is commonly called a builder pattern or a fluent interface.

It's also common in the Java API:

String s = new StringBuilder().append("testing ").append(1)
  .append(" 2 ").append(3).toString();


Is it a good or bad idea to make setters in java return "this"?

public Employee setName(String name){ = name;
   return this;

This pattern can be useful because then you can chain setters like this:

list.add(new Employee().setName("Jack Sparrow").setId(1).setFoo("bacon!"));

instead of this:

Employee e = new Employee();
e.setName("Jack Sparrow");
...and so on...

...but it sort of goes against standard convention. I suppose it might be worthwhile just because it can make that setter do something else useful. I've seen this pattern used some places (e.g. JMock, JPA), but it seems uncommon, and only generally used for very well defined APIs where this pattern is used everywhere.


What I've described is obviously valid, but what I am really looking for is some thoughts on whether this is generally acceptable, and if there are any pitfalls or related best practices. I know about the Builder pattern but it is a little more involved then what I am describing - as Josh Bloch describes it there is an associated static Builder class for object creation.

Is it bad practice to make a setter return “this”?

I don't think there's anything specifically wrong with it, it's just a matter of style. It's useful when:

  • You need to set many fields at once (including at construction)
  • you know which fields you need to set at the time you're writing the code, and
  • there are many different combinations for which fields you want to set.

Alternatives to this method might be:

  1. One mega constructor (downside: you might pass lots of nulls or default values, and it gets hard to know which value corresponds to what)
  2. Several overloaded constructors (downside: gets unwieldy once you have more than a few)
  3. Factory/static methods (downside: same as overloaded constructors - gets unwieldy once there is more than a few)

If you're only going to set a few properties at a time I'd say it's not worth returning 'this'. It certainly falls down if you later decide to return something else, like a status/success indicator/message.

To summarize:

  • it's called a "fluent interface", or "method chaining".
  • this is not "standard" Java, although you do see it more an more these days (works great in jQuery)
  • it violates the JavaBean spec, so it will break with various tools and libraries, especially JSP builders and Spring.
  • it may prevent some optimizations that the JVM would normally do
  • some people think it cleans code up, others think it's "ghastly"

A couple other points not mentioned:

  • This violates the principal that each function should do one (and only one) thing. You may or may not believe in this, but in Java I believe it works well.

  • IDEs aren't going to generate these for you (by default).

  • I finally, here's a real-world data point. I have had problems using a library built like this. Hibernate's query builder is an example of this in an existing library. Since Query's set* methods are returning queries, it's impossible to tell just by looking at the signature how to use it. For example:

    Query setWhatever(String what);
  • It introduces an ambiguity: does the method modify the current object (your pattern) or, perhaps Query is really immutable (a very popular and valuable pattern), and the method is returning a new one. It just makes the library harder to use, and many programmers don't exploit this feature. If setters were setters, it would be clearer how to use it.

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.

Conventionally, can a set() method return a value in Java?

Strictly conventionally - no, a setter usually returns void.

Having said that, you are free to return boolean if you wish - conventions are often broken (even in the internal java api's) and the method signature, including it's return type, should inspire an interested developer to navigate into the code to see exactly why a boolean is returned.

To make it clearer, you might want to use a different method name, e.g. setTiresIfAble(Tires tires), or you could alternatively return void and throw an Exception as per below:

 public void setTires(Tires tires){
     if(!hasAxels()) {
         throw new IllegalStateException("no axels!");
     mTires = tires;
     mHasTires = true;

Incidentally, mHasTires is redudant, you can always check if tires == null.

Lastly, you can avoid the m or Hungarian notation in java (as per convention), like this:

 public setAxels(boolean hasAxels){
      this.hasAxels = hasAxels;

A setSomething method shouldn't return anything

A trySetSomethingmust return a boolean which say if the set has been successful or not.

Why ? When you are writting some code, in Java, in C++, in any langage, you want that any reader of your code, which is probably a human being, can access to the knownledge of most of what a method do, just reading his name.

To complete this assertion, we can study the case that a set can fail. There are two possibilities to deal with failures, depending on the scope of the method :

  1. if the scope of setSomething is protected, private or package, it means that you have control, as API developper, on the way it will be called. Possible failures can be managed with assertion, or with RuntimeException (as it's not necessay to declare a throws clause in the method signature).

  2. if the scope of setSomething is public, it means that you don't have control, as API developper, on the way it will be call. You have to warn the users of your API that the setSomething isn't error-safe. You have to manage the possible failures with an Exception which has to be declared in a throw clause.