Refactoring: An Anticipation

‹prev | My Chain | next›

After getting an all-recipes link working yesterday, I noticed that that pagination was a little... disturbing:



Unfortunately, this means refactoring because the pagination helper is already longish:

    def pagination(query, results)
      total = results['total_rows']
      limit = results['limit']
      skip  = results['skip']

      last_page    = (total + limit - 1) / limit
      current_page = skip / limit + 1

      link = "/recipes/search?q=#{query}"

      if results['sort_order']
        link += "&sort=#{results['sort_order'].first['field']}"
        if results['sort_order'].first['reverse']
          link += "&order=desc"
        end
      end

      links = []

      links <<
        if current_page == 1
          %Q|<span class="inactive">« Previous</span>|
        else
          %Q|<a href="#{link}&page=#{current_page - 1}">&laquo; Previous</a>|
        end

      links << (1..last_page).map do |page|
        if page == current_page
          %Q|<span class="current">#{page}</span>|
        else
          %Q|<a href="#{link}&page=#{page}">#{page}</a>|
        end
      end

      links <<
        if current_page == last_page
          %Q|<span class="inactive">Next »</span>|
        else
          %Q|<a href="#{link}&page=#{current_page + 1}">Next &raquo;</a>|
        end

      %Q|<div class="pagination">#{links.join}</div>|
    end

First up, I DRY up this code. The links to various pages are all done the same way:

    def page_link(link, page, text=nil)
      %Q|<a href="#{link}&page=#{page}">#{text || page}</a>|
    end

The edge cases are also similar. If we are at an edge (the first or last page), then the previous / next links need to be disabled. This can be extracted out into and edge_page_link helper:

    def edge_page_link(disabled, link, page, text)
      disabled ?
        %Q|<span class="inactive">#{text}</span>| :
        page_link(link, page, text)
    end

Using these methods, the original helper method gets smaller:

    def pagination(query, results)
      total = results['total_rows']
      limit = results['limit']
      skip  = results['skip']

      last_page    = (total + limit - 1) / limit
      current_page = skip / limit + 1

      link = "/recipes/search?q=#{query}"

      if results['sort_order']
        link += "&sort=#{results['sort_order'].first['field']}"
        if results['sort_order'].first['reverse']
          link += "&order=desc"
        end
      end

      links = []

      links << edge_page_link(current_page == 1, link, current_page-1, "&laquo; Previous")

      links << (1..last_page).map do |page|
        if page == current_page
          %Q|<span class="current">#{page}</span>|
        else
          page_link(link, page)
        end
      end

      links << edge_page_link(current_page == last_page, link, current_page+1, "Next &raquo;")

      %Q|<div class="pagination">#{links.join}</div>|
    end

I run my specs to make sure that nothing has changed—this is critical to do whenever doing any refactoring lest you find an early change broke your examples and all subsequent work was built on a poor foundation.

I also need to refactor that map with a conditional inside it. The map builds the links to individual pages, with the conditional responsible for marking the current page. An equivalent way of accomplishing this, one that will put me in a better position moving forward, is to build all the links up to the current page, build the current page marker, and then build all of the links after the current page:

      links << (1...current_page).map { |p| page_link(link, p) }
      links << %Q|<span class="current">#{current_page}</span>|
      links << (current_page+1..last_page).map { |p| page_link(link, p) }

That is much nicer—no conditionals. As an aside, I am using two different range operators—the two dot (include the last value) and the three dot form (exclude the last value).

The last thing that I will do tonight is create a first and last page link. Unless the user is on the first page, there should always be a link to the first page. If the user is on page 26, there is not much need to present a link to page 2, but there should definitely be a way to get to the start of the list. Similarly, if the user is not on the last page, there should always be a way to get there. So I break out the first and last pages:

      links << page_link(link, 1) if current_page != 1

      links << (2...current_page).map { |p| page_link(link, p) }

      links << %Q|<span class="current">#{current_page}</span>|

      links << (current_page+1...last_page).map { |p| page_link(link, p) }

      links << page_link(link, last_page) if current_page != last_page

That will serve nicely as a stopping point for tonight. I have changed absolutely no functionality—29 pages still produce 29 pagination links. What I have done is cleaned up the code and put myself in a better position to add a sliding window of links around the current page.

To be clear, this is not an exercise in building good pagination—something along the lines of Rails' old classic pagination probably would have been a better option for me here. But this solution will do, and it is nice to exercise my refactoring chops from time-to-time.