I am refactoring my controllers, trying to improve the way common data is loaded for the actions.
Initially I was using before_filter
methods to do this but read that helper methods were preferred.
Can I please get some feedback on the following controller?
Thanks
Initial Code
class CategoriesController < ApplicationController
before_filter :authenticate_user!
helper_method :category, :categories, :organisation
# GET /categories
def index
end
# GET /categories/new
def new
end
# GET /categories/1/edit
def edit
end
# POST /categories
def create
if category.save
redirect_to(
categories_path(oid: organisation.id),
notice: I18n.t("category.create.success"))
else
render action: :new
end
end
# PUT /categories/1
def update
if category.update_attributes(params[:category])
redirect_to(
categories_path(oid: organisation.id),
notice: I18n.t("category.update.success"))
else
render action: :edit
end
end
# DELETE /categories/1
def destroy
begin
category.destroy
redirect_to(
categories_path(oid: organisation.id),
notice: I18n.t("category.delete.success"))
rescue ActiveRecord::DeleteRestrictionError
redirect_to(
categories_path(oid: organisation.id),
alert: I18n.t("category.delete.failure.foreign_key"))
end
end
private
# Gets the category being operated on.
#
def category
return @category unless @category.nil?
if params[:id]
@category = Category.find_by_id(params[:id])
elsif params[:category]
@category = Category.new(params[:category])
else
@category = Category.new({organisation_id: organisation.id})
end
end
# Gets the organisation of the Category.
#
# If no Category exists (e.g. in the case of actions index
# and new) there will be a querystring parameter 'oid' indicating the
# Organisation.
#
def organisation
return @organisation unless @organisation.nil?
if params[:oid]
@organisation = Organisation.find_by_id(params[:oid])
elsif params[:category]
@organisation = Organisation.find_by_id(
params[:category][:organisation_id])
elsif !category.nil?
@organisation = category.organisation
else
@organisation = nil
end
end
# Gets the existing Categories for the organisation.
#
def categories
return @categories unless @categories.nil?
if !organisation.nil?
@categories = Category
.where(organisation_id: organisation.id)
.order(:name)
.all
else
@categories = nil
end
end
end
First Revision
class CategoriesController < ApplicationController
helper_method :category, :categories, :organisation
# GET /categories
def index
@category = Category.new
@category.organisation = organisation
end
# GET /categories/new
def new
@category = Category.new
@category.organisation = organisation
end
# GET /categories/1/edit
def edit
end
# POST /categories
def create
@category = Category.new(params[:category])
if @category.save
redirect_to(
categories_path(oid: @category.organisation_id),
notice: I18n.t("category.create.success"))
else
render action: "new"
end
end
# PUT /categories/1
def update
if category.update_attributes(params[:category])
redirect_to(
categories_path(oid: category.organisation_id),
notice: I18n.t("category.update.success"))
else
render action: "edit"
end
end
# DELETE /categories/1
def destroy
begin
category.destroy
redirect_to(
categories_path(oid: category.organisation_id),
notice: I18n.t("category.delete.success"))
rescue ActiveRecord::DeleteRestrictionError
redirect_to(
categories_path(oid: organisation.id),
alert: I18n.t("category.delete.failure.foreign_key"))
end
end
private
# Gets the Category being operated on.
#
def category
@category ||= Category.find(params[:id])
end
# Gets the Organisation of the Category.
#
# If no organisaton id has been supplied as a querystring parameter
# (required for the index and new actions) derive the Organisation
# from the Category.
#
def organisation
@organisation ||=
params[:oid] ? Organisation.find(params[:oid]) : category.organisation
end
# Gets the existing Categories for the Organisation.
#
def categories
@categories ||=
organisation ? organisation.categories.order(:name) : nil
end
end