1
\$\begingroup\$

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.

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
\$\endgroup\$

3 Answers 3

1
\$\begingroup\$

To load data, you can see the gem Cancan. It think it can do something like you do. I find your code very clear.

Instead of this:

if !organisation.nil?

You can do this:

if organisation.present?

Instead of this:

@categories = Category
  .where(organisation_id: organisation.id)
  .order(:name)
  .all

You can do this:

organisation.categories.order(:name)

Instead of using the method find_by_id you can use just the method find.

You did a good job!

\$\endgroup\$
0
0
\$\begingroup\$

Instead of this:

render action: "new"

you can do this, considered good practice:

respond_to do |format| 
  format.html { render action: 'new', status: :ok }
end

For strings without interpolation, it's good to use single quotes:

"new"

'new'

# Gets the Category being operated on.
  #
  def category
    @category ||= Category.find(params[:id])
  end

find() throws exceptions, so it's better to handle exceptions:

# Gets the Category being operated on.
  #
  def category
    @category ||= Category.find(params[:id])
  rescue ActiveRecord::RecordNotFound
    flash[:notice] = 'Record Not found'
    @category = Category.new
  end
\$\endgroup\$
1
  • \$\begingroup\$ It appears that you've partially reviewed the newer revision, which shouldn't have been added. Could you remove that part and maybe replace it with something else in the original? \$\endgroup\$
    – Jamal
    Commented Dec 30, 2014 at 17:58
0
\$\begingroup\$

Instead of this:

@category ||= Category.find(params[:id])

and this:

@category = Category.new

and this:

@category = Category.new(params[:category])

You can use CanCan like this:

class CategoriesController < ApplicationController
  load_resource
  ...
end

See the doc at load_resource.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.