Refactoring: A Payoff

‹prev | My Chain | next›

Today, I continue trying to resolve my too-many-page-links problem:



Yesterday, I refactored the pagination helper to put me in a better position for this work, ending up in this form:

    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 = base_pagination_link(query, results)

      links = []
      links << edge_page_link(current_page == 1, link, current_page-1, "« Previous")

      links << page_link(link, 1) if current_page != 1
      # Window prior to the current page
      links << (2...current_page).map { |p| page_link(link, p) }

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

      # Window after to the current page
      links << (current_page+1...last_page).map { |p| page_link(link, p) }

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

      links << edge_page_link(current_page == last_page, link, current_page+1, "Next »")

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

The two windows around the current page are indicated in bold above. At this point, the windows run all the way from the first page up to the current page (or from the current page up to the last). In other words, every single page (all 42 if there are 42 pages) are being displayed. Functionally, that is exactly where I started yesterday, but I can work with this form to make limited windows around the current page.

An RSpec example of what I need:

  context "in the middle (page 21) of a large result sets (42 pages)" do
    before(:each) do
      @results['skip'] = 400
      @results['total_rows'] = 841
    end
    it "should have a link to page 1" do
      pagination(@query, @results).
        should have_selector("a", :content => "1")
    end
    it "should not have a link to page 2" do
      pagination(@query, @results).
        should_not have_selector("a", :href => "/recipes/search?q=foo&page=2")
    end
  end

After verifying that the example fails, I get it to pass by defining a start window:

    def pagination(query, results)
#...
      start_window = 3
      links << (start_window...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) }
#...
    end

It passes, but starting the pagination links at 3 instead of 2 is not much of a window. To drive the window, another two examples:

    it "should not have a link to page 17" do
      pagination(@query, @results).
        should_not have_selector("a", :href => "/recipes/search?q=foo&page=17")
    end
    it "should have a link to page 18" do
      pagination(@query, @results).
        should have_selector("a", :href => "/recipes/search?q=foo&page=18")
    end

I use two examples to accurately describe the boundary condition that I am probing here. Besides, I am not adding two failing examples—the second example passes in my current window-starting-at-page-3 state as it should pass once I have a better window.

To get those two examples to pass, I need to modify the calculation of the start_window local variable:

    def pagination(query, results)
#...
      start_window = current_page - 3
      links << (start_window...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) }
#...
    end

I use similar examples to drive the implementation of the end of the window, ending up with this:

    def pagination(query, results)
#...
      start_window = current_page - 3
      links << (start_window...current_page).map { |p| page_link(link, p) }

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

      end_window = current_page + 3
      links << (current_page+1..end_window).map { |p| page_link(link, p) }
#...
    end

That works just fine for a large result set, such as the one used for these examples. If the current page is 2, then the start_window is going to be -1. That might cause problems. In fact, it breaks some existing examples:

==
Helper specs
............................F......................................

1)
'pagination should have only 2 pages, when results.size == 2 * page size' FAILED
expected following output to omit a <a>3</a>:
<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN" "http://www.w3.org/TR/REC-html40/loose.dtd">
<html><body><div class="pagination">
<span class="inactive">« Previous</span><a href="/recipes/search?q=foo&page=-2">-2</a><a href="/recipes/search?q=foo&page=-1">-1</a><a href="/recipes/search?q=foo&page=0">0</a><span class="current">1</span><a href="/recipes/search?q=foo&page=2">2</a><a href="/recipes/search?q=foo&page=3">3</a><a href="/recipes/search?q=foo&page=4">4</a><a href="/recipes/search?q=foo&page=2">2</a><a href="/recipes/search?q=foo&page=2">Next »</a>
</div></body></html>
./spec/eee_helpers_spec.rb:214:

Rather than fixing that failing spec, I create new examples specifically describing the edge cases of being near the first page or near the last page:

  context "at the beginning (page 2) of a large result sets (42 pages)" do
    before(:each) do
      @results['skip'] = 20
      @results['total_rows'] = 841
    end
    it "should not have a link to page 0" do
      pagination(@query, @results).
        should_not have_selector("a", :href => "/recipes/search?q=foo&page=0")
    end
  end

That example fails because the start window currently includes a page zero(!). To get rid of it, I add a ternary to the start_window calculation:

    def pagination(query, results)
#...
      start_window = current_page > 4 ? current_page - 3 : 2
      links << (start_window...current_page).map { |p| page_link(link, p) }

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

      end_window = current_page + 3
      links << (current_page+1..end_window).map { |p| page_link(link, p) }
#...
    end

After adding something similar for the end_window, I have all of my examples (and Cucumber scenarios) passing and my pagination looking sane:



(commit)

Nice! It may be time to set up a VPS tomorrow!