Why is chaining setters unconventional?

Question

Having chaining implemented on beans is very handy: no need for overloading constructors, mega constructors, factories, and gives you increased readability. I can't think of any downsides, unless you want your object to be immutable, in which case it would not have any setters anyway. So is there a reason why this isn't an OOP convention?

public class DTO {

    private String foo;
    private String bar;

    public String getFoo() {
         return foo;
    }

    public String getBar() {
        return bar;
    }

    public DTO setFoo(String foo) {
        this.foo = foo;
        return this;
    }

    public DTO setBar(String bar) {
        this.bar = bar;
        return this;
    }

}

//...//

DTO dto = new DTO().setFoo("foo").setBar("bar");

Answer 1

So is there a reason why isn't this a OOP convention?

My best guess: because it violates CQS

You've got a command (changing the state of the object) and a query (returning a copy of state -- in this case, the object itself) mixed into the same method. That's not necessarily a problem, but it does violate some of the basic guidelines.

For instance, in C++, std::stack::pop() is a command that returns void, and std::stack::top() is a query that returns a reference to the top element in the stack. Classically, you would like to combine the two, but you can't do that and be exception safe. (Not a problem in Java, because the assignment operator in Java doesn't throw).

If DTO were a value type, you might achieve a similar end with

public DTO setFoo(String foo) {
    return new DTO(foo, this.bar);
}

public DTO setBar(String bar) {
    return new DTO(this.foo, bar);
}

Also, chaining return values are a colossal pain-in-the- when you are dealing with inheritance. See the "Curiously recurring template pattern"

Finally, there's the issue that the default constructor should leave you with an object that is in a valid state. If you must run a bunch of commands to restore the object to a valid state, something has gone Very Wrong.

Answer 2

1. Saving a few keystrokes isn't compelling. It might be nice, but OOP conventions care more about concepts and structures, not keystrokes.

2. The return value is meaningless.

3. Even more than being meaningless, the return value is misleading, since users may expect the return value to have meaning. They may expect that it is an "immutable setter"

   public FooHolder {
       public FooHolder withFoo(int foo) {
           /* return a modified COPY of this FooHolder instance */
       }
   }
   

   In reality your setter mutates the object.

4. It doesn't work well with inheritance.

   public FooHolder {
       public FooHolder setFoo(int foo) {
           ...
       }
   }
   

   public BarHolder extends FooHolder {
       public FooHolder setBar(int bar) {
           ...
       }
   } 
   

   I can write

   new BarHolder().setBar(2).setFoo(1)
   

   but not

   new BarHolder().setFoo(1).setBar(2)
   

---

For me, #1 through #3 are the important ones. Well-written code is not about pleasantly arranged text. Well-written code is about the fundamental concepts, relationships, and structure. Text is only a outward reflection of code's true meaning.

Answer 3

I don't think this is an OOP convention, it's more related to the language design and its conventions.

It seems you like to use Java. Java has a JavaBeans specification which specifies the return type of the setter to be void, i.e. it is in conflict with chaining of setters. This spec is widely accepted and implemented in a variety of tools.

Of course you might ask, why isn't chaining part of the specification. I don't know the answer, maybe this pattern just wasn't known/popular at that time.

Answer 4

As other people have said, this is often called a fluent interface.

Normally setters are call passing in variables in response to the logic code in an application; your DTO class is a example of this. Conventional code when setters don’t return anything is normal best for this. Other answers have explained way.

However there are a few cases where fluent interface may be a good solution, these have in common.

• Constants are mostly passed to the setters
• Program logic does not change what is passed to the setters.

Setting up configuration, for example fluent-nhibernate

Id(x => x.Id);
Map(x => x.Name)
   .Length(16)
   .Not.Nullable();
HasMany(x => x.Staff)
   .Inverse()
   .Cascade.All();
HasManyToMany(x => x.Products)
   .Cascade.All()
   .Table("StoreProduct");

Setting up test data in unit tests, using special TestDataBulderClasses (Object Mothers)

members = MemberBuilder.CreateList(4)
    .TheFirst(1).With(b => b.WithFirstName("Rob"))
    .TheNext(2).With(b => b.WithFirstName("Poya"))
    .TheNext(1).With(b => b.WithFirstName("Matt"))
    .BuildList(); // Note the "build" method sets everything else to
                  // senible default values so a test only need to define 
                  // what it care about, even if for example a member 
                  // MUST have MembershipId  set

However creating good fluent interface is very hard, so it only worth it when you have lots of “static” setup. Also fluent interface should not be mixed in with “normal” classes; hence the builder pattern is often used.

Answer 5

I think much of the reason it's not a convention to chain one setter after another is because for those cases it's more typical to see an options object or parameters in a constructor. C# has an initializer syntax as well.

Instead of:

DTO dto = new DTO().setFoo("foo").setBar("bar");

One might write:

(in JS)

var dto = new DTO({foo: "foo", bar: "bar"});

(in C#)

DTO dto = new DTO{Foo = "foo", Bar = "bar"};

(in Java)

DTO dto = new DTO("foo", "bar");

---

setFoo and setBar are then no longer needed for initialization, and can be used for mutation later.

While chainability is useful in some circumstances, it's important to not try to stuff everything on a single line just for the sake of reducing newline characters.

For example

dto.setFoo("foo").setBar("fizz").setFizz("bar").setBuzz("buzz");

makes it harder to read and understand what's happening. Reformatting to:

dto.setFoo("foo")
    .setBar("fizz")
    .setFizz("bar")
    .setBuzz("buzz");

Is much easier to understand, and makes the "mistake" in the first version more obvious. Once you've refactored code to that format, there's no real advantage over:

dto.setFoo("foo");
dto.setBar("bar");
dto.setFizz("fizz");
dto.setBuzz("buzz");

Answer 6

That technique is actually used in the Builder pattern.

x = ObjectBuilder()
        .foo(5)
        .bar(6);

However, in general it is avoided because it is ambiguous. It is not obvious whether the return value is the object (so you can call other setters), or if the return object is the value that was just assigned (also a common pattern). Accordingly, the Principle of Least Surprise suggests you shouldn't try to assume the user wants to see one solution or the other, unless its fundamental to the object's design.

Answer 7

This is more of a comment than an answer, but I can't comment, so...

just wanted to mention that this question surprised me because I don't see this as uncommon at all. Actually, in my environment of work (web developer) is very very common.

For instance, this is how Symfony's doctrine:generate:entities command auto-generates all setters, by default.

jQuery kinda chains most of its methods in a very similar way.