Extracting Polymer Behaviors from Existing Polymer Elements

My hack to get Polymer elements working with native forms (breaking encapsulation and inserting hidden <input> elements into the containing document) is growing on me. It was fairly easy to implement as a proof of concept. It was nearly as easy to write a bunch of tests verifying that the 80% use-cases work. So tonight I push ahead.

My normal next step while writing Patterns in Polymer would be to reimplement in the other language in which the book is written. In this case, since I have written the initial implementation in Dart, I would normally redo the implementation in JavaScript. If a solution works similarly as well in both languages, then I have some assurance that I have a solution that transcends languages—that it is a true pattern of the library.

But that's not what I am going to do tonight. I can't help myself—I must refactor. I wrote a handful of tests describing how I'd like Polymer elements to behave when they are used as HTML form elements. Tests are a great way to verify that things do not break over time. That is a necessary, but boring, reason to test. More fun is refactoring. I have all of these tests describing how I expect my element to behave. Unless I have poor tests (always a possibility), these tests will pass no matter how I might rework the implementation.

And the reimplementation that I hope to realize tonight is extracting this is-a-form-input behavior into a separate Polymer class that my custom Polymer elements can extend as subclasses.

The core <input> tests that I would like to continue to pass tonight are:

PASS: <x-pizza> acts like <input> - value property is updated when internal state changes
PASS: <x-pizza> acts like <input> - value attribute is updated when internal state changes
PASS: <x-pizza> acts like <input> - containing form includes input with supplied name attribute
PASS: <x-pizza> acts like <input> - setting the name property updates the name attribute

I should be able to pull all of last night's code out of the <x-pizza> element into <a-form-input> leaving <x-pizza> unchanged from when I began save for a new superclass:

import 'package:polymer/polymer.dart';
import 'a_form_input.dart';

@CustomTag('x-pizza')
class XPizza extends AFormInput {
  // Code reverted back to original implementation
}

That turns out to be quite easy to make happen. I define the AFormInput class as:

import 'package:polymer/polymer.dart';
import 'dart:html';

@CustomTag('a-form-input')
class AFormInput extends PolymerElement {
  @PublishedProperty(reflect: true)
  String name;

  @PublishedProperty(reflect: true)
  String value;

  Element lightInput;

  AFormInput.created(): super.created();

  void attached() {
    super.attached();

    lightInput = new HiddenInputElement();
    if (name != null) lightInput.name = name;
    parent.append(lightInput);
  }

  void attributeChanged(String name, String oldValue, String newValue) {
    if (name == 'name') {
      lightInput.name = newValue;
    }
  }
}

All of that code is a direct copy of the code from last night except for the attached() lifecycle method. Polymer calls this method when the custom element is attached to a live DOM. This was more complicated last night because <x-pizza> had to do some work when attached as did this is-a-form-input code. Now that this code is properly encapsulated, I no longer need to keep these different behaviors in separate methods. Thanks to proper class encapsulation, these behaviors are pushed back up to the direct methods applying them.

Even better, my tests continue to pass:

PASS: <x-pizza> acts like <input> - value property is updated when internal state changes
PASS: <x-pizza> acts like <input> - value attribute is updated when internal state changes
PASS: <x-pizza> acts like <input> - containing form includes input with supplied name attribute
PASS: <x-pizza> acts like <input> - setting the name property updates the name attribute

Better still, is that my Polymer-is-a-form-input hack still works in live code:



Best of all is that I was absolutely certain that it would continue to work—thanks to the tests that I wrote last night.


Day #8

‹prev | My Chain | next›

Refactoring in the Spirit of Polymer

One of the really solid pieces of advice that I have received on Patterns in Polymer is that the example in the Model Driven View chapter is a tad large.

The MDV example is a simple pizza maker:



The “model” part being a simple object literal comprised of different lists for the toppings:

Polymer('x-pizza', {
  ready: function() {
    this.model = {
      firstHalfToppings: [],
      secondHalfToppings: [],
      wholeToppings: []
    };
  },
  // ...
});

I favor this approach because the model, which is the central piece of the MDV chapter, it relatively small and easily understood. That said, the code that backs these lists is repetitive and long. In other words, the backing class strays from the “Polymer way.” I hate to lose the current, easily understood model, but I also hate to stray from the spirit of Polymer in any of my examples.

I may very well have to introduce an entirely different example. Before I go to that extreme, I try one of the suggestions of moving the model into a new <x-pizza-toppings> Polymer element. This initially means that the <x-pizza> template gets much simpler. It goes from:

<polymer-element name="x-pizza">
  <template>
    <p>
      <select class="form-control" value="{{currentFirstHalf}}">
        <option>Choose an ingredient...</option>
        <option value="{{ingredient}}" template repeat="{{ingredient in ingredients}}">
          {{ingredient}}
        </option>
      </select>
      <button on-click="{{addFirstHalf}}" type="button" class="btn btn-default">
        Add First Half Topping
      </button>
    </p>
    <!-- Nearly identical 2nd half and whole toppings template code... -->
  </template>
  <script src="x_pizza.js"></script>
</polymer-element>

To the much more readable:

<link rel="import" href="x-pizza-toppings.html">
<polymer-element name="x-pizza">
  <template>
    <h2>Build Your Pizza</h2>
    <pre>{{pizzaState}}</pre>
    <x-pizza-toppings id="firstHalfToppings"
                      name="First Half Toppings"
                      ingredients="{{ingredients}}"></x-pizza-toppings>
    <x-pizza-toppings id="secondHalfToppings"
                      name="Second Half Toppings"
                      ingredients="{{ingredients}}"></x-pizza-toppings>
    <x-pizza-toppings id="wholeToppings"
                      name="Whole Toppings"
                      ingredients="{{ingredients}}"></x-pizza-toppings>
  </template>
  <script src="x_pizza.js"></script>
</polymer-element>

The repetitive HTML goes into the template for <x-pizza-toppings> with almost no changes. The backing class of <x-pizza-toppings> then gets the model:

Polymer('x-pizza-toppings', {
  ingredients: [],
  ready: function() {
    this.model = [];
  },
  current: '',
  add: function() {
    this.model.push(this.current);
  }
});

That really is much cleaner. The “model” is now an array, which is a little weird, but I can live with that. The code is much cleaner and much DRYer, so this definitely feels better.

That said, I am more than a little concerned at the number of concepts that I would be introducing with this example. In addition to discussing MDV, I would also have to introduce child Polymer elements. I would even have to mention the data binding of the master ingredients list in <x-pizza> for sharing the list with the child elements. This is not too horrible, especially for a book that is aimed at beyond the introduction. Still, the fewer the concepts, the better.

Speaking of concepts, I am still not quite done here. In the old version, I updated a string representation of the entire pizza whenever a topping change was made. To do that in this version, I need the child <x-pizza-toppings> elements to communicate up to the parent <x-pizza> element whenever a change occurs. That means observing the <x-pizza-toppings> model for changes so that it can fire a custom event:

Polymer('x-pizza-toppings', {
  observe: {
    'model': 'fireChange'
  },
  // ...
  fireChange: function() {
    this.fire('topping-change');
  }
});

The <x-pizza> can then listen for these topping change events, updating the “pizza state” accordingly:

Polymer('x-pizza', {
  // ...
  ready: function() {
    this.addEventListener('topping-change', function(event){
      this.updatePizzaState();
    });
  },
  updatePizzaState: function() {
    var pizzaState = {
      firstHalfToppings: this.$.firstHalfToppings.model,
      secondHalfToppings: this.$.secondHalfToppings.model,
      wholeToppings: this.$.wholeToppings.model
    };
    this.pizzaState = JSON.stringify(pizzaState);
  }
});

With that, I have a fully functional <x-pizza> Polymer element:



I have much less code accomplishing this and it all feels much more approachable. Still...

I will need to think about this approach before pulling it into the book. The ultimate solution may be as simple as only discussing <x-pizza-toppings> and leaving the inclusion in <x-pizza> until a later chapter. That would require some chapter reorganization, but it may be worth it.

Regardless, something needs to change because, thanks to some very helpful feedback, I am much happier with this solution than my previous approach. Improved solutions are always worth the effort, so I have some editing ahead of me!


Day #6

‹prev | My Chain | next›

Refactoring a Fab.js Stack

‹prev | My Chain | next›

Yesterday, I did not quite finish with my fab.js cleanup. Previously, I had refactored the code in my (fab) game to something along the lines of:

  ( /^\/comet_view/ )
    ( broadcast_new )
    ( init_comet )
    ( add_player )
    ( player_from_querystring )

The init_comet and player_from_querystring (fab) apps got the refactoring love. The broadcast_new and add_player apps are the leftovers that did not get refactored, but they are quite important to the game. They add the new player to the list of players and broadcast the new player to that list of players respectively.

The trouble I ended with last night was getting them both to play nicely with each other and getting the init_comet app to pass along the player information to broadcast_new. It turns out that the latter is fairly easy to resolve. Previously, init_comet only sent comet initialization back to the client, ending with something like:

         ({body: "<script type=\"text/javascript\">\"123456789 123456789 123456789 123456789 123456789 12345\";</script>\n"})
         ({body: "<script type=\"text/javascript\">\"123456789 123456789 123456789 123456789 123456789 12345\";</script>\n"})

It did not send along the player information because the browser did not need to know it (the browser had added the new player in the first place). The trick is to pass along the player information, but to have broadcast_new use it and not pass it along to the browser. In init_comet, I pass along the player information by passing the object encapsulating it back downstream:

         ({body: "<script type=\"text/javascript\">\"123456789 123456789 123456789 123456789 123456789 12345\";</script>\n"})
         ({body: "<script type=\"text/javascript\">\"123456789 123456789 123456789 123456789 123456789 12345\";</script>\n"})
         (obj);

Then, in broadcast_new, I add a condition to broadcast the new player to the existing players, but send back the comet_initialization to the new player's browser:

function broadcast_new (app) {
  return function () {
    var out = this;

    return app.call( function listener(obj) {
      if (obj && obj.body && obj.body.id) {
        broadcast(comet_walk_player(JSON.stringify(obj.body)));
      }
      else {
        out(obj);
      }
      return listener;
    });
  };
}

The add_player app turns out to be a bit trickier. So tricky in fact that I have to eliminate it. It had added the player along with its comet channel to a list of players. The problem I ran into was the comet channel. The comet channel is further upstream than the init_comet binary app:

  ( /^\/comet_view/ )
    ( broadcast_new )
    ( init_comet )
    ( add_player )
    ( player_from_querystring )

This means that any subsequent communication over the channel needed to go through the init_comet channel. Initializing the comet channel (sending new headers and new opening HTML tags) each time a new player is added turns out to be a brilliant way to close all previous comet channels. So clearly I need something as close downstream as possible to handle this.

For now, I add the code that previously went in the add_player app into broadcast_new, making the add-player stack a bit smaller:

  ( /^\/comet_view/ )
    ( broadcast_new )
    ( init_comet )
    ( player_from_querystring )

Smaller and with a broadcast_new app with too many responsibilities. Still, I am in a better place than when I started. The init_comet and player_from_querystring apps are now tested and all of my untested code is working (in that I can play the game again) and in one place for future refactoring.

And that is a good place to stop for the night.

Day #120

Small couch_docs Updates

‹prev | My Chain | next›

After updating couch-replicate yesterday, I take a look at my couch_docs gem tonight. I received a patch a while back suggesting that watch mode should not update all documents when any update is made. That seem reasonable, so...

When I first start watching a directory, all documents should be pushed to the CouchDB server. Afterwards, if the updates are design documents, only the design documents should be updated. If the updates are normal documents, then the individual, updated documents should be updated. Sounds like a bunch of predicate methods to me.

First up, an RSpec example describing initial directory parsing:

    it "should be an initial add if everything is an add" do
      args = [mock(:type => :added),
              mock(:type => :added)]
      CommandLine.should be_initial_add(args)
    end

This dumps me into change-the-message of:

1)
NoMethodError in 'CouchDocs::CommandLine an instance that dumps a CouchDB database should be an initial add if everything is an add'
undefined method `initial_add?' for CouchDocs::CommandLine:Class
./spec/couch_docs_spec.rb:471:

I eventually get this passing with:

  def initial_add?(args)
    args.all? { |f| f.type == :added }
  end

I add a few more predicate methods, and then I am ready to refactor my directory watcher update block:

        dw.add_observer do |*args|
          puts "Updating documents on CouchDB Server..."
          CouchDocs.put_dir(@options[:couchdb_url],
                            @options[:target_dir])
        end

First up, I pull the put_dir call out into a new (testable) directory_watcher_update method:

#...
        dw.add_observer do |*args|
          puts "Updating documents on CouchDB Server..."
          directory_watcher_update(args)
        end
#...

  def directory_watcher_update(args)
    CouchDocs.put_dir(@options[:couchdb_url],
                      @options[:target_dir])
  end

I run all of my specs to ensure nothing has broken (it has not) before describing in more detail what should happen with directory watcher updates. First up, it should update both design document and normal documents when first starting up (which is what put_dir does):

      it "should only update design docs if only local design docs have changed" do
        CouchDocs.
          should_receive(:put_dir)

        @it.stub!(:initial_add?).and_return(true)
        @it.directory_watcher_update(@args)
      end

That example passes without any changes (because that is the current behavior of the method). The trick will be to retain this behavior going forward.

I drive this method to be able to handle design document updates as well before needing to call it a night:

  def directory_watcher_update(args)
    if initial_add? args
      CouchDocs.put_dir(@options[:couchdb_url],
                        @options[:target_dir])
    else
      if design_doc_update? args
        CouchDocs.put_design_dir(@options[:couchdb_url],
                                 "#{@options[:target_dir]}/_design")
      end
    end
  end

The call to update normal (non-design) documents may require some refactoring before it can be used in here. I will pick up with that (and hopefully finish) tomorrow.

Day #56

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!

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.