Controller refactoring

Controller-refactoring.rb

def users
  per_page = Variable::default_pagination_value
  @users = User.find(:all)
  # First, check to see if there
  # was an operation performed
  if not params[:operation].nil? then
    if (params[:operation] == "reset_password") then
      user = User.find(params[:id])
      user.generate_password_reset_access_key
      user.password_confirmation = user.password
      user.email_confirmation = user.email
      user.save!
      flash[:notice] = user.first_name + " " +
      user.last_name + "'s password has been reset."
    end
    if (params[:operation] == "delete_user") then
      user = User.find(params[:id])
      user.item_status = ItemStatus.find_deleted()
      user.password_confirmation = user.password
      user.email_confirmation = user.email
      user.save!
      flash[:notice] = user.first_name + " " +
      user.last_name + " has been deleted"
    end
    if (params[:operation] == "activate_user") then
      user = User.find(params[:id])
      user.item_status = ItemStatus.find_active()
      user.password_confirmation = user.password
      user.email_confirmation = user.email
      user.save!
      flash[:notice] = user.first_name + " " +
      user.last_name + " has been activated"
    end
    if (params[:operation] == "show_user") then
      @user = User.find(params[:id])
      render :template => show_user
      return true
    end
  end
  user_order = 'username'
  if not params[:user_sort_field].nil? then
    user_order = params[:user_sort_field]
      if !session[:user_sort_field].nil? &&
        user_order == session[:user_sort_field] then
        user_order += " DESC"
      end
    session[:user_sort_field] = user_order
  end
  @user_pages, @users = paginate(:users,
  :order => user_order,
  :conditions => ['item_status_id <> ?',
  ItemStatus.find_deleted().id],
  :per_page => per_page)
end

This AdminController object has a users action. The users action expects an addi-
tional parameter, operation, which takes values that determine what functionality
occurs within the action.

Controller naming is very important, and the name of the controller in this case
may indicate a problem. In a RESTful structure, the controller is named for the
resource that is being operated on. An AdminController object, then, would be
expected to operate on Admins. This is not the case.

The following code is the remainder of the controller action, for the sake of clar-
ity and thoroughness

As you can see, this one action is using the operation parameter to provide the same
functionality that would normally be present within the index, show, and destroy
actions of a UsersController, as well as additional actions for resetting a user’s pass-
word and activating a user, with URLs that looked something like the following:

  /admin/users?operation=reset_password?id=x
  /admin/users?operation=delete_user?id=x
  /admin/users?operation=activate_user?id=x
  /admin/users?operation=show_user?id=x
  /admin/users

Before we go any further in identifying the changes and solutions to this problem,
we need to note the importance of using an automated test suite when making large
refactorings like this. If this application didn’t have a test suite (it probably wouldn’t),
then it’s recommended that one be written. The most appropriate types of tests would
be integration tests using a tool such as Cucumber. These tests allow you to prevent
regressions because it should be possible to write the integration tests such that they
don’t fail if you haven’t broken anything. This is because integration tests operate on
the links that are clicked, the fields that are typed in, and so on, rather than on the
internal controller organization of the application.
When your integration tests have been addressed, you can refactor the monolithic
controller into one or more RESTful controllers. Fortunately, non-RESTful controller
actions are very often given the name that your controllers should have. Let’s start by
mapping out what the new URLs will be:

  POST
  DELETE
  POST
  GET
  GET
  /admin/users/:id/password
  /admin/users/:id
  /admin/users/:id/activation
  /admin/users/:id
  /admin/users

You need to rename AdminController to UsersController (or create a new one)
and create new PasswordsController and ActivationsController objects. Next,
you simply take the existing code from the if statements in the existing controller and
move it into the corresponding new controller actions:

class UsersController < ApplicationController
  def index
    per_page = Variable::default_pagination_value
    user_order = 'username'
    if not params[:user_sort_field].nil? then
      user_order = params[:user_sort_field]
      if !session[:user_sort_field].nil? &&
        user_order == session[:user_sort_field] then
        user_order += " DESC"
      end
      session[:user_sort_field] = user_order
    end
    @user_pages, @users = paginate(:users,
    :order => user_order,
    :conditions => ['item_status_id <> ?',
    ItemStatus.find_deleted().id],
    :per_page => per_page)
  end
  
  def destroy
    user = User.find(params[:id])
    user.item_status = ItemStatus.find_deleted()
    user.password_confirmation = user.password
    user.email_confirmation = user.email
    user.save!
    flash[:notice] = user.first_name + " " +
    user.last_name + " has been deleted"
  end
  
  def show
    @user = User.find(params[:id])
    render :template => show_user
  end
end

class PasswordsController < ApplicationController
  def create
    user = User.find(params[:id])
    user.generate_password_reset_access_key
    user.password_confirmation = user.password
    user.email_confirmation = user.email
    user.save!
    flash[:notice] = user.first_name + " " +
    user.last_name + "'s password has been reset."
  end
end

class ActivationsController < ApplicationController
  def create
    user = User.find(params[:id])
    user.item_status = ItemStatus.find_active()
    user.password_confirmation = user.password
    user.email_confirmation = user.email
    user.save!
    flash[:notice] = user.first_name + " " +
      user.last_name + " has been activated"
    end
  end
end

Now, with functionality organized into these new controllers, the routes for these
controllers would be as follows:
  
  namespace :admin do
    resources :users do
      resource :passwords
      resource :activations
    end
  end

As you review this code in more detail, you’re likely to see many other things that
could be improved in it. While fixing this example is fairly straightforward, it’s important
that you tackle only one issue at a time. You should refactor to a RESTful controller
first and then continue improving the code from there. Making a controller RESTful
often exposes better improvements and keeps things easier to organize. In addition, by
tackling one item at a time, you lessen the risk of getting lost or overwhelmed.