0
\$\begingroup\$

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
\$\endgroup\$
2
\$\begingroup\$

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).

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks, that’s exactly what I needed. I’ll go with the latter approach; it’s nice to keep try out of the view. I don’t think it needs testing either. \$\endgroup\$ – Jezen Thomas Feb 4 '15 at 11:53
  • \$\begingroup\$ @JezenThomas #try has its uses in views too, though. It's very useful if you need localization for fallback values, for instance. I'd rather handle that in a view, e.g. <%= model.try(:foo) || t(".no_value") %> \$\endgroup\$ – Flambino Feb 4 '15 at 12:24
  • \$\begingroup\$ I think you've got the right idea that "having an entire decorator seems a bit much just to absorb nils", but maybe the best suggestion is to create a base decorator that absorbs nil by default for all methods unless overridden? Then that could be tested once and wouldn't need to be tested for every decorator. \$\endgroup\$ – Lee Feb 13 '15 at 17:37
  • \$\begingroup\$ @Lee It's tempting, but also somewhat blunt as it could camouflage actual errors. It could work of course, but personally I usually prefer to explicitly handle nils where they may be - and seeing things fail if there's a nil where it shouldn't be. \$\endgroup\$ – Flambino Feb 13 '15 at 18:19
1
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.