This refactoring is based on a topic Marcel and I covered at RailsConf Europe.

Before

1
2
3
4
5
6
7
8
9
10
11
12
13

class Expense < ActiveRecord::Base
  belongs_to :payee
  protected
    
    # Nice and concise, but what happens as we add more rules
    # and how do we write test cases for the four different possible 
    # validation states?
    def validate
      errors.add("Not enough funds") if payee.balance - amount > 0
      errors.add("Charge is too great") if payee.account.maximum_allowable_charge > amount
    end
end

After

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37

class Expense < ActiveRecord::Base
  belongs_to  :payee
  
  # Instead of one large validation method, break each individual
  # rule into methods, and declare them here.
  validate      :ensure_balance_is_sufficient_to_cover_amount
  validate      :amount_does_not_exceed_maximum_allowable_charge

  protected

    # These validation callbacks simply add error messages if a particular 
    # condition is met.   Each of them can be tested and understood on their own
    # without having to understand the entire body of the validate method.
    def ensure_balance_is_sufficient_to_cover_amount
      errors.add("Not enough funds") if insufficient_funds?
    end
    
    def amount_does_not_exceed_maximum_allowable_charge
      errors.add("Charge is too great") if exceeds_maximum_allowable_charge?
    end
    
    # By defining separate predicate methods we can test each of them individually
    # and new programmers can see the intent of the code, not just the implementation.


    # Instead of subtracting the amount from the balance and checking if the
    # value is greater than 0,  change the implementation to mirror the intent.
    # There was a bug in the before code,  this is more obvious.
    def insufficient_funds?
      amount > payee.balance
    end
    
    def exceeds_maximum_allowable_charge?
      payee.account.maximum_allowable_charge > amount
    end
end

While the refactored version may have more lines of code, but don’t let that scare you. It’s far more important for code to be human readable than incredibly concise.

Today I’m reviewing Joe Van Dyk’s monkeycharger application, which is a web-service for storing and charging credit cards. I loved looking at this app, because its only interface is a RESTful web service: there is no HTML involved. (If you’ve never written an app that only exposes a web-service UI, you ought to. It’s a blast.)

In general, Joe has done a fantastic job with keeping the controllers slim and moving logic to models. The only significant gripe I had with the application is that it is not ActiveResource compatible.

For those of you that are late to the party, ActiveResource is the newest addition to the Rails family. It lets you declare and consume web-services using an ActiveRecord-like interface…BUT. It is opinionated software, just like the rest of Rails, and makes certain assumptions about the web-services being consumed.

1. The service must understand Rails-style REST URLs. (e.g. “POST /credit_cards.xml” to create a credit card, etc.)
2. The service must respond with a single XML-serialized object (Rails-style).
3. The service must make appropriate use of HTTP status codes (404 if the requested record cannot be found, 422 if any validations fail, etc.).

It’s really not much to ask, and working with ActiveResource (or “ares” as we affectively call it) is a real joy.

However, monkeycharger tends to do things like the following:

1
2
3
4
5
6
7
8
9
10

class AuthorizationsController < ApplicationController
  def create
    @credit_card   = Authorizer.prepare_credit_card_for_authorization(params)
    transaction_id = Authorizer::authorize!(:amount => params[:amount], :credit_card => @credit_card)
    response.headers['X-AuthorizationSuccess'] = true
    render :text => transaction_id
  rescue AuthorizationError => e
    render :text => e.message
  end
end

Three things: the request is not representing an “authorization” object, the response is not XML, and errors are not employing HTTP status codes to indicate failure.

Fortunately, this is all really, really easy to fix. First, you need (for this specific example) an Authorization model (to encapsulate both the the XML serialization and the actual authorization).

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20

class Authorization
  attr_reader :attributes

  def initialize(attributes)
    @attributes = attributes
  end

  def credit_card
    @credit_card ||= Authorizer.prepare_credit_card_for_authorization(attributes)
  end

  def authorize!
    @transaction_id = Authorizer.authorize!(:amount => attributes[:amount],
      :credit_card => credit_card)
  end

  def to_xml
    { :transaction_id => @transaction_id }.to_xml(:root => "authorization")
  end
end

Then, we rework the AuthorizationsController to use the model:

1
2
3
4
5
6
7
8

class AuthorizationsController < ApplicationController
  def create
    authorization = Authorization.new(params[:authorization])
    authorization.authorize!
    render :xml => authorization.to_xml, :status => :created
  rescue AuthorizationError => e
    render :xml => "<errors><error>#{e.message}</error></errors>", :status => :unprocessable_entity
  end

(Note the use of the “created” status, which is HTTP status code 201. Other verbs just use “ok”, status code 200, to indicate success. Also, with an error, we return an “unprocessable_entity” status, which is HTTP status code 422. ActiveResource will treat that as a failed validation.)

With that change, you could now use ActiveResource to authorize a credit card transaction:

1
2
3
4
5
6
7
8
9
10
11
12

class Authorization < ActiveResource::Base
  self.site = "http://my.monkeycharger.site"
end

auth = Authorization.new(:amount => 15, :credit_card_id => 1234,
  :remote_key => remote_key_for_card)

if auth.save
  puts "success: #{auth.transaction_id}"
else
  puts "error: #{auth.errors.full_messages.to_sentence}"
end

It should be mentioned, too, that making an app ActiveResource-compatible does nothing to harm compatibility with non-ActiveResource clients. Everything is XML, both ways, with HTTP status codes being used to report whether a request succeeded or not. Win-win!

Obviously, real, working code trumps theoretical whiteboard sketches every time, and Joe is to be congratulated on what’s done. Even though ActiveResource-compatibility can buy you a lot, you should always evaluate whether you really need it and implement accordingly.

“Cargo culting”, when used in a computer-programming context, refers to the practice of using techniques (or even entire blocks of code) seen elsewhere without wholly understanding how they work. (The term “cargo cult”, if you are unfamiliar with it, has its own fascinating etymology, which is covered nicely at wikipedia.) Cargo culting is a dangerous phenomenon, watering down the state of the art and encouraging cookie-cutter code shoved blindly into black boxes.

Consider the following snippet of code, taken from a project that was submitted to us some time ago. (Alas, I cannot find the original submitter—I apologize for that!)

1
2
3

def account_code?
  !! @account_code.nil?
end

To me, this looks cargo-culted, since it is seems that the programmer did not understand what the ”!!” idiom was all about. They probably saw it used somewhere and “cargo culted” it, using it without knowledge, assuming that it was, for some reason, “necessary”.

Now, the way ”!!” works is this: take the value behind the ”!!”, negate it, and negate it again. It’s just double-negation: !(!(@account_code.nil?)). The ultimate effect is to take some value, and convert it into an honest-to-goodness “true” or “false”. (In my ever-so-humble opinion, the ”!!” idiom is an abomination: it’s far too clever for its own good. First of all, you rarely ever need a real boolean value, and for those times you do, it is better to be explicit in the conversion, by using a ternary operator or full-blown if statement, for instance.)

In other words, the double-negation of nil? results in absolutely no difference from the use of nil? by itself, since nil? will return a true/false value. This, in turn, means the effect in the original is actually not what was intended for the account_code? predicate. It should have returned “true” if the account code existed (was “non-nil”), not “false”. Thus, the method should have actually been written thus:

1
2
3

def account_code?
  ! @account_code.nil?
end

In this case, cargo-culting resulted in the code being buggy. This is not an uncommon outcome of using techniques or code without understanding their purpose. If you ever find yourself copying something into your own code, with a justfying “I-don’t-know-what-it-does, but-it-appears-to-work”, stop immediately. Do some research. Figure it out. Learn what it means.

Further, note that unless you actually need a true boolean value from that, you can shorten the implementation of the account_code? predicate even further:

1
2
3

def account_code?
  @account_code
end

This works because Ruby treats nil and false as false, and everything else as true.

If there is one thing that Koz and I want you, our readers, to come away from this site with, it is an understanding of why you should do things one way and not another. Ultimately, it makes the difference between being a mediocre programmer, and becoming a great programmer.

Another topic we touched briefly on at RailsConf was the idea of named callbacks.

Consider this snippet (also from Brian Cooke’s expense tracking application):

1
2
3
4
5
6
7
8

class Expense < ActiveRecord::Base
  protected
    def before_create
      if self.created_at == Time.now.to_date.to_time
        self.created_at = Time.now
      end
    end
end

One thing to keep in mind here is that when a new Expense record is created, the created_at column is used to track when the expense originally occurred, not when the record was created. As a special case, if the timestamp is 00:00 of the current day, then it is assumed to actually be the current time.

Now, looking at that code, it’s definitely not immediately obvious what it is trying to do. In fact, it took me a few minutes of steady concentration (and cross-referencing other parts of the project) to understand it. The fact that it uses a generic “before_create” callback makes it hard to know the purpose of the method, and the use of “Time.now.to_date.to_time” (though effective) is pretty intention-obscuring.

Here’s a clearer, more self-documenting approach, using a named callback:

1
2
3
4
5
6
7
8
9
10

class Expense < ActiveRecord::Base
  before_create :make_created_now_if_created_today

  protected
    def make_created_now_if_created_today
      if self.created_at == Time.now.beginning_of_day
        self.created_at = Time.now
      end
    end
end

The named callback helps make it clearer what the purpose of the method is (though in this case, an additional comment would not be amiss). Also, ActiveSupport comes to the rescue, allowing us to convert the convoluted “Time.now.to_date.to_time” into the more self-documenting “Time.now.beginning_of_day”. (Alternatively, you might prefer “Time.now.midnight”, though I find “beginning_of_day” to be clearer, since it reveals the intention better.)

Always look for ways to make your code document itself. Ruby is one of the most readable programming languages I’ve ever used, and it’s a pity to not take advantage of that readability as often as you can.

RailsConf 2007 was a blast. Koz and I were able to do a live “Rails Way” presentation in front of all 1,600 attendees, and it was a lot of fun. We’d like to thank everyone who helped (with their questions) to make it a success, and especially Chad Fowler (for inviting us to do a plenary session instead of a regular session) and O’Reilly and Ruby Central for all their work behind the scenes to bring the conference off so spectacularly.

Now that RailsConf is past, though, we’re looking forward to bringing The Rails Way out of hibernation. The next few articles will review the points we mentioned in our RailsConf presentation.

For this first article, I’ll be examining one of the controllers in an expense tracking application submitted by Brian Cooke. In general, the application was really well done, but I did find one example of a fat controller.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15

class ReportsController < ApplicationController
  def create
    report = params[:report]
    category_condition = report[:category] == "all" ? "" : "AND category_id = #{report[:category]}" 
    from = Date.parse("#{report["from(1i)"]}-#{report["from(2i)"]}-#{report["from(3i)"]}")
    to = Date.parse("#{report["to(1i)"]}-#{report["to(2i)"]}-#{report["to(3i)"]}").to_time+1.day-1
    @expenses = current_user.expenses.find(:all, 
                                           :conditions => ["created_at >= ? AND created_at <= ? #{category_condition}", from.to_s(:db), to.to_s(:db)], 
                                           :order => "created_at ASC")
    @graph = params[:report][:type] == "graph" 

    @total_price = 0.0
    @expenses.each {|exp| @total_price += exp.price}
  end
end

All that code obscures the purpose of the action. Looking at that, your brain has to do a lot of work to figure out what the intention is. To make the intention clearer, most of that code should be refactored into a model, but the question is: which model? You can get some idea of what model is needed by looking at the query parameter that is used: report.

Now, there is no “reports” table in the domain for this application. That does not mean there can’t be a Report model. Many people new to MVC who are learning Rails have the idea that all models correspond to database tables. The fact is that a model is just an object, persistent or otherwise.

Consider the ReportsController#create action, rewritten to take advantage of a Report model:

1
2
3
4
5

class ReportsController < ApplicationController
  def create
    @report = Report.new(current_user, params[:report])
  end
end

Now that’s skinny. Looking at that action, it is now immediately obvious (at a high level) what it is doing. Skinny controllers are intention-revealing. Fat controllers are intention-obscuring.

Let’s take a quick look at how the Report model breaks down:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53

class Report
  attr_reader :user, :options

  def initialize(user, options)
    @user = user
    @options = options || {}
  end

  def graph?
    @options[:type] == "graph" 
  end

  def total_price
    @total_price ||= expenses.sum(&:price)
  end

  def expenses
    @expenses ||= user.expenses.find(:all,
      :conditions => conditions,
      :order => "created_at ASC")
  end

  def from
    @from ||= date_from_options(:from)
  end

  def to
    @to ||= date_from_options(:to)
  end

  private

    def date_from_options(which)
      part = Proc.new { |n| options["#{which}(#{n}i)"] }
      if part[1]
        Date.new(part[1].to_i, part[2].to_i, part[3].to_i)
      else
        Date.today
      end
    end

    def conditions
      conditions = ["created_at between ? AND ?"]
      parameters = [from.to_time, to.to_time.end_of_day]

      if options[:category] != "all" 
        conditions << "category_id = ?" 
        parameters << options[:category]
      end

      [conditions.join(" AND "), *parameters]
    end
end

Now, this is a lot more lines of code than the original action. That’s not a bad thing! More lines of code are always better, if they make the code easier to read and maintain. In the above, I broke the original code into discrete parts, each of which went into it’s own method. This has several nice side-effects:

1. You can now test the Report model in isolation, instead of having to write a functional test and assert_select your way through the output.
2. The multiple instance variables in the original action are replaced now with getter attributes on the model, so in the view you have @report.total_price instead of the more ambiguous @total_price.
3. You can now use the form_for helper to simplify the view.

1
2
3
4
5

<%= form_for(:report, @report, ...) do |f| %>
  From: <%= f.date_select(:from) %>
  To: <%= f.date_select(:to) %>
  <!-- etc.... -->
<% end %>

Thanks to Brian Cooke for the submission!

In addition to the REST refactoring for SignOut, we thought we’d cover a few more changes we’d suggest.

The biggest thing which jumped out at me was that SignOut has no tests. If you’ve never written a test in rails all the different tools and frameworks available can make it seem quit daunting. It’s often hard to tell if you should you use rspec, heckle, autotest, rcov and where to start. So in this article we’ll take the set_away action from EmployeeController and write a few tests for it, using just the stuff you get for free with rails.

The action as it stands has some reasonably involved code for handling decoding time_back from the request parameters:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18

def set_away
  @employee = Employee.find(params[:id])
  if params[:date][:meridian] == "PM"
    if params[:date][:hour].to_i == 12
      hour = params[:date][:hour]
    else
      hour = (params[:date][:hour].to_i + 12).to_s
    end
  else
    if params[:date][:hour].to_i == 12
      hour = "0"
    else
      hour = params[:date][:hour]
    end
  end
  time_back = Time.mktime(params[:date][:year], params[:date][:month], params[:date][:day], hour, params[:date][:minute] )
  @employee.update_attributes(:reason => params[:employee][:reason], :time_out => Time.now, :time_in => time_back)
end

So there are four distinct cases that we need to ensure are tested:

1. A Meridian of “PM” and an hour of 12
2. A Meridian of “PM” and some other hour
3. A Meridian of “AM” and an hour of 12
4. A Meridian of “AM” and some other hour

First up we need some fixtures data for an employee, lets pretend I work there and create a test fixture for koz:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15

koz:
  id: 1
  firstname: Michael 
  lastname: Koziarski
  company_email: koz@example.com
  personal_email: michael@koziarski.com
  extension: 665
  cellphone: +64 21 555 1337
  homephone: +64 4 555 1337
  time_out:
  time_in:
  reason:
  created_on: <%= 5.days.ago.to_date.to_s(:db) %>
  modified_on: <%= Date.today.to_s(:db) %>
  initials: MAK

Now the tests in EmployeeControllerTest:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33

class EmployeeControllerTest < Test::Unit::TestCase
  def setup
    @controller = EmployeeController.new
    @request    = ActionController::TestRequest.new
    @response   = ActionController::TestResponse.new
  end
  # Ensure that the employees table gets reloaded as needed
  fixtures :employees

  # we can DRY up the test data by providing a default date 
  #and then just overriding what we need
  def default_date
    {:meridian=>"PM", :year=>now.year, :month=>now.month, 
     :day=>now.day, :minute=>"0", :hour=>"12"}
  end

  # Test the first case, PM and 12
  def test_set_away_for_pm_and_12_oclock
    # the default data matches this test case, 
    post :set_away :id=>employees(:koz).id, :date=>default_date
    # ensure no errors ocurred
    assert_response :success
    # reload our fixture to match the database changes
    koz = employees(:koz).reload
    # make sure the time_back attribute has been set 
    assert time_back = koz.time_back
    assert_equal 0, time_back.minute
    # 12PM is 12 midday, so the hour should be 12
    assert_equal 12, time_back.hour
    # We provided a reason here, make sure the controller has set it
    assert_equal "Because", koz.reason
  end
end

We could continue to write tests like that, but instead we can get a little clever and let ruby write the tests for us:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24

  def default_date
    {:meridian=>"PM", :year=>now.year, :month=>now.month, 
     :day=>now.day, :minute=>"0", :hour=>"12"}
  end

  TEST_SCENARIOS= {0=>{:hour=>"12", :meridian=>"AM"}, 5=>{:hour=>"5", :meridian=>"AM"}, 
                   18=>{:hour=>"6", :meridian=>"PM"}, 12=>{:hour=>"12", :meridian=>"PM"}}
  
  TEST_SCENARIOS.each do |hour, hash_overide|
    # Make sure we give our tests useful names, otherwise it's hard to tell what failed
    name = "test_with_hour_#{hash_overide[:hour]}_and_meridian_#{hash_overide[:meridian]}"
    define_method(name) do 
      post :set_away :id=>employees(:koz).id, :date=>default_date.merge(hash_override)
      # ensure no errors ocurred
      assert_response :success
      # reload our fixture to match the database changes
      koz = employees(:koz).reload
      assert time_back = koz.time_back
      assert_equal 0, time_back.minute
      # Make sure the hour matches our expected hour
      assert_equal hour, time_back.hour
    end
  end
end

Now you can refactor the set_away action without having to worry about introducing new date related bugs.

Here we go, continuing the RESTful refactoring of Randy Schmidt’s “SignOut” application. In Part 1 I talked at a high level about how the existing application could be viewed as an interface to several different resources. In this article, I’ll show you how to implement the “employees” resource. I’ll be doing this by just showing you heavily commented code snippets.

Note that, for this article, I am not using the SimplyHelpful plugin. This is mostly so that I can demonstrate how this all would work with current Rails, without all the convenience magic that SimplyHelpful adds in. Once you understand how this all fits together, though, SimplyHelpful is an enormous time saver.

Firstly, we define the routes we need:

1
2
3
4
5
6
7
8
9

ActionController::Routing::Routes.draw do |map|

  # So far, our RESTful implementation has only a single resource defined,
  # employees. We use map.resources to set up all the routes (named and
  # otherwise) that we will need. This assumes the existence of a controller
  # named EmployeesController.
  map.resources :employees

end

Then, we implement our controller:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102

# This replaces the AdminController in the original implementation. It
# provides an interface to the employees resource, allowing employees to be
# listed, edited, created, and destroyed.
#
# Note a few things about this implementation:
#
# * I'm using respond_to in a few of the actions, to demonstrate how trivial
#   it is to make your application into a web-service. There's really very
#   little reason _not_ to do it.
#
# * I'm not bothering with any kind of authorization mechanism, because the
#   original wasn't either. However, in practice, I'd be using HTTP
#   authentication in a before_filter to make sure only people authorized
#   as administrators were accessing this resource.
#
# * Note the extensive use of named routes; map.resources sets up a bunch for
#   you, for free, and they work WONDERS in cleaning up both your controllers
#   and your views.
class EmployeesController < ApplicationController
  
  # Rather than doing Employee.find(params[:id]) in all the actions that need
  # to, just use a before filter. This makes it obvious which actions need
  # an employee ID, and keeps things DRY.
  before_filter :find_employee, :only => %w(show update destroy)

  # Return a list of all employees currently in the system.
  def index
    @employees = Employee.find(:all)
    respond_to do |format|

      # If we don't give a block, the default behavior is used, which (for
      # HTML) is to render the "employees/index.rhtml" template.
      format.html

      # We specify the :root option here, because if the list of employees
      # is empty, you'd otherwise get tags like <NilClass>.
      format.xml { render :xml => @employees.to_xml(:root => "employees") }

    end
  end

  # We could technically leave this action out completely, since Rails will
  # find the new.rhtml template and render it just fine, but it's nice to be
  # able to tell by looking at the controller what actions are defined.
  def new
  end

  # Creates a new employee. It expects a hash to come in with a single
  # :employee key, which points to a subhash of the attributes to use to
  # create the employee.
  def create
    @employee = Employee.create(params[:employee])
    respond_to do |format|

      # If we're in HTML mode, redirect back to the master list.
      format.html { redirect_to(employees_path) }

      # If we're in XML mode, just return a 201 Created response.
      format.xml { head :created, :location => employee_path(@employee) }

    end
  end

  # Display the requested employee record. For this app, we just use this
  # to display the form for modifying the employee.
  def show
    respond_to do |format|
      format.html
      format.xml { render :xml => @employee.to_xml }
    end
  end

  # Update the specified employee record. Expects the same input format as 
  # the #create action.
  def update
    @employee.update_attributes(params[:employee])
    respond_to do |format|
      format.html { redirect_to(employee_path(@employee)) }

      # "head" is a wildly useful little method. It just returns a blank
      # HTTP response, which is often what you want when dealing with XML
      # requests. Here, we just say to return a "200 OK" response with no
      # body.
      format.xml  { head :ok }
    end
  end

  # Destroy the specified employee record.
  def destroy
    @employee.destroy
    respond_to do |format|
      format.html { redirect_to(employees_path) }
      format.xml  { head :ok }
    end
  end

  private

    def find_employee
      @employee = Employee.find(params[:id])
    end
end

And, lastly, we implement our RHTML views:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51

<!-- employees/index.rhtml
     Pretty much the same as the original, but this one makes use of
     the named routes provided by map.resources. Also note that for
     the "Delete" link, you have to set the :method to :delete, since
     RESTful actions use the same URL with different HTTP verbs to
     differentiate the actions. -->
<p><%= link_to "Add", new_employee_path, :class => "Add" %></p>
<ul>
<% @employees.each do |employee| %>
  <li>
    <%= employee.last_first %>
    <%= link_to "Edit", employee_path(employee), :class => "Edit" %>
    <%= link_to "Delete", employee_path(employee), :method => :delete,
          :confirm => "Are you sure you want to delete #{employee.name}?",
          :class => "Delete" %>
  </li>
<% end %>
</ul>

<!-- employees/new.rhtml
     We set up the form to POST to the /employees resource, which will
     create the new record. Note the use of form_for, which yields an instance
     of FormBuilder. This lets us do things like "f.text_field(:firstname)"
     and have the text field set up with the correct name ("employee[firstname]").
     This makes it dead simple to get the kind of nested hashes that RESTful
     actions expect (see #create and #update). -->
<% form_for :employee, Employee.new, :url => employees_path do |f| %>
  <%= render :partial => "employees/form", :locals => { :form => f } %>
<% end %>

<!-- employees/show.rhtml
     Pretty much the same as new.rhtml, but this time we post to the URI for
     a specific employee. We also set the HTTP verb to PUT, via the _method
     parameter. -->
<% form_for :employee, @employee, :url => employee_path(@employee) do |f| %>
  <%= hidden_field :_method, :put %>
  <%= render :partial => "employees/form", :locals => { :form => f } %>
<% end %>

<!-- employees/_form.rhtml
     This expects a local variable 'form' to exist, which points to a
     FormBuilder instance. -->
<dl>
  <dt><label for="employee_firstname">First Name:</label></dt>
  <dd><%= form.text_field :firstname, :size => '30', :maxlength => '100' %></dd> 

  <dt><label for="employee_initials">Initials:</label></dt>
  <dd><%= form.text_field :initials, :size => '5', :maxlength => '5' %></dd> 

  <!-- etc, etc, etc. -->
</dl>

And that’s the first resource! In the next article, I’ll illustrate the StatusesController.

Randy Schmidt sent us a project of his called SignOut, which he wrote for work and which “lets people sign in and out for meetings/lunch from their browser.” It’s a pretty simple application; an administrator first uses the admin interface to add the employees to the system, and employees can then go in and mark themselves as “away” or “not away”. (The repository is here, and we’ll be reviewing revision 47…which I can’t link to directly, unfortunately.)

As it stands, there are only a few things about the application that make me cringe; for the most part I have no complaint with how it was architected. So instead of pointing out the little nitpicks, I thought this might be a prime opportunity to demonstrate the thinking processes that go into taking an existing non-RESTful application, and redesigning it as a RESTful app.

What is REST?

REST is a term that is getting a lot of traffic these days. At the risk of grossly oversimplifying, it basically refers to a way of writing your app such that the underlying resources are exposed via a consistent, URL-based interface formed from the basic HTTP operations of “get”, “post”, “put”, and “delete”. (I have absolutely no doubt that some will take issue with this definition; please feel free to expound on your own view of the technique by commenting below.)

Sound like a lot of work? It’s not, really, especially if you do it RESTfully from the get-go, instead of reworking an existing application after the fact. But even if it is easy, why should you bother? That’s a valid question, and one worth addressing before going into this any further.

Why REST?

First of all, a RESTful application will generally have more controllers, but each controller will be smaller. Smaller objects are happy objects. Smaller objects are easier to maintain. Smaller objects are easier to refactor. Smaller objects are, quite frankly, better than bigger objects.

Secondly, a RESTful application in Rails is guided by convention, which means all RESTful Rails apps will share some basic things, like the format of their URLs. This means that we can now write a framework for consuming RESTful web services, and have that framework easily communicate with any application that conforms to those Rails conventions. And guess what? That framework already exists, and is called ActiveResource.

Thirdly (and I openly acknowledge that this is largely a matter of opinion), RESTful apps look a lot cleaner on the inside. Each line of code is nicely groomed and placed on it’s shelf. Order prevails. Chaos quails.

Convinced yet? Alrighty then. Let’s move on.

Identifying Resources

So, when writing a RESTful app, the first step is to identify the resources that you want to expose, and then create at least one controller for each of those resources. It is tempting to think of your models as this set of resources, but that’s not actually correct. In the SignOut application, there is only one model: Employee. But by looking at the actions of the two existing controllers, I was able to extract three different resources.

As the application is written, here’s the list of all of its actions:

• #index: show all employees
• #add: creates a new employee record
• #edit: displays the employee edit form (if called via GET) or updates a specific employee (if called via POST)
• #delete: deletes an existing employee

• #set_view: just sets a :view key in the session, which determines whether the status view should be brief or verbose.
• #status: shows a list of all employees in the system, along with their status. From this view, employees may mark themselves as “away”, or “available”.
• #periodic and EmployeeController#search: returns all employees matching the given criteria. These are pretty much the same operation, with the difference that #periodic is called every 10 seconds (so the view remains up-to-date).
• #set_away_info: returns the HTML form (via AJAX) that employees may use to make themselves as “away”. It allows things like the reason to be specified, along with the state.
• #set_away: updates the Employee record with the information specified by the #set_away_info form.
• #come_back: clears the away status information from the Employee’s record.

First off, note that the AdminController is the part that actually controls the employees, creating, reading, updating, and deleting them. Thus, in my refactoring, the AdminController gets renamed to EmployeesController (note the plural; controllers in a RESTful Rails app are named for the plural of the resource they control).

Second, note that the EmployeeController in the original version doesn’t really control the employees, it controls the employees’ statuses. So, I add a StatusesController. Lastly, note the EmployeeController#set_view action. This manipulates neither the employee, nor an employee’s status. It changes the view of an employee’s status, which suggests a third resource: views, which will be managed by a ViewsController.

So in my refactoring the three controllers and their actions are:

• #index: show all employees
• #new: display the “new employee” form
• #create: create a new employee
• #show: display the “edit” form for a specific employee
• #update: save an existing employee’s data
• #destroy: delete an existing employee

• #index: show the statuses of all employees. Specify a query parameter of :q for searches that filter this view.
• #show: show the away-info form for a specific employee
• #update: mark an employee as away
• #destroy: mark an employee as having come back from being away (“destroy” the away status)

• #show: sets the requested view in the session, and redirects back to StatusesController#index.

I’ll go through and implement these in subsequent articles, but don’t let that discourage you! Go ahead and think about how you would reimplement some of these yourself. Perhaps you’d even break the existing application into different resources than I did; feel free to try it out and see how it works.

Carl Fyffe submitted a small snippet of an existing application, asking how we’d polish it. We’ve decided to try out a new format for this kind of micro-refactoring; let us know what you think.

Before:

1
2
3
4
5
6
7
8
9
10
11
12

# returns Hash of Engagements keyed on date.
def self.find_by_year_month(year, month)
  date = Time.gm year, month
  engagements = {}
  all = find(:all, :conditions => ["start >= ? and start <= ?", date.last_month.end_of_month, date.next_month.beginning_of_month], :order => :start)
  all.each do |e|
    key = e.start.strftime('%Y%m%d')
    engagements[key] = [] unless engagements.has_key?(key)
    engagements[key].push e
  end
  engagements
end

After:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36

def self.find_by_year_month(year, month)
  # We're dealing with Dates, so let's use the Date class. Date can
  # be a little slower than Time, but as this method isn't going to
  # perform thousands of operations per request, we'll be fine.

  requested_date = Date.new(year, month, 1)

  # The range in the original code went from the last day of the 
  # previous month, to the first day of the following month.

  # Date#- subtracts days from a given date, which lets us find
  # the end of last month.

  from = requested_date - 1

  # Date#>> lets us advance requested date by one month, 
  # giving us the first of the next month.

  to =  requested_date >> 1

  # The BETWEEN operator lets you simplify the SQL, and it more 
  # clearly matches the intent.

  engagements = find(:all, :conditions => ["start BETWEEN ? and ?", from, to],
                           :order      => :start)

  # Enumerable#group_by returns a hash with the date strings as keys
  # and an array of the corresponding engagements as the values.
  # If you find yourself initializing a collection, adding some items
  # then returning it, you can probably find a method to tidy that
  # up.

  engagements.group_by do |engagement|
    engagement.start.strftime("%Y%m%d")
  end
end

Working in the AssetsGraphed source code has been fun, and I want to take this chance to thank John Topley once again for submitting it. He’s done a great job, and it’s a great application, make no mistake of that. It takes a lot of courage to spread your code before all the world, and have people pick and poke at it. Thanks, John!

For this last article, I want to focus on a pattern that hearkens back to the Skinny Controller, Fat Model idea that I wrote about on my blog, back in October. The goal, here, is to slim down the following action from the IncomingController:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31

def create
  asset = Asset.find(session[:asset_id])
  incoming = Incoming.create
  incoming.asset_id = asset.id
  incoming.category_id = params[:incoming][:category_id]
  incoming.transaction_date = get_date(
    params[:incoming]['transaction_date(1i)'],
    params[:incoming]['transaction_date(2i)'],
    params[:incoming]['transaction_date(3i)'])
  incoming.amount = params[:incoming][:amount]
  incoming.balance = Balance.create(:asset_id => asset.id,
    :incoming_id => incoming.id,
    :transaction_date => incoming.transaction_date,
    :amount => asset.balance + incoming.amount)
  if incoming.save
    redirect_to asset_path(session[:username], asset.id)
  else
    flash[:notice] = "Amount #{incoming.errors.on(:amount)}"
    redirect_to :back
  end
rescue ArgumentError => exception
  case exception.to_s
    when 'invalid date'
      flash[:notice] = 'Please select a real date.'
    else
      flash[:notice] = 'Please enter a numeric amount.'
  end
  incoming.destroy
  redirect_to :back
end

I’m going to assume that the refactoring I recommended in Part 1 has been applied. That is to say, Incoming and Outgoing have been combined into a single model called Item. I’m also going to assume that the refactoring that Koz recommended in Part 2 has also been done, such that current_account is a method that always returns the account associated with the current session.

So, let’s take it a bit at a time. First, consider the first line:

1
2

asset = Asset.find(session[:asset_id])

This is referring to an asset-id that was stored in the session by the AssetController, in the show action. My advice here is to not do that. In general, store as little in the session as possible. Suppose someone wants to view two assets side-by-side, by opening them in separate windows? BOOM. They clobber each other, and you wind up with bizarre errors like incoming or outgoing items being added to the wrong asset.

Instead, encode the asset ID in the URL. You can do this easily with the RESTful routes like so:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23

# in config/routes.rb, this allows for routes like
# item_path(asset, item) --> "/assets/:asset_id/items/:id"
map.resources :assets do |assets|
  assets.resources :items
end

# in the view, you need both asset and item in order to build
# the URLs.
link_to("Item", item_path(@asset, @item)

# in the controller, you can use a before_filter to extract
# the @asset variable consistantly
class ItemsController < ApplicationController
  before_filter :find_asset

  #...

  private
    def find_asset
      @asset = current_account.assets.find(params[:asset_id])
    end
end

Now, you can rely on @asset existing wherever you need it to within that controller, and you can drop that first line of the #create action altogether.

For the next bit, we can collapse almost that entire method into a single create statement, as Koz explained in the last article:

1
2

@asset.items.create!(params[:item])

Note a few things about this. First, we don’t care about the returned item instance, since we never need to reference it. We just ignore it, resting comfortably in the knowledge that the record has been recorded in the database. Secondly, the get_date hokey-pokey that the original code performs is not necessary, since Rails will automatically detect and combine attributes named with the “(1i)” (etc.) suffix. Thirdly, by adding an after_create callback, you can take care of building and assigning the new Balance instance:

1
2
3
4
5
6
7
8
9
10
11
12
13

class Item < ActiveRecord::Base
  after_create :record_balance

  #...

  private
    def record_balance
      create_balance :asset_id => asset_id,
        :transaction_date => transaction_date,
        :amount => asset.balance + balance
    end
end

Finally, notice that I used the “bang” version of the #create method; this means that if any validation fails, a RecordInvalid exception is raised. We can use that to handle the case of an invalid record:

1
2
3
4

rescue ActiveRecord::RecordInvalid => error
  flash[:notice] = "Amount #{error.record.errors.on(:amount)}"
  redirect_to :back

Fortunately, RecordInvalid includes a record accessor that returns the record that failed validation, so we just reference that in the notice.

If everything works as expected and no exceptions are raised, we just redirect back to the containing asset:

1
2

redirect_to asset_path(session[:username], @asset)

Note that you don’t need to do @asset.id, since the routing code will call #to_param on each argument, and the default ActiveRecord::Base#to_param implementation just returns the id. It’s all about convention, baby. :)

So, the final, refactored version:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15

def create
  item = @asset.items.create!(params[:item])
  redirect_to asset_path(session[:username], @asset)
rescue ActiveRecord::RecordInvalid => error
  flash[:notice] = "Amount #{error.record.errors.on(:amount)}"
  redirect_to :back
rescue ArgumentError => exception
  if exception.to_s == 'invalid date'
    flash[:notice] = 'Please select a real date.'
  else
    flash[:notice] = 'Please enter a numeric amount.'
  end
  redirect_to :back
end

Much tighter and easier to read. This is a pretty powerful pattern, actually—once you wrap your mind around it, you’ll find it hard to go back to the fat controller philosophy. Not only is it more elegant, but it’s easier to test, since you can now test the entire item creation process independently of the controller. But that’s a whole ‘nother can o’ worms, and we won’t be going there today!

Thanks again, John, it’s been fun.

John Topley has been kind enough to submit his AssetsGraphed application to us for review. He even set up a publicly-accessible subversion repository for (a subset of) the code: http://johntopley.com/svn/projects/assetsgraphed/.

AssetsGraphed is, according to the site, “a simple asset tracking application that also tracks your data.” “Assets”, in this sense, is referring to financial assets, so you can keep track of your spending. Koz and I will be spending a few articles discussing better ways to approach some of the programming problems John has tackled in this application.

For this first article, I’d like to focus on how to refactor away some of the grosser kinds of code duplication. One of the first questions John asked us in his submission was about how to clean up the duplication in the “incoming” and “outgoing” controllers and models. (Compare them here: incoming and outgoing controllers; incoming and outgoing models.) As you can see, the two are almost completely word-for-word identical—a poster child for refactoring if there ever was one.

Let’s begin by looking at the models. In this case, the only textual difference between the two files is the name of the model:

1
2
3
4
5

$ diff app/models/incoming.rb app/models/outgoing.rb 
1c1
< class Incoming < ActiveRecord::Base
---
> class Outgoing < ActiveRecord::Base

Given that the two models have that level of similarity, my first suspicion was that they were really the same model, simply considered from different directions. What if, instead of Incoming and Outgoing models to track “money you are getting” and “money you are spending”, we simply defined a single “Item” model that encapsulated all transactions?

The differentiator between “incoming” and “outgoing” transactions then simply becomes the sign on the amount. “Incoming” transactions are positive numbers, and “outgoing” transactions are negative. As long as an index exists that includes the “base_amount” column, queries that ask for “base_amount < 0” and “base_amount >= 0” should be plenty efficient.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17

class DefineItemsTable < ActiveRecord::Migration
  def self.up
    create_table :items do |t|
      t.column "asset_id",         :integer
      t.column "category_id",      :integer
      t.column "transaction_date", :date
      t.column "created_on",       :datetime
      t.column "base_amount",      :integer
    end

    # for finding all items by asset
    create_index :items, %w(asset_id base_amount)

    # for finding all items by category
    create_index :items, %w(category_id base_amount)
  end
end

(That second index, for finding all items by category, may be superfluous, unless the application needs to retrieve items by category.)

Once that new table and model is in place, we can add some methods to the Asset class for returning the “incoming” and “outgoing” items:

1
2
3
4
5
6
7
8
9
10
11
12

class Asset < ActiveRecord::Base
  # all items, incoming and outgoing
  has_many :items, :dependent => :destroy_all do
    def incoming
      @incoming ||= find(:all, :conditions => "base_amount > 0")
    end

    def outgoing
      @outgoing ||= find(:all, :conditions => "base_amount < 0")
    end
  end
end

The “do…end” syntax on the has_many association simply extends the association with an anonymous module containing the given methods. In other words, we can now do something like this:

1
2
3
4
5
6
7
8
9
10
11

# given some asset...
asset = Asset.find(params[:id])

# we can return the list of _all_ items...
asset.items

# or only the incoming items...
asset.items.incoming

# or only the outgoing items...
asset.items.outgoing

1
2
3
4
5

class Asset < ActiveRecord::Base
  has_many :items, :dependent => :destroy_all 
  has_many :incoming_items, :class_name => 'Item', :conditions => "base_amount > 0"
  has_many :outgoing_items, :class_name => 'Item', :conditions => "base_amount < 0"
end

Once the models are unified, the task of unifying the duplicated controllers follows naturally, resulting in a single ItemsController. The client must simply submit the item amount as either positive (for incoming) or negative (for outgoing), and the categorization occurs automatically. If there is a concern about requiring users to enter negative numbers, you can pass an extra parameter to the create action that indicates whether the number should be negative or not.

I really love extreme refactorings like this—there is very little in programming that gives me as much pleasure as axing code.

Todd Huss has submitted the file import code for Wind and Tides. Periodically the site needs to fetch data from the NOAA and update the local database. Here’s his import code:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25

class Wind < ActiveRecord::Base

  validates_presence_of :created, :wind_location_id
  
  belongs_to :wind_location
  
  # Get all of the wind locations, then fetch, parse, and save the data
  def self.batch_update!(wind_locations = WindLocation.all)
    wind_locations.each do | wind_location |
      raw_noaa_data = Net::HTTP.get_response(URI.parse(wind_location.url)).body
      raise "Received unexpected wind response from NOAA: " + raw_noaa_data unless raw_noaa_data=~/TIME/
      save_wind!(raw_noaa_data, wind_location)
    end
  end
  
  # Take a text based NOAA forecast, parse it, populate the model objects, and save it
  def self.save_wind!(noaa_text, wind_location)
    # ...
  end      

  # NOAA publishes 181809Z for Wind which we need to turn into YEAR/MONTH/18 18:09 UTC
  def self.convert_noaa_to_time(noaa_time, now = Time.now.getutc)
    # ...
  end 
end

There’s one major structural change I’d make, and a few minor enhancements.

The structural change I’d make is to move the downloading and parsing code out of the model and into a utility class. This lets you keep your models focused on your application, and have the file import code tucked away in the lib directory. At the same time, if we tie the importer to a single WindLocation and remove the loop in the batch_update! method, we make the focus tighter:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26

class WindImporter
  class << self
    def batch_update!(wind_location)
      raw_noaa_data = Net::HTTP.get_response(URI.parse(wind_location.url)).body
      raise "Received unexpected wind response from NOAA: " + raw_noaa_data unless raw_noaa_data=~/TIME/
      save_wind!(raw_noaa_data, wind_location)
    end
    
    # Take a text based NOAA forecast, parse it, populate the model objects, and save it
    def save_wind!(noaa_text, wind_location)
      # ...
    end      
    
    # NOAA publishes 181809Z for Wind which we need to turn into YEAR/MONTH/18 18:09 UTC
    def convert_noaa_to_time(noaa_time, now = Time.now.getutc)
      # ...
    end
  end
end

class WindLocation < ActiveRecord::Base
  has_many :winds
  def batch_update!
    WindImporter.batch_update!(self)
  end