@the_rails_way.awaken!

After more than 15 months since the last post, and constant questions from users, I’m finally ready to bring The Rails Way back from hibernation.

The challenge I had here was the amount of time involved. Review articles are incredibly time consuming, scouring an application for code to improve can take hours, making the changes takes time, and all of that is dependent on getting that perfect submission.

So while I intend to continue to do review pieces here (keep those submissions coming in) I’m going to extend the format here to include a few different kinds of posts. I’ll be doing some focussed introductory pieces which cover the best practices for a few tricky areas that I see experienced rails programmers getting wrong. I’ll also be doing a few ‘soapboxy’ pieces where I can address misinformation about Rails and Ruby or just advocate a particular piece of technology or code that I think is really cool.

One of my primary goals with this relaunch will be to post regularly, but I’m not going to try and stick to a schedule that takes the fun out of it for me. Some weeks might see multiple posts, and others will see none at all. I’m just hoping that you guys will enjoy most of them.

Many Skinny Methods

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
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
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.

Using ActiveResource to Consume Web-services

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.

Testing the Right Stuff

I’m going to take a slightly different tack here, and review some of the unit tests in rails itself. They show up two common anti patterns, spurious assertions and coupling your tests to the implementation.

Perhaps the biggest benefit of a suite of unit tests is that they can provide a safety net, preventing you from accidentally adding new bugs or introducing regressions of old bugs. With a large codebase, the unit tests can also help new developers understand your intent, though they’re no substitute for comments. However if you’re not careful with what gets included in your test cases, you can end up with a liability.

Be careful what you assert

Whenever you add an assertion to your test suite you’re sending a signal to future developers that the behaviour you’re asserting is both intentional and desired. Future developers who try to refactor your code will see a failing test, and either give up, or waste time trying to figure out if the assertion is ‘real’ or whether it was merely added because that’s what the code happened to do at present.

For an example, take the test_hatbm_attribute_access_and_respond_to from associations_test.rb , especially the assertions that the project responds to access_level= and joined_on=. Because of the current implementation of respond_do?, those assertions pass. But should they?

In reality while those values will get stored in the object, they’ll never be written back to the database. This is a surprising result for some developers, and removing those accessor methods would go a long way to helping avoid some frustrating moments.

Mock and Stub with care

Mock object frameworks like flexmock and mocha make it really easy to test how your code interacts with another system or a third party library. However you should make sure that the thing that you’re mocking doesn’t merely reflect the current implementation of a method. To take a case from rails, take a look at setup_for_named_route in routing_test.rb.

It takes the seemingly sensible approach of building a stubbed-out implementation of url_for instead of trying to build a full implementation into the test cases. The stubbed version of url_for simply returns the arguments it was passed, this makes it extremely easy to work with and to test.

The problem is not with stubbing out the method, but in the way it is used in all the named route test cases. Take test_named_route_with_nested_controller.

1
2
3
4
5
6
def test_named_route_with_nested_controller
  rs.add_named_route :users, 'admin/user', :controller => '/admin/user', :action => 'index'
  x = setup_for_named_route.new
  assert_equal({:controller => '/admin/user', :action => 'index', :use_route => :users, :only_path => false},
  x.send(:users_url))
end

The strange hash value you see in the assertion is the result of the named route method calling url_for, and returning that. The current implementation of the named route helpers does this, but what if you wanted to implement a new version of named routes which completely avoids the costly call to url_for? Every single named route test fails, even though applications which use those methods will work fine.

In this situation you have two options, you could make your tests depend on the full implementation of url_for. This would probably slow down your test cases, and require a lot more setup code, but because the return values are correct you’re not likely to impede future refactoring.

The other option is to use different stubs for every test case. Leaving you with something like this:

1
2
3
4
5
6
7
def test_named_route_with_nested_controller
  rs.add_named_route :users, 'admin/user', :controller => '/admin/user', :action => 'index'
  generated_url = "http://test.named.routes/admin/user"
  x = setup_for_named_route.new
  x.stubs(:url_for).returns(generated_url)
  assert_equal(generated_url,  x.send(:users_url))
end

Doing this for each and every test case is going to be quite time consuming and make your test cases extremely verbose. As with all things in software you’ll have to make a judgement call on this trade off and make a choice between coupling or verbosity.

Whatever approach you choose, just remember that misleading test ‘failures’ can slow down refactoring, and end up reducing your ability to respond to change. As satisfying as ‘100% coverage’ or 2:1 ratios may be, don’t blindly add assertions or mock objects just to satisfy a tool. Every line in your test cases should be there for a reason, and should be placed there with just as much care as you’d use for a line of application code.

Dangers of Cargo-culting

“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.

Free-for-all: Tab Helper (Summary)

The first RailsWay free-for-all came off quite well. Many of you posted your favorite solutions to the problem of tab-based navigation, as posed by Nate Morse.

Jamis’ Take

Of all the solutions posted, my personal favorite was the pragmatic and simple CSS-based solution given by Mr. eel (Nate Morse came to the same solution independently):

I take a completely different approach. I ID the body of the page with the name of the current controller. Then I use a descendent CSS selector to highlight the current tab based on the body id and an id given to each link. I don’t bother with replacing the current tab link with a span. If the user wants to click that link again… then it’s the same as refreshing. Totally up to them.

With html like:

1
2
3
4
5
6
<body id="users">
  <ul>
    <li><a href="/users" id="usersNav">Users</a></li>
    <li><a href="/comments" id="commentsNav">Comments</a></li>
    <li><a href="/posts" id="postsNav">Posts</a></li>
  </ul>

I would use CSS like this

1
2
3
4
5
6
#users #usersNav,
#comments #commentsNav,
#posts #postsNav {
  background:red;
  font-weight:bold;
}

What a great approach. Although I would make the choice of the body ID explicit (rather than depending on the controller name), it is otherwise really nice. It shrugs off the whole issue of “should the current tab be a link” by saying it just doesn’t matter—every tab is always a link. Such pragmatism gets right to the heart of the Rails Way: implement just what matters, and nothing more.

Koz’s Take

A number of solutions relied on tightly coupling the controller and tabs. While this may seem like a time-saver at first, I believe that it’s unlikely to remain useful as your application grows. You’ll find yourself moving functionality into strange locations in order to make your tabs highlight correctly.

The problem is amplified with a restful application where your choice of controllers are dictated by the resources that you’re managing. You may have a list of comments in several different sections of your application, but not want to highlight the ‘comment’ tab whenever you display them.

Personally, I prefer the really simple approach of a before filter and a navigation partial.

1
2
3
def set_current_tab
  @current_tab = :people
end

Thanks, everyone for your submissions!

RailsConf Recap: Named Callbacks

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.

Free-for-all: Tab Helper

Koz and I have opinions, and we like sharing them. This site was designed as a platform for us to express those opinions.

You all have opinions, too, and some of you even have opinions that (gasp!) differ from ours. Don’t try to deny it—we read the comments on this site, too.

Well here’s your chance play the game. Nate Morse sent us an email shortly after RailsConf in which he described a problem that Koz and I would like to present to you, our readers. How would you respond?

I’ve been trying to figure out a clean way to implement a tabbed view on a website where the selected tab is written out in a <span /> element and anything unselected is a link to the particular action. So far, this is what I’ve come up with.

1
2
3
4
5
6
7
8
9
def tab_to(name, options = {})
  url = options.is_a?(String) ? options : url_for(options.merge({:only_path => false}))
  current_url = url_for(:action => @current_action, :only_path => false)
  if (url == current_url)
    content_tag(:span, name)
  else
    link_to(name, options)
  end
end

The @current_action variable is set in a before filter and used to determine if the tab I’m specifying is for the current action. Some of this code is lifted straight from the source of link_to (which is why it will accept either an options hash or a url) and is probably overkill. Also, it seems kind of hokey that I’m setting the :only_path key in a couple of places just to get the urls in a standard form. Is there a better way to do this?

Please post your responses in the comments. Because the comments aren’t editable, you should probably draft your response externally and then paste it in. You can use textile codes to format the text. For syntax-highlighted code snippets, just enclose the snippets in <macro:code> tags, with the lang attribute set to the language of the snippet (e.g., “ruby”, or “rhtml”). For example:

<macro:code lang=”ruby”> def foo(a,b) return a + b end </macro:code>

At the end of the week, Koz and I will write up a summary, highlighting a few of the responses. Read, set, go!

RailsConf Recap: Skinny Controllers

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!

House Keeping

Things have been a bit quiet around here while Jamis and I prepare for our talk at Rails Conf. In the meantime there are a few housekeeping announcements.

Sponsor

We’d like to thank Geoff Grosenbach’s Peep Code for sponsoring the site. Peep Code are high quality screen casts that take a particlar topic in Rails development, and cover it in depth. I’ve personally paid for a few of these, so when he offered to sponsor the site we were happy to oblige.

New Search

We’ve decided to try out Google co-op instead of mephisto’s built in search. In addition to searching this site, it’ll also search the rails wiki, the api docs and our personal blogs. If you have some sites you think we should add to the index, drop us a link in the comments. We’re looking for great reference and tutorial material, there’s gotta be some great links out there.

Submissions

Thanks to everyone who’s submitted code so far. Just because we haven’t used your example yet, doesn’t mean we won’t use it in the future. We’re especially interested in examples which are small enough to be covered in a single article, so if you have anything you’re sitting on, please send it in.

All the best, and we’re looking forward to seeing you all at RailsConf 07.