Is it bad practice to make a setter return "this"?

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();

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.

I prefer using 'with' methods for this:

public String getFoo() { return foo; }
public void setFoo(String foo) { this.foo = foo; }
public Employee withFoo(String foo) {
  setFoo(foo);
  return this;
}

Thus:

list.add(new Employee().withName("Jack Sparrow")
                       .withId(1)
                       .withFoo("bacon!"));

Warning: this withX syntax is commonly used to provide "setters" for immutable objects, so callers of these methods might reasonably expect them to create new objects rather than to mutate the existing instance. Maybe a more reasonable wording would be something like:

list.add(new Employee().chainsetName("Jack Sparrow")
                       .chainsetId(1)
                       .chainsetFoo("bacon!"));

With the chainsetXyz() naming convention virtually everyone should be happy.

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:

One mega constructor (downside: you might pass lots of nulls or default values, and it gets hard to know which value corresponds to what) Several overloaded constructors (downside: gets unwieldy once you have more than a few) 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.

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!"
});

It not only breaks the convention of getters/setters, it also breaks the Java 8 method reference framework. MyClass::setMyValue is a BiConsumer<MyClass,MyValue>, and myInstance::setMyValue is a Consumer<MyValue>. If you have your setter return this, then it's no longer a valid instance of Consumer<MyValue>, but rather a Function<MyValue,MyClass>, and will cause anything using method references to those setters (assuming they are void methods) to break.

I don't know Java but I've done this in C++. Other people have said it makes the lines really long and really hard to read, but I've done it like this lots of times:

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

This is even better:

list.add(
    new Employee("Jack Sparrow")
    .Id(1)
    .foo("bacon!"));

at least, I think. But you're welcome to downvote me and call me an awful programmer if you wish. And I don't know if you're allowed to even do this in Java.

At least in theory, it can damage the optimization mechanisms of the JVM by setting false dependencies between calls.

It is supposed to be syntactic sugar, but in fact can create side effects in the super-intelligent Java 43's virtual machine.

That's why I vote no, don't use it.

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();

Because it doesn't return void, it's no longer a valid JavaBean property setter. That might matter if you're one of the seven people in the world using visual "Bean Builder" tools, or one of the 17 using JSP-bean-setProperty elements.

This scheme (pun intended), called a 'fluent interface', is becoming quite popular now. It's acceptable, but it's not really my cup of tea.

If you use the same convention in whole applicaiton it seems fine.

On the oher hand if existing part of your application uses standard convention I'd stick to it and add builders to more complicated classes

public class NutritionalFacts {
    private final int sodium;
    private final int fat;
    private final int carbo;

    public int getSodium(){
        return sodium;
    }

    public int getfat(){
        return fat;
    }

    public int getCarbo(){
        return carbo;
    }

    public static class Builder {
        private int sodium;
        private int fat;
        private int carbo;

        public Builder sodium(int s) {
            this.sodium = s;
            return this;
        }

        public Builder fat(int f) {
            this.fat = f;
            return this;
        }

        public Builder carbo(int c) {
            this.carbo = c;
            return this;
        }

        public NutritionalFacts build() {
            return new NutritionalFacts(this);
        }
    }

    private NutritionalFacts(Builder b) {
        this.sodium = b.sodium;
        this.fat = b.fat;
        this.carbo = b.carbo;
    }
}

Paulo Abrantes offers another way to make JavaBean setters fluent: define an inner builder class for each JavaBean. If you're using tools that get flummoxed by setters that return values, Paulo's pattern could help.

I'm in favor of setters having "this" returns. I don't care if it's not beans compliant. To me, if it's okay to have the "=" expression/statement, then setters that return values is fine.

I used to prefer this approach but I have decided against it.

Reasons:

Readability. It makes the code more readable to have each setFoo() on a separate line. You usually read the code many, many more times than the single time you write it.

Side effect: setFoo() should only set field foo, nothing else. Returning this is an extra "WHAT was that".

The Builder pattern I saw do not use the setFoo(foo).setBar(bar) convention but more foo(foo).bar(bar). Perhaps for exactly those reasons.

It is, as always a matter of taste. I just like the "least surprises" approach.

Yes, I think it's a good Idea.

If I could add something, what about this problem :

class People
{
    private String name;
    public People setName(String name)
    {
        this.name = name;
        return this;
    }
}

class Friend extends People
{
    private String nickName;
    public Friend setNickName(String nickName)
    {
        this.nickName = nickName;
        return this;
    }
}

This will work :

new Friend().setNickName("Bart").setName("Barthelemy");

This will not be accepted by Eclipse ! :

new Friend().setName("Barthelemy").setNickName("Bart");

This is because setName() returns a People and not a Friend, and there is no PeoplesetNickName.

How could we write setters to return SELF class instead of the name of the class ?

Something like this would be fine (if the SELF keyword would exist). Does this exist anyway ?

class People
{
    private String name;
    public SELF setName(String name)
    {
        this.name = name;
        return this;
    }
}

This particular pattern is called Method Chaining. Wikipedia link, this has more explanation and examples of how it's done in various programming languages.

P.S: Just thought of leaving it here, since I was looking for the specific name.

On first sight: "Ghastly!".

On further thought

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

is actually less error prone than

Employee anEmployee = new Employee();
anEmployee.setName("xxx");
...
list.add(anEmployee);

So quite interesting. Adding idea to toolbag ...

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.

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.

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); 

I have been making my setters for quite a while and the only real issue is with libraries that stick with the strict getPropertyDescriptors to get the bean reader/writer bean accessors. In those cases, your java "bean" will not have the writters that you would expect.

For example, I have not tested it for sure, but I would not be surprised that Jackson won't recognizes those as setters when creating you java objects from json/maps. I hope I am wrong on this one (I will test it soon).

In fact, I am developing a lightweight SQL centric ORM and I have to add some code beyong getPropertyDescriptors to recognized setters that returns this.

Long ago answer, but my two cents ... Its fine. I wish this fluent interface were used more often.

Repeating the 'factory' variable does not add more info below:

ProxyFactory factory = new ProxyFactory();
factory.setSuperclass(Foo.class);
factory.setFilter(new MethodFilter() { ...

This is cleaner, imho:

ProxyFactory factory = new ProxyFactory()
.setSuperclass(Properties.class);
.setFilter(new MethodFilter() { ...

Of course, as one of the answers already mentioned, the Java API would have to be tweaked to do this correctly for some situations, like inheritance and tools.

It is better to use other language constructs if available. For example, in Kotlin, you would use with, apply, or let. If using this approach, you won't really need to return an instance from your setter.

This approach allows your client code to be:

Indifferent to the return type

Easier to maintain

Avoid compiler side effects

Here are some examples.

val employee = Employee().apply {
   name = "Jack Sparrow"
   id = 1
   foo = "bacon"
}


val employee = Employee()
with(employee) {
   name = "Jack Sparrow"
   id = 1
   foo = "bacon"
}


val employee = Employee()
employee.let {
   it.name = "Jack Sparrow"
   it.id = 1
   it.foo = "bacon"
}

If I'm writing an API, I use "return this" to set values that will only be set once. If I have any other values that the user should be able to change, I use a standard void setter instead.

However, it's really a matter of preference and chaining setters does look quite cool, in my opinion.

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.

Bad habit: a setter set a getter get

what about explicitly declaring a method, that does it for U

setPropertyFromParams(array $hashParamList) { ... }