Query actions in Rails controllers

Recently some of my controller actions have taken on a definite new shape. Particularly when the action is a read-only query of the app’s state. Such actions tend to make up the bulk of my apps, and they can be simple because they are unlikely to “fail” in any predictable way. Here’s an example from my wiki app:

This has a couple of significant plus points: First, no instance variables, so both the controller action and the view are more readable and easier to refactor. Second, no instance variables! So there’s a clear, documented, named interface between the controller and the view. And third, this is so transparently readable that I never bother to test it.

The wiki used in the above action is a repository, built in a memoizing helper method that most of the controllers use:

In this case the correct kind of repository is created for the current user, and all of the other code in this request sits on top of that. So the controller action, helped by the memoized repository builder, effectively constructs an entire hexagonal architecture for each request, and the domain logic is thus blissfully unaware of its context.

Here’s a slightly bigger example. This is for a page that shows a variety of informative graphs about the wiki; and because I may want to re-organise my admin pages in the future, each graph’s data is built independently of the others:

That’s the most complex query controller action I have, and I maintain that it’s so simple I don’t need to test it. Would you?

Advertisements

Hexagonal rails: Rake tasks are adapters

If I’m thinking about my Rails app in terms of a hexagonal architecture, I find it also pays to consider every rake task to be an Adapter. Thus:

Picture: @rosieemackenzie

The true picture is a little more complicated than that, but the principal ideas are there. The rake task acts as a mediator, allowing me to send messages to (ie. call methods on) my application’s objects.

In general, we want our Adapters to be as thin as possible (and no thinner). That’s because the Adapter code inherently depends on some framework, and that fact will usually make its tests difficult and/or slow. We still need those tests, but we want to have as few paths through the Adapter as possible, and thus fewer tests of the Adapter, so that the total test complexity and test run-time are minimized.

As an example, suppose I have a rake task that validates the posts and comments on my blog:

I want to maximize the unit tested percentage of the code executed during the task, so I move the code out of the rake task and into a new domain object. That new object “is” the task, and usually my rake tasks can then slim down to a single line of code.

Stripping all of the code out of the rake task and moving it into a domain object is analogous to the approach currently being explored by @mattwynne and @tooky in their Rails controller refactorings. In my own code I call these task objects “use case objects”. Currently I keep them in a UseCases namespace within app/models, although I’m open to exploring alternative conventions. One of the nice side-effects of this is that I sometimes discover synergies between the work done by rake tasks and that done by controllers. By pulling Use Case objects out of both kinds of adapter I’m creating a convention for some of the code that turns out to be cleaner, more (quickly) testable, and which names things better.

Please let me know if you try this — or indeed if you’re already doing it.

Hexagonal rails: Hiding the finders

This is a brief follow-up to the Hexagonal Rails sessions I did last week at the Scottish Ruby Conference with Matt and Steve. We tried to cram a 3-day course into 45 minutes, with inevitable consequences. So by way of an apology, here’s another brief foray into some of the same territory…

Today I’ve been exploring ways of improving the unit tests in one of my Rails apps. Let’s pretend it’s a blog app, and I have to add a feature to publish posts. I’ve pulled some code out of a controller to make a UseCase object:

(Here ui is a controller, and makes decisions about routing and rendering in response to the events sent by the use case object.)

I’m really sick and tired of looking at code like this. It seems that most of the classes in any Rails app know that finders return nil when they can’t find something, and so they all have to cope with that case. I want to get rid of that if, or at least hide it away in a single place so that it doesn’t contaminate the rest of the app. I want the test to be a bit simpler and more readable too. And I want the option to have richer finders, maybe coping with fuzzy edge cases and still finding the desired database record. In short, I want to wrap the finder in an Adapter.

So I experimented with a couple of alternatives to the above. First, a variant on Smalltalk’s ifTrue:ifFalse message argument style:

I don’t like this because it’s near impossible to isolate and test the interactions. For example, Blog.should_receive(:lookup_post).with( … what?

Another approach might be something like this:

Indeed Rails itself uses this style in its controllers, and so does the inherited_resources gem. But it suffers from the same testing problems as my previous attempt. So I finally settled on this:

This version uses an explicit listener object, which I can thus instantiate and test independently of the calling context. (In real code I might make it an inner class too.) And I’ve already found a couple more uses for the lookup_post method, removing dependencies and ifs along the way.

I find this approach highly readable and testable, nicely separating different responsibilities and allowing me to give them names. But that doesn’t mean I’ll stop exploring. Have you tried other approaches? How did they work out?

Update

Luke points out that I forgot to include the option I actually use the most, the Self Shunt:

This is the same as the listener option above, but the UseCase object passes itself as the listener, thus saving a class and in my opinion) improving readability a little. Can’t think why I forgot it, cos it’s the best and neatest of them all. Anyroadup, thanks Luke!

Conditionals on the edge

As you know, I have a thing about conditional code. Most conditionals are duplicates, and the only “genuine” conditionals are at the system boundaries, where they test external state and input information. But I discovered recently that, even at the edges, not every conditional is necessary…

Here’s (a drastically simplified version of) some code I wrote a while back. (It’s from a Rails app, but the message in this post applies to any technology and any kind of system input.)

class Users::RegistrationsController < Devise::RegistrationsController
  def create
    user = create_new_user_account
    if params[:invitation_token]
      sign_in_as(user)
      redirect_to Invitation.find(params[:invitation_token]).shared_content
    else
      redirect_to new_user_session_path
    end
  end
end

This route handles incoming user registrations, and it has a conditional branch to cater for the case in which the new user was invited, via email, by an existing user. If the new user arrived by clicking a link in an email we do one set of things; whereas we do a different set of things if the user got here via the ‘Register now’ button on the home page. Perfectly reasonable, I thought. Sure, it’s conditional code; but it’s sitting on the system boundary and therefore it’s fine, yes? No.

This conditional check represents duplication. Somewhere else in my application already knew, at some time in the recent past, which route should be taken through this method when it was finally invoked. When the app sent that email invitation, it was in a sense delegating to the recipient the task of supplying user account details. The conditional branch that handles the invitation is a callback from across a technology boundary, and the inviting code already knew to expect it.

The duplication also has a nasty side-effect: high complexity. This method is an order of magnitude harder to understand and to test(-drive) because of the conditional construct. (And you’re reading a version from which I’ve elided all manner of error handling.) Oh, and by forcing two routes into the same method I also made it violate the Single Responsibility Principle.

Happily the duplication is really easy to remove. The code that sends the email invitation knows which path it wants the registration code to take, so I just need to create a new input route for “invitation acceptance” and change the email invitation to link to it. As a result I’ll have two controller methods, each with simpler logic than their predecessor:

class Users::RegistrationsController < Devise::RegistrationsController
  def create
    user = create_new_user_account
    redirect_to new_user_session_path
  end
end

 

class InvitationAcceptancesController < Devise::RegistrationsController
  def create
    user = create_new_user_account
    sign_in_as(user)
    redirect_to Invitation.find(params[:invitation_token]).shared_content
    end
  end
end

So I have now removed the duplication, and the controller code is simpler as a result. These controller methods are also much easier to test(-drive) than the earlier code. And as a final bonus, I can design the user interface to this route with more knowledge, maybe adding some details of the invitation so that the incoming potential user feels a little more at home. In short, now that each code path has its own method (and class) I can pay it more attention, and it will flourish as a result.

Now, having refactored this code, I take away a few points for the future:

  • Where appropriate and possible, model system outputs as delegations into a space that your code can’t reach or control; and model the resulting future input as a response.
  • Where a system input is due to the system having earlier delegated a responsibility across its boundary, make sure the return route is differentiated from all others.
  • Differentiated incoming routes have code that is easier to read and simpler to test.
  • When an input route has a single use, the user experience can be tailored and enriched.
  • Not every conditional at the system boundary tests external state.

Why I don’t use spork

Spork is great. And so is guard and its family of plugins. Early this year I spent a while converting all of my rails projects to use spork, and we even had a team standard tmux setup that ran spork in one of the start-up screens. So every time we saved a file, guard/spork ran some of our specs. We even had growl/notify pop up a little message telling us the result, so we didn’t have to go to the trouble of switching screen to find out. How very efficient!

But now I’ve turned spork off, and here’s why: Spork solves the wrong problem.

We enabled guard/spork because our specs were slow, and during the few months we had spork, those specs became even slower. But we never noticed. Most of the time guard/spork guessed correctly which specs to run, and we became quite good at configuring guard to re-load Rails when critical files changed. So we were reasonably comfortable, and our specs ran quickly most of the time. But our design was getting worse every day.

Slow specs are a sign of too much coupling, and in the case of a Rails app that usually means not enough separation between the domain objects and the adapters. So we’ve stopped running spork, so that we can feel the pain of the 15-second Rails load time, and so that we can feel the pain of all that coupling. And slowly we are making the app’s specs faster. Many spec files now run in under a hundredth of a second, and more will follow as we gradually peel domain code out from the Rails infrastructure. We don’t need to test the Rails components, and we have a very good suite of cucumber features that act as end-to-end integration tests. So our specs should be fast, and only need to tell us that our own objects each do their thing. That’s what we are now working towards, and that’s the pot of gold that spork was hiding from us.

Faster Rails controller specs

One of the Rails apps I work on has this:

$ rspec spec
#...
Finished in 61.82 seconds
475 examples, 0 failures

61 seconds!  (And on top of that I have to wait another 15 seconds for Rails load; that’s a whole other story, and I hope to come back to that in a future post.) A quick dig into the 61 seconds reveals this:

$ rspec spec/controllers
#...
Finished in 22.82 seconds
114 examples, 0 failures

It happens that almost every controller action in our app needs the user to be logged in, so every controller spec needs that too. We’re using Devise, so every spec has something equivalent to this:

before :each do
  sign_in Factory(:user)
end

This implies hits on the database to create the user object, record the IP address and last sign-in time, etc etc. We can do much better by using a mock_model for the User, and stubbing out Warden‘s authentication completely.

First, in the interests of DRYness, I made some helper methods to do some of the jobs that factory_girl does:

module RandomHelpers
  def random_number() SecureRandom.random_number(10000); end
  def random_id() random_number; end
  def random_name() SecureRandom.hex(20); end
end

Next, a method that creates a test double representing the User who will be logged in:

module ControllerHelpers
  def user_double(attrs = {})
    user_attrs = {
      :first_name => random_name,
      :last_name => random_name,
      :authenticatable_salt => 'x'
    }.merge(attrs)
    mock_model(User, user_attrs)
  end
end

This is the place to fill the User double with default attribute values. Our app requires every User to have a first and last name, so I’ve stubbed these with random strings; they can be overridden by the caller if specific values are needed for any test. In your own app, change this method to set any mandatory attributes your user model needs. Note also the stubbed authenticatable_salt() method, which wasn’t required when mocking earlier versions of Devise.

I also need a method that logs a user in:

module ControllerHelpers
  def stub_sign_in_with(user)
    request.env['warden'] = double(Warden,
                                   :authenticate => user,
                                   :authenticate! => user,
                                   :authenticate? => true)
    sign_in(user)
    return user
  end
end

This replaces Warden with a test double, and Devise is none the wiser.
I also create a method to glue these together for the simplest (and most common) case:

module ControllerHelpers
  def stub_sign_in(attrs = {})
    stub_sign_in_with user_double(attrs)
  end
end

(Note that this returns the User double; you’ll see why that’s useful in a moment.)

Finally, I configure the specs so they have access to these helper methods. To achieve that, spec/spec_helper.rb needs to look something like this:

Dir[Rails.root.join("spec/support/**/*.rb")].each {|f| require f}

RSpec.configure do |config|
  config.include RandomHelpers
  config.include Devise::TestHelpers, :type => :controller
  config.include ControllerHelpers, :type => :controller
end

Putting all this together allows me to write controller specs with:

before do
  stub_sign_in
end

or, if I need access to the user double:

let(:user) { stub_sign_in }

And the payoff?

$ rspec spec/controllers
#...
Finished in 7.36 seconds
114 examples, 0 failures

I saved as whole 15 seconds! There’s still some work to do in my specs, but stubbing out Warden has made a massive difference to every test run that involves this project’s controller microtests.

Note that the above has been tested only with the following gem versions:

  • rails 3.0.3
  • devise 1.3.4
  • warden 1.0.4
  • rspec-core 2.6.4
  • rspec-mocks 2.6.0
  • rspec-rails 2.6.1
  • factory_girl 1.3.3

A condensed gist for the above code is here.

interviewed at scottish ruby conf

Way back in March of this year, while I was attending the Scottish Ruby Conference, Werner Schuster interviewed me for InfoQ. The video has been online since August, and this is me finally getting around to telling you about it; watch it here. I talk about Reek, Refactoring in Ruby, and agile development in general.

This was the first time I’ve done a video interview, so I’m keen to hear your comments on how it went!