1772298f2d14152c13bbd3221e6ab6ec

I have this method as part of a security module included in my ApplicationController to help verify whether or not a member can access a page or not. It's based on an app with two models, an Account model that has many Members, and a Member model that belongs to an Account. current_member is a method from the built in Authentication gem I'm using (Devise), and current_account is a separate method that let's the application always know what account is being accessed.

I can't help but think I could refactor the unless condition in a more concise way. It's essentially saying that if a member does not belong to an account, and is not an admin, don't allow them to access it, and redirect them.

I'd love to hear any refactoring ideas!

def verify_admin!
    unless ((!params[:account_id].blank?) && (params[:account_id].to_i == current_account.id) && (params[:id].to_i == current_member.id)) || current_member.is_admin?
      flash[:alert] = "Can't access this page"
      redirect_to account_member_path(current_member.account, current_member) and return 
    end
end

Refactorings

No refactoring yet !

D41d8cd98f00b204e9800998ecf8427e

steved

May 14, 2010, May 14, 2010 18:03, permalink

No rating. Login to rate!

- push logic into model (it's fine to pass params into a model method)
- changed name from verify_admin!; you're verifying more than admin, right?

class MyController < ApplicationController
  before_filter :verify_access

  # action methods

  private

  def verify_access
    unless current_member.can_access_page?(params)
      flash[:alert] = "Can't access this page"
      redirect_to account_member_path(current_member.account, current_member) and return 
    end
  end

end

class Member < ActiveRecord::Base
  belongs_to :account
  
  def can_access_page?(params)
    is_admin? || (account_id == params[:account_id].to_i && id == params[:id].to_i)
  end
  
end

Your refactoring





Format Copy from initial code

or Cancel