Let's say we are modelling London's Boris Bikes system in Ruby.
Here's a typical method that we might have in our
DockingStation class (N.B.
@bikes is an array):
def dock(bike) fail 'Docking station full' if full? @bikes << bike end
Seems harmless enough right? But there is a subtle problem.
The command-query separation principle states that a method should either perform an action or return a value to the caller, but not both.
dock is a command, so it's easy to overlook that it also returns a value. Because all Ruby methods return a value. In this case
dock implicitly returns the result of the last line evaluated in the method:
@bikes << bike.
In a Ruby array, the shovel method (
<<) returns the array on which it is called. So in our case,
@bikes << bike evaluates to
dock implicitly returns
Uh oh. We've just exposed the internal implementation of our bikes collection. He he, I can now fill up my docking station with elephants (N.B.
docking_station is an instance of
bikes = docking_station.dock(Bike.new) 100.times do bikes << 'Elephant!' end
To prevent this, we need to ensure a different value is returned from the
dock method. But which value? There are four reasonable candidates:
nilbecause this is a command method and the return value should be irrelevant.
trueto convey that the bike was successfully docked.
bikeas this is the object that was docked.
selfwhich enables method chaining and might be considered a reasonable default.
nil seems like a reasonable candidate (as it's as close as you can get to
void in Ruby), on closer consideration,
nil is falsey, so the following naive conditional would fail:
if docking_station.dock(Bike.new) # some action conditional on successfully docking a bike end
Of course, the above is a code smell, but it's entirely possible.
For the reason stated above,
true might be a better choice. Indeed, it seems helpful to convey that the dock was successful. However, this introduces another subtle design smell. In the case that a bike cannot be docked (e.g. the docking station is full), an exception is raised. In the case that a bike can be docked, we return
true. This is inconsistent.
This introduces another ideological debate about whether raising an exception in this case is also a design smell. Preventing bikes from being docked into a full docking station is domain logic and therefore not exceptional. Whichever argument you subscribe to, it's clear that a method which returns
true, should conversely return
false. Personally, I'm happy that
dockraises an exception when the station is full. In the domain, docking a bike into a full docking station is not defined, so I do consider it exceptional. However it is only acceptable if we also provide an opportunity to avoid the exception by testing the docking station first; which we can do with
That leaves us with
The argument in favour of
bike is twofold:
- This is the default behaviour of
attr_writer) in Ruby. When calling an attribute setter in Ruby, the method returns the new value of the attribute (which is the passed parameter). So for example, in IRB (given a class
> test = Test.new => #<Test:...> > test.name = 'Ben' => "Ben" > test.name = :some_value => :some_value
- It provides some convenience in our tests.
bike = Bike.new docking_station.dock bike expect(docking_station).to include bike
we can have:
bike = docking_station.dock Bike.new expect(docking_station).to include bike
The argument against appeals to methods with multiple arguments; which one should be returned? Perhaps all of them should be? But that's getting messy.
Finally, we could return
self. This allows us to chain methods for convenience:
I'm undecided which of
self I prefer. Clearly, any of the four options is better than returning the internal array, but I'll leave the choice of which to you.