Comment refactoriser la méthode avec plusieurs variables booléennes dans Ruby

Question

J'ai une méthode dans ruby ​​qui définit quelques variables d'instance, conditionnellement et je me demande comment je pourrais le refactoriser pour le nettoyer et le rendre moins bavard. Ma première idée était de décomposer les différentes conditions en plusieurs petites méthodes d'aide, mais je ne suis pas sûr que ce soit la bonne façon de procéder. Tout conseil serait utile.

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 

Basé sur la réponse ci-dessous, j'ai mis à jour mon code comme suit.

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

Tout d'abord, vous pouvez vouloir utiliser CanCanCan pour encapsuler correctement vos autorisations. C'est une façon plus formelle de définir les restrictions d'accès et de les tester dans votre contrôleur et afficher le code.

Cela dit, vous pouvez faire bouillir considérablement votre code si vous structurez votre code un peu différemment:

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 

Ensuite, utilisez cela comme si:

@admin_privs = admin_permissions 

Définir des méthodes d'aide comme celui-ci :

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 

Personnellement, j'ai trouvé que la réduction de la duplication dans votre code par application mentir le principe «Ne vous répétez pas» (DRY) expose souvent la structure sous-jacente et facilite le remodelage en quelque chose de plus concis et flexible.

Par exemple, il y avait un certain nombre de tests ici pour resource.ed_level != 'group' quand en raison d'être dans le bloc else d'un test affirmant le contraire, il n'y avait aucune façon que ce ne serait jamais le cas.

Answer 2

juste faire une méthode d'aide setter, comme ceci:

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

bâtiment hors de l'idée de Maxim, mais remarquant que vos autorisations sont hiérarchiques (c.-à « plein » implique modifier & vue et « modifier » implique vue), je réduis votre méthode d'aide à ceci:

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 

Et puis votre code devient:

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

Si vous trouvez tous les ifs imbriqués et la logique répétée un peu confuse. N'oubliez pas que vous pouvez utiliser l'instruction return pour rendre le code plus propre. Je ne peux pas garantir que la logique ci-dessous est exactement ce que vous recherchez, mais la structure est, à mon avis, plus lisible.

def admin_view 
    redirect_to school_missing_path unless resource.present? 
    access_level = calc_access_level 

end 

def calc_access_level 

    return :none unless resource.present? 
    return :none unless current_user 
    return :full if current_user.admin 

    email_raw = current_user.email 
    email = email_raw.downcase 

    if (resource.ed_level == 'group') 
     return resource.admins_byemail.include?(email_raw) ? :full, :none 
    end 

    ['view','full','edit'].each do |access_level| 
     if resource.admin_email_list(access_level).include?(email) 
     return access_level.to_sym 
     end 
    end 

    return :none 

end 

def set_access_level(level) 

    @admin_full, @admin_edit, @admin_view = false, false, false 
    case level 
    when :full 
    @admin_full, @admin_edit, @admin_view = true, true, true 
    when :edit 
    @admin_edit, @admin_view = true, true 
    when :view 
    @admin_view = true 
    end 
end