Carl Weis Carl Weis - 3 months ago 10
Ruby Question

How to refactor method with multiple boolean variables in Ruby

I have a method in ruby which sets a few instance variables, conditionally and I'm wondering how I could refactor it to clean it up and make it less verbose. My first though was to break the different conditions up into multiple smaller helper methods, but I'm not sure if that is the right way to go about it. Any advise would be helpful.

def admin_view
if resource.present?
if resource.ed_level == 'group'
if current_user && (current_user.admin || resource.admins_byemail.include?(current_user.email))
@admin_full = true
@admin_edit = true
@admin_view = true
else
@admin_full = false
@admin_edit = false
@admin_view = false
end
else
if current_user && (current_user.admin || resource.admin_email_list('view').include?(current_user.email.downcase))
if current_user.admin || (resource.admin_email_list('full').include?(current_user.email.downcase) && resource.ed_level != 'group')
@admin_full = true
@admin_edit = true
@admin_view = true
elsif resource.admin_email_list('edit').include?(current_user.email.downcase) && resource.ed_level != 'group'
@admin_full = false
@admin_edit = true
@admin_view = true
elsif resource.admin_email_list('view').include?(current_user.email.downcase) && resource.ed_level != 'group'
@admin_full = false
@admin_edit = false
@admin_view = true
end
else
@admin_full = false
@admin_edit = false
@admin_view = false
end
end
else
redirect_to school_missing_path
end
end


Based on the answer below, I've updated my code as follows.

def admin_view
if resource.present?
if resource.ed_level == 'group'
if current_user && (current_user.admin || resource.admins_byemail.include?(current_user.email))
set_admin_permissions(full: true, edit: true, view: true)
else
set_admin_permissions(full: false, edit: false, view: false)
end
else
if current_user && (current_user.admin || resource.admin_email_list('view').include?(current_user.email.downcase))
if current_user.admin || (resource.admin_email_list('full').include?(current_user.email.downcase) && resource.ed_level != 'group')
set_admin_permissions(full: true, edit: true, view: true)
elsif resource.admin_email_list('edit').include?(current_user.email.downcase) && resource.ed_level != 'group'
set_admin_permissions(full: false, edit: true, view: true)
elsif resource.admin_email_list('view').include?(current_user.email.downcase) && resource.ed_level != 'group'
set_admin_permissions(full: false, edit: false, view: true)
end
else
set_admin_permissions(full: false, edit: false, view: false)
end
end
else
redirect_to school_missing_path
end
end

private

def set_admin_permissions(full:, edit:, view:)
@admin_full = full
@admin_edit = edit
@admin_view = view
end

Answer

First of all you may want to look at using CanCanCan to properly encapsulate your permissions. This is a more formal way of defining access restrictions and testing for them in your controller and view code.

That being said, you can boil down your code considerably if you structure your code a little differently:

def admin_permissions
  return [ ] unless resource.present?

  case resource.ed_level
  when 'group'
    if current_user && (current_user.admin || resource.admins_byemail.include?(current_user.email))
      [ :full, :edit, :view ]
    else
      [ ]
    end
  else
    email = current_user && current_user.email.downcase

    if current_user && (current_user.admin || resource.admin_email_list('view').include?(email))
      if current_user.admin || resource.admin_email_list('full').include?(email)
        [ :full, :edit, :view ]
      elsif resource.admin_email_list('edit').include?(email)
        [ :edit, :view ]
      elsif resource.admin_email_list('view').include?(email)
        [ :view]
      end
    else
      [ ]
    end
  end
end

Then use this like so:

@admin_privs = admin_permissions

Define some helper methods like this:

def admin_full?
  @admin_privs and admin_privs.include?(:full)
end

def admin_edit?
  @admin_privs and admin_privs.include?(:edit)
end

def admin_view?
  @admin_privs and admin_privs.include?(:view)
end

Personally I've found that reducing duplication in your code by applying the "Don't Repeat Yourself" (DRY) principle often exposes the underlying structure and makes it easier to reshape it into something more concise and flexible.

For example, there were a number of tests here for resource.ed_level != 'group' when by virtue of being in the else block of a test asserting the opposite there was no way that would ever not be the case.