How to clean up messy views and controllers in Rails?

Question

I've got a lot of code like this in my app:

if @document.template.name == "Newsletter"
  ...
end

Which I realise is poor and ugly code. I'm not sure though what alternatives exist for this kind of code. Are there any best practices for it? I hope so. Cheers!

Sample controller code

In this controller code sample it posts an image to Twitter if the name is "Newsletter". I know it's messy, and that a lot of the code should be moved to the model. I'm more concerned about the conditional though.

if @document.template.name == "Newsletter"
  source = Magick::Image.read(@document.component.image_newsletter.path).first
  overlay = Magick::Image.read(@document.user.logo.path).first.resize_to_fit(source.columns)
  rec = Magick::Draw.new
  rec.stroke = "##{@document.user.colour1}"
  rec.fill = "##{@document.user.colour1}"
  rec.rectangle 0, 0, source.rows, 5
  lank = source.extent(source.columns, source.rows+overlay.rows, 0 ,0)
  combo = lank.composite(overlay, Magick::SouthGravity, 0, 0, Magick::OverCompositeOp)
  rec.draw(combo)
  client.update_with_media("#{@document.title}: #{@document.remove_html(@document.components.first.body[0..100])}...", open(combo.to_blob))
else
  client.update("#{@document.title}: #{@document.remove_html(@document.components.first.body[0..100])}... http://domain.com#{share_path(@document.user.ftp, @document)}")
end

Answer 1

Presenter Pattern to the rescue

app/helpers/application_helper.rb

This will give you convenient access to instantiate a presenter anywhere in any of your views.

Example, if you use present @document it will instantiate a DocumentPresenter.

module ApplicationHelper
  def present object, klass = nil
    klass ||= "#{object.class}Presenter".constantize
    presenter = klass.new object, self
    yield presenter if block_given?
    presenter
  end
end

To override the presenter used, you can do present @document, MyPresenter

---

app/presenters/document.rb

Your actual presenter. Create as many instance methods as you like and keep all of the view logic in here. You have access to all view helper methods through @template

class DocumentPresenter
  def initialize document, template
    @document = document
    @template = template
  end

  def name
    if @document.template.name == "Newsletter"
      # for example ...
      @template.link_to 'Newsletter', @template.document_index_path
    end
  end

  def description
    @template.content_tag :p, @document.description, class: "description"
  end

end

---

app/views/document/show.html.erb

<% present @document do |document_presenter| %>
    <div id="document">
      <%= document_presenter.description %>
      <%= document_presenter.name %>
    </div>
<% end %>

---

Result

<div id="document">
  <p class="description">
    lorem ipsum
  </p>
  <a href="/newsletters">Newsletters</a>
</div>

You can learn more about the Presenter Pattern as done by Ryan Bates in his RailsCast episode "Presenters from Scratch"

Answer 2

The only alternative I can think of presently is to move the template-specific code in to the Template model, separated in to individual methods which follow a particular naming convention.

For example, your methods could follow the convention process_x, where x is the name of the template. In this case, the code you posted for the "newsletter" would be in a method called process_newsletter.

I would also create a single point of entry, lets call it process, in the same model, which is responsible for delegating to one of these methods, like so:

class Template < ActiveRecord::Base
    ... other model code

    def process # this is the method to be called from the controller
        method_name = "process_#{self.name}" # the name of the method to be called
        send method_name # call a method by this name
    end

    def process_newsletter
        # your newsletter code already posted
    end

    def process_article # another example for illustration purposes
       # article specific code
    end
end

This not only eliminates the need for template name checking, but also helps to further separate your code, and moves any model-specific stuff away from the controller.