Rubyで複数のブール変数を使用してメソッドをリファクタリングする方法

Question

条件付きでいくつかのインスタンス変数を設定するメソッドがありますが、リファクタリングしてどのようにリファクタリングしてあまり冗長にするのか不思議です。私の最初のことは、さまざまな条件を複数の小さなヘルパーメソッドに分割することでしたが、それが正しいかどうかはわかりません。どんな助言も役に立つでしょう。

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 

以下の回答に基づいて、コードを次のように更新しました。

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 1

まず、CanCanCanを使用してアクセス許可を適切にカプセル化するとよいでしょう。これは、アクセス制限を定義し、コントローラとビューコードでアクセス制限をテストする、より正式な方法です。あなたは少し違ったあなたのコードを構造化する場合には言われて、あなたはかなりあなたのコードを煮詰めることができ

：

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 

次にそうのようにこれを使用します。

@admin_privs = admin_permissions 

は、このようないくつかのヘルパーメソッドを定義します：

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 

個人的には、アプリでコードの重複を減らす「自分自身を繰り返さないでください」（DRY）の原則は、しばしば基礎となる構造を公開し、より簡潔で柔軟なものに簡単に変形することを容易にします。

たとえば、これとは逆の方法がないと主張する試験のブロックの中にある場合、resource.ed_level != 'group'のための多数のテストがここにありました。

Answer 2

ただ、そうのような、セッターのヘルパーメソッドを作る：

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_values(true, true, true) 
     else 
     set_values(false, false, 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_values(true, true, true) 
     elsif resource.admin_email_list('edit').include?(current_user.email.downcase) && resource.ed_level != 'group' 
      set_values(false, true, true) 
     elsif resource.admin_email_list('view').include?(current_user.email.downcase) && resource.ed_level != 'group' 
      set_values(false, false, true) 
     end 
     else 
     set_values(false, false, false) 
     end 
    end 
    else 
    redirect_to school_missing_path 
    end 
end 

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

Answer 3

ビルマキシムのアイデアのオフが、あなたの許可が「フル」、すなわち（階層化していることに気付いする編集&ビューと「編集」を意味します）ビューを意味し、私はこれまで、あなたのヘルパーメソッドを凝縮します：

def set_access_level(level) 
    case level 
    when :full 
    @admin_full, @admin_edit, @admin_view = true, true, true 
    when :edit 
    @admin_full, @admin_edit, @admin_view = false, true, true 
    when :view 
    @admin_full, @admin_edit, @admin_view = false, false, true 
    else 
    @admin_full, @admin_edit, @admin_view = false, false, false 
    end 
end 

そして、あなたのコードは次のようになります。

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_access_level(:full) 
     else 
     set_access_level(:none) 
     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_access_level(:full) 
     elsif resource.admin_email_list('edit').include?(current_user.email.downcase) && resource.ed_level != 'group' 
      set_access_level(:edit) 
     elsif resource.admin_email_list('view').include?(current_user.email.downcase) && resource.ed_level != 'group' 
      set_access_level(:view) 
     end 
     else 
     set_access_level(:none) 
     end 
    end 
    else 
    redirect_to school_missing_path 
    end 
end 

Answer 4

すべてのネストされたifと繰り返しロジックを見つけるのが少し混乱します。 return文を使用すると、コードをよりきれいにすることができます。私は以下の論理があなたが何をしているか正確には保証できませんが、私の意見では、構造がより読みやすくなります。