Update: Nicolas pointed out the User.find_and_activate! method had a serious bug where the user could pass a nil activation_code param and it would find the first user with a nil activation code and activate them. I’ve updated the model method to raise an ArgumentError if a nil activation code is passed in. Thanks Nic!

Jamis Buck has been blogging like a bat out of hell coming out of christmas, totalling 26 posts in all. If you’re a Rails dev and haven’t checked out his articles you should do so right away. I just wish I lived in his vincinity so I could pick his brains at Rails meetups.

Jamis recently blogged about skinny controllers and fat models, which I had also been thinking about and agree it feels like a great way to go. As an aside though, I don’t like the idea of doing all your HTTP parameter parsing in the model, but maybe that’s ok in your model until you feel the need to refactor it.

Getting out of the controller making business decisions mindset is tough. Most of the books written, and most of the apps written until now, have made controllers the starting point for determining application behaviour and they treat ActiveRecord models as simple data stores with a few extra methods and overrides. For example, here’s some of my code that handles user activation:

class UsersController < ActionController::Base
  def activate
    unless user = User.find_by_activation_code(params[:activation_code])
      flash[:notice] = 'Activation code not found.'
      redirect_to must_activate_path and return
    end

    if user.activated?
      flash[:notice] = 'Your account has already been activated. You can log in below.'
      redirect_to login_path and return
    end

    user.activate
    self.current_user = user # log in user
    flash[:notice] = "Your account has been activated!" 
    redirect_back_or_default('/')
  end
end

and the corresponding User model method:

class User < ActiveRecord::Base
  def activate
    @activated = true
    self.attributes = {:activated_at => Time.now.utc, :activation_code => nil}
    save(false)
  end
end

So what’s wrong with the above? Firstly, the controller’s main purpose isn’t really obvious, it begins 12 lines down at user.activate.

The UsersController#activate specification, if it had one, would read:

Activate the user with the corresponding activation code, log them in and redirect them to the home page.

If the activation code is invalid, display some error message.

If the account is already activated, display some error message.

Does the implementation read similarly to the spec? Nope.

So if we forget for the moment about handling wrong activation codes and reactivations, we could refactor the controller to read more like the spec:

class UsersController < ActionController::Base
  def activate
    self.current_user = User.find_and_activate!(params[:activation_code])
    flash[:notice] = "Your account has been activated!" 
    redirect_back_or_default('/')
  end
end

Much simpler, and we even removed a temporary variable. So how about handling those two edge cases?

We could use if statements and have the controller check the model and make decisions in a similar fashion to our first implementation, but that’d ruin the way our action reads, and its making the controller more intelligent about the activation process than it needs to be.

Every if statement makes a method’s behaviour less obvious. Refactoring away nasty nested ifs is a great past time of mine.

Instead of explicitly handling the edge cases with ifs we’ll just make the meat of the method assume things went well, and then handle exceptions (aha!) after the fact.

class UsersController < ActionController::Base
  def activate
    self.current_user = User.find_and_activate!(params[:activation_code])
    flash[:notice] = "Your account has been activated!" 
    redirect_back_or_default('/')
  rescue User::ActivationCodeNotFound
    flash[:notice] = 'Activation code not found.'
    redirect_to must_activate_path
  rescue User::AlreadyActivated
    flash[:notice] = 'Your account has already been activated. You can log in below.'
    redirect_to login_path
  end
end

Compare that to the first implementation of the activate action.

# The old UsersController#activate implementation
class UsersController < ActionController::Base
  def activate
    unless user = User.find_by_activation_code(params[:activation_code])
      flash[:notice] = 'Activation code not found.'
      redirect_to must_activate_path and return
    end

    if user.activated?
      flash[:notice] = 'Your account has already been activated. You can log in below.'
      redirect_to login_path and return
    end

    user.activate
    self.current_user = user # log in user
    flash[:notice] = "Your account has been activated!" 
    redirect_back_or_default('/')
  end
end

What does the model implementation look like now?

class User < ActiveRecord::Base

  class ActivationCodeNotFound < StandardError; end
  class AlreadyActivated < StandardError
    attr_reader :user, :message;
    def initialize(user, message=nil)
      @message, @user = message, user
    end
  end

  # Finds the user with the corresponding activation code, activates their account and returns the user.
  # 
  # Raises:
  #  +User::ActivationCodeNotFound+ if there is no user with the corresponding activation code
  #  +User::AlreadyActivated+ if the user with the corresponding activation code has already activated their account
  def self.find_and_activate!(activation_code)
    raise ArgumentError if activation_code.nil?
    user = find_by_activation_code(activation_code)
    raise ActivationCodeNotFound if !user
    raise AlreadyActivated.new(user) if user.activated?
    user.send(:activate!)
    user
  end

  private
    def activate!
      @activated = true
      self.update_attribute(:activated_at, Time.now.utc)
    end
end

There’s a few neat things worth noting in the above snippet; nested classes for neatly tucking away the Exceptions, making User.find_and_activate the only public way to activate a user, and a custom intialiser for the AlreadyActivated exception in case the controller wants to know which user we’re talking about when we raise the error.

We’ve ended up with a much cleaner and intelligent model, we formalised the edge cases as actual Exceptions (which means the edge cases can be nicely documented via RDoc) and cleaned up the controller.

I think it simply comes down to:

Models want to be smart about everything business specific.

ActionControllers want to be smart about HTTP and request control-flow.

1. We’re still learning.
2. The community actually care about this stuff (one of the things I love most about this community).
3. Rails is flexible enough in its opinions that we can still change the way we’re approaching problems.
4. The community (and Jamis in particular) is open to admitting they don’t know all the answers and are sharing knowledge instead of getting on high horses or climbing ivory towers.

Time for the ActionController weight-watchers plan.