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

Question

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

public Employee setName(String name){
   this.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...
list.add(e);

...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.

Update:

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.

Answer 1

If you don't want to return 'this' from the setter but don't want to use the second option you can use the following syntax to set properties:

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

As an aside I think its slightly cleaner in C#:

list.Add(new Employee() {
    Name = "Jack Sparrow",
    Id = 1,
    Foo = "bacon!"
});

Answer 2

It's not a bad practice at all. But it's not compatiable with JavaBeans Spec.

And there is a lot of specification depends on those standard accessors.

You can always make them co-exist to each other.

public class Some {
    public String getValue() { // JavaBeans
        return value;
    }
    public void setValue(final String value) { // JavaBeans
        this.value = value;
    }
    public String value() { // simple
        return getValue();
    }
    public Some value(final String value) { // fluent/chaining
        setValue(value);
        return this;
    }
    private String value;
}

Now we can use them together.

new Some().value("some").getValue();

Here comes another version for immutable object.

public class Some {

    public static class Builder {

        public Some build() { return new Some(value); }

        public Builder value(final String value) {
            this.value = value;
            return this;
        }

        private String value;
    }

    private Some(final String value) {
        super();
        this.value = value;
    }

    public String getValue() { return value; }

    public String value() { return getValue();}

    private final String value;
}

Now we can do this.

new Some.Builder().value("value").build().getValue();

Answer 3

In general it’s a good practice, but you may need for set-type functions use Boolean type to determine if operation was completed successfully or not, that is one way too. In general, there is no dogma to say that this is good or bed, it comes from the situation, of course.

Answer 4

This may be less readable

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

or this

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

This is way more readable than:

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

Answer 5

From the statement

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

i am seeing two things

1) Meaningless statement. 2) Lack of readability.

Answer 6

I agree with all posters claiming this breaks the JavaBeans spec. There are reasons to preserve that, but I also feel that the use of this Builder Pattern (that was alluded to) has its place; as long as it is not used everywhere, it should be acceptable. "It's Place", to me, is where the end point is a call to a "build()" method.

There are other ways of setting all these things of course, but the advantage here is that it avoids 1) many-parameter public constructors and 2) partially-specified objects. Here, you have the builder collect what's needed and then call its "build()" at the end, which can then ensure that a partially-specified object is not constructed, since that operation can be given less-than-public visibility. The alternative would be "parameter objects", but that IMHO just pushes the problem back one level.

I dislike many-parameter constructors because they make it more likely that a lot of same-type arguments are passed in, which can make it easier to pass the wrong arguments to parameters. I dislike using lots of setters because the object could be used before it is fully configured. Further, the notion of having default values based on previous choices is better served with a "build()" method.

In short, I think it is a good practice, if used properly.