← Back to Upcase

How would you improve this small presenter (or would you)?


(Bruce) #1

Hi, so let’s say I have a Rails 4.2 app that has a search box that lets you search for “Flying” or “All” kites. Once you get to the search results page, a “horizontal navigation menu” appears with the 2 options “Flying” or “All”. Whichever type of kite you searched for is the option in the menu is not hyperlinked, the one that you didn’t search for does. This lets you “change” your search parameters by clicking on the hyperlink.

This basically means i need to dynamically generate the html needed to make this happen, or I need to have a lot of if/else in the view. Hence, I chose the presenter pattern. Is the following presenter the right approach? Is there anything you would improve?

I do this via a presenter:

class KiteMenuPresenter
  include Rails.application.routes.url_helpers
  include ActionView::Helpers::UrlHelper

  def initialize(search_header_menu = {left: "Flying Kites", right: "All Kites"}, options)
    @search_header_menu = search_header_menu
    @options = options
  end

  def generate_view
    @menu_str = nil
    if @options[:kite_status]=="Flying"
      @menu_str = <<-EOS
        <li><h2>#{@search_header_menu[:left]}</h2></li>
        <li>#{link_to @search_header_menu[:right], kite_search_path( offset: @options[:offset], limit: @options[:limit], kite_status: "", search_terms: @options[:search_terms])}</li>
      EOS
    else
      @menu_str = <<-EOS
        <li>#{link_to @search_header_menu[:left], kite_search_path( offset: @options[:offset], limit: @options[:limit], kite_status: "Flying", search_terms: @options[:search_terms])}</li>
        <li><h2>#{@search_header_menu[:right]}</h2></li>
      EOS
    end
  end
  
end

HTML (HAML) containing presenter:

%section
  %ul
    %li
      Kites:
    =@kite_menu_presenter.generate_view.html_safe

Final HTML output

<section>
<ul>
<li>
Kites:
</li>
        <li><h2>Flying Kites</h2></li>
        <li><a href="/kite_search?limit=15&amp;offset=0&amp;kite_status=&amp;search_terms=&amp">All Kites</a></li>

</ul>
</section>

(Andy Waite) #2

Here’s my take on it:

In the if/else statement, you’re branching based on @options[:kite_status], which is essentially a type. Whenever you see that happen in OO code, it’s often a sign that it could be better structured.

Also, HTML markup within a class tends to be awkward to format, difficult to read, and prevents you from using HAML, so let’s try move that too.

I would start by extracting the markup for each branch into its own partial: _flying.html.haml and _all.html.haml.

You can then add a method to the presenter which tells Rails which partial to use for rendering:

def to_partial_path
  @options[:kit_status].downcase # will return `flying` or `all`
end

The whole generate_view method is then no longer needed, and you can update the view to render the appropriate partial:

%section
  %ul
    %li
      Kites:
    = render @kite_menu_presenter # will render based on `to_partial_path`

(You may need to pass some additional parameters to the render method.)

After this, you will likely see other ways to improve the code, but it’s good to work in small incremental steps where possible.


(Bruce) #3

Thank you Andy, this solution worked great!