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.
I don't think there's anything specifically wrong with it, it's just a matter of style. It's useful when:
ReplyDeleteYou 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.
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.
ReplyDeleteThis 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:
ReplyDeleteit'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.
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:
ReplyDeletelist.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!"
});
At least in theory, it can damage the optimization mechanisms of the JVM by setting false dependencies between calls.
ReplyDeleteIt 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.
I prefer using 'with' methods for this:
ReplyDeletepublic String getFoo() { return foo; }
public void setFoo(String foo) { this.foo = foo; }
public String withFoo(String foo) {
setFoo(foo);
return this;
}
Thus:
list.add(new Employee().withName("Jack Sparrow")
.withId(1)
.withFoo("bacon!"));
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.
ReplyDeleteBecause 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.
ReplyDeleteI don't know Java but I've done this in C++.
ReplyDeleteOther 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.
I used to prefer this approach but I have decided against it.
ReplyDeleteReasons:
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.
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.
ReplyDeleteOn first sight: "Ghastly!".
ReplyDeleteOn 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 to. In general, there is no dogma to say that this is good or bed, it comes from the situation of course.
ReplyDeleteI'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.
ReplyDeleteFrom the statement
ReplyDeletelist.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
ReplyDeletelist.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);