Domhnall Murphy

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.