Testing a simple guard clause in isolation

Question

I’m using decorator objects in a Rails app so that I don’t have to do nil? checks in views. Everything is working nicely and it’s easy to test, but I’m suspicious that my test:code ratio is a little high. Am I being too verbose? Is there a better way this could be done?

Test suite:

# spec/decorators/client_decorator_spec.rb
require_relative "../../app/decorators/client_decorator"

describe ClientDecorator do
  describe ".position" do
    context "with a recorded client position" do
      it "returns the name of the position" do
        occupation = double("Occupation", name: "CEO")
        client = double("Client", occupation: occupation)
        expect(ClientDecorator.new(client).position).to eq "CEO"
      end
    end

    context "with no recorded position" do
      it "returns an empty string" do
        client = double("Client", occupation: nil)
        expect(ClientDecorator.new(client).position).to eq ""
      end
    end
  end
end

Production code:

# app/decorators/client_decorator.rb
require "simple_decorator"

class ClientDecorator < SimpleDecorator
  def position
    return "" unless occupation
    occupation.name
  end
end

Answer 1

Yeah, the code/test ratio is probably a little high - especially if you have to do this for many attributes.

May I direct your attention to the #try method? Seems to me you that you might be able to skip a lot of code (and tests) by doing things like @client.try(:occupation).try(:name).

#try simply returns nil if the receiver doesn't respond to the whatever message you're sending. And since #try is also implemented on the nil object, you can chain "tries" together.

In you views, writing something like <%= @client.try(:occupation).try(:name) %> will just print nothing if the try-chain ends up returning nil - same as your current setup. And if you want a different fallback, you can do this:

<%= @client.try(:occupation).try(:name) || "N/A" %>

In other words, having an entire decorator seems a bit much just to absorb nils. But granted, the view ends up having to dig a bit further into the models.

You can of course also keep the decorator (at least so you current code doesn't break elsewhere), but simply rewrite it using #try to something like

# app/decorators/client_decorator.rb
require "simple_decorator"

class ClientDecorator < SimpleDecorator
  def position
    occupation.try(:name) || ""
  end
end

which I honestly don't know if I'd bother to test very rigorously (lest it breaks horribly).

Answer 2

I dont believe that you are that verbose. The specs are covering the method implementation properly. Something that I would consider improving in your specs ir the repetitive parts of the code. You could DRY your spec with an approach like this:

describe ClientDecorator do    
  describe "#position" do
    subject { decorator.position }
    let(:decorator) { ClientDecorator.new(client) }
    let(:client) { double "Client", occupation: occupation }

    context "with a recorded client position" do
      let(:occupation) { double "Occupation", name: "CEO" }
      it "returns the name of the position" do
        expect(subject).to eq "CEO"
      end
    end

    context "with no recorded position" do
      let(:occupation) { nil }
      it "returns an empty string" do
        expect(subject).to be_blank
      end
    end
  end
end

Also take a look here about how to describe your methods (e.g. # for instance methods and . for class methods). Don't think of it of something as a rule, but rather than a best practise.