I recently used a library that allows the following type of syntax:
MyClass myObject;
myObject
.setMember1("string value")
.setMember2(4.0f)
.setMember3(-1);
Obviously this is accomplished by having the setters return a MyClass & type; something like return *this. I like the way this code looks, but I don't see it a lot. When that happens I'm usually suspicious as to why.
So, is this a bad practice? What are some of the implications of doing it like this?
This is sometimes referred to as the Named Parameter Idiom or method chaining. This isn't bad practice, it can aid readability. Consider this example lifted from the C++ FAQ
File f = OpenFile("foo.txt")
.readonly()
.createIfNotExist()
.appendWhenWriting()
.blockSize(1024)
.unbuffered()
.exclusiveAccess();
The alternative would be to use positional arguments to the OpenFile method, requiring the programmer to remember the position of each argument.
Some people call this fluent programming (or a fluent interface). Others call it a mess.
I tend somewhat toward the latter camp. In particular, my experience has been that in many cases, people writing code this way depend on the "fluent interface" for quite a bit of initializing an object. In other words, despite the disguise, it's still two-step initialization. Likewise, although it's probably avoidable in many cases, it frequently seems to result in quite a bit of what should be entirely private to the class being made publicly modifiable via manipulators.
Personally I prefer that objects are immutable after creation. That's clearly not always possible, and in some cases you can't even come very close. Nonetheless, the more of an object's internals that you make open to outside manipulation, the less certain you become about that object maintaining a coherent state (and, typically, the more work you have to do to maintain a coherent state).
Your example is not the named parameters idiom.
With the named parameters idiom, the chainable setters set attributes of an argument (parameter) pack.
Your code instead has a bunch of setters for modifying the final object you're constructing, called two-phase construction.
In general two-phase construction is just Bad™, and two-phase construction implemented by exposing attributes to client code, as in your example, is Very Bad™.
For example, you do not, in general, want to be able to modify a file object's attributes, once that object has been constructed (and possibly file opened).
Cheers & hth.,
It's a common practice. The overloading of operator= implies to do so to chain calls:
class Foo {
public:
Foo& operator=(const Foo& f) {
if (this != &f) { // check for self-assignment
// do some stuff...
}
return *this;
}
};
This code allows to do things like:
Foo a, b, c;
a = b = c;
Please note that checking for self-assignment is compulsory because you often have to deallocate stuffs in the current object, so allowing a = a would break your code.
Following @Fred Nurk's comment, I'd like to add that you should have a look at the Copy-and-Swap idiom in order to avoid code duplication and issue an exception-free code.
Have a look at the links hereunder for more information:
What is the copy-and-swap idiom?
http://gotw.ca/gotw/059.htm
There's no problem with this style. The only downside is that you can't use the return value for more typical purposes, like returning the result of the function.
It's called a fluent api. It's not bad practice, just a different style of programming.
The biggest drawbacks (IMO) would be that since you are returning a reference to yourself, you can't return anything else and it can be difficult to debug fluent statements since they are seen by the compiler as one giant "line" of code.
I'm not sure if it's considered bad practice or not, but one implication is that you can no longer return error codes, so you're either forced to use exceptions or ugly error objects passed in by reference. Exceptions are sometimes the right solution, but often aren't since they can be expensive to throw and are disabled on some platforms.
I really think it's a stylistic thing though, and because of C++'s close relationship with C both in syntax and culture, many C++ programmers like error codes and so will prefer returning them instead of returning a reference. But I've seen return of *this often as well and I don't think it's bad practice.
Not bad practice, in fact you see it often with output streams in order to concatenate multiple strings and values.
Only disadvantage that I see is that it prevents you from returning anything else, though if it's a set method, it shouldn't matter.
