If you have read the title and thought, "Yeah, obviously!" then this post is probably not for you. But this fact caught me out recently and led to a few unexpected test failures, so I thought it worth highlighting.
Introduction
A ruby setter methods allow us to provide a custom implementation for
=
-assignments to object attributes. For example
class Foo
attr_reader :name
def initialize
@name = "default"
end
def name=(value)
@name = value.downcase
end
end
These methods are a very useful tool and can be used to great effect in making the client code a lot simpler.
foo = new Foo()
foo.name # default
foo.name = "UPDATED"
foo.name # updated
Beautiful.
The Context
The situtation arose within a Rails model. The lesson is not specific to Rails, but I will introduce it in a Rails context, as this provides the motivation for the change. So the story starts with a pretty innocuos-looking method
class Fing < ApplicationRecord
has_one :ova_fing
…
def set_ova_fing(name)
self.ova_fing = OvaFing.create(name: name)
self.ova_fing
end
…
end
This worked fine and somewhere in a controller we would have called this method, thusly:
class OvaFingsController < ApplicationController
def update
@fing = get_fing(params[:fing_id])
render json: @fing.set_ova_fing(params[:ova_fing_name])
end
end
The controller method creates a new OvaFing
record, assigns it to
the @fing.ova_fing
association and return an JSON representation of the
OvaFing
just created.
And buried in the tests for Fing
model we could have something similar to this:
…
it "should create an OvaFing record" do
orig_count = OvaFing.count
@fing.set_ova_fing("george")
expect(OvaFing.count).to eq orig_count+1
end
it "should set the :ova_fing association" do
expect(@fing.ova_fing).to be_nil
@fing.set_ova_fing("george")
expect(@fing.reload.ova_fing).not_to be_nil
end
it "should return an OvaFing instance" do
expect(@fing.set_ova_fing("george")).to be_a OvaFing
end
…
The Problem
In a future iteration we need to update a Fing
, along with the associated
OvaFing
, from the FingsController
:
class FingsController < ApplicationController
def update
@fing = get_fing(params[:fing_id])
@fing.update(fing_params)
@fing.set_ova_fing(params[:ova_fing_name]) # Can we get rid of this using a setter method??
end
def fing_params
params.require(:fing).permit(… :ova_fing_name)
end
end
In seeing this code we would like to have a single update statement that is able to
handle all of the params that we throw at it, including the ova_fing_name
.
We see what looks like a simple solution, by simply updating the Fing
model like so:
class Fing < ApplicationRecord
has_one :ova_fing
…
def ova_fing_name=(name)
self.ova_fing = OvaFing.create(name: name)
self.ova_fing
end
…
end
where we have just replaced the method name :set_ova_fing
to be a
setter method: :ova_fing_name=
.
We will obviously need to update the OvaFingsController#update
which also uses
this method:
class OvaFingsController < ApplicationController
def update
@fing = get_fing(params[:fing_id])
render json: @fing.ova_fing_name=(params[:ova_fing_name]) # Here is the problem
end
end
And herein lies the problem. Our :set_ova_fing
method was constructed to return
the OvaFing
instance created, and the client code relied on this fact.
The method body is still the exact same as before, but the setter method will always return
the value passed to it, i.e. the String parameter, params[:ova_fing_name]
.
This is the expected behaviour and is stated clearly in the ruby docs
Note that for assignment methods the return value will be ignored when using the assignment syntax. Instead, the argument will be returned:def a=(value) return 1 + value end p(self.a = 5) # prints 5
We can resolve the issue simply in one of two ways, but perhaps only after a bit of head-scratching about why this innocuous search-and-replace on a method name has caused a bunch of test failures!
First, we could just set the
@fing.ova_fing_name
in one line, and retrieve the newly created
OvaFing
right after:
class OvaFingsController < ApplicationController
def update
@fing = get_fing(params[:fing_id])
@fing.ova_fing_name = params[:ova_fing_name]
render json: @fing.ova_fing
end
end
Alternatively, we can use :send
to call the setter method directly, and
ensure the explicit return statement is honoured. But, personally, I think this kind of
undermines the readability motivation for using the setter method in the first place:
class OvaFingsController < ApplicationController
def update
@fing = get_fing(params[:fing_id])
render json: @fing.send(:ova_fing_name=, params[:ova_fing_name])
end
end
The Lesson
Not so much a lesson here, just a warning. If you are switching an existing method to become a setter method, be careful that you are not relying on a return value different from the value passed into the setter. The setter method will always return the value that has been passed into it.