帮我重构这个令人讨厌的Ruby if/else语句

Question

所以我在我的应用程序的通讯分发中有这个大方法。方法是更新人造丝，我需要分配一个用户人造丝。我通过表colporteur_in_rayons关系n：n，其具有属性since_date和until_date。

我是一名初级程序员，我知道这段代码非常虚拟:) 我很感谢每一个建议。

def update 
    rayon = Rayon.find(params[:id]) 
    if rayon.update_attributes(params[:rayon]) 
    if params[:user_id] != "" 
     unless rayon.users.empty? 
     unless rayon.users.last.id.eql?(params[:user_id]) 
      rayon.colporteur_in_rayons.last.update_attributes(:until_date => Time.now) 
      Rayon.assign_user(rayon.id,params[:user_id]) 
      flash[:success] = "Rayon #{rayon.name} has been succesuly assigned to #{rayon.actual_user.name}." 
      return redirect_to rayons_path 
     end 
     else 
     Rayon.assign_user(rayon.id,params[:user_id]) 
     flash[:success] = "Rayon #{rayon.name} has been successfully assigned to #{rayon.actual_user.name}." 
     return redirect_to rayons_path 
     end 
    end 
    flash[:success] = "Rayon has been successfully updated." 
    return redirect_to rayons_path 
    else 
    flash[:error] = "Rayon has not been updated." 
    return redirect_to :back 
    end 
end 

Answer 1

def update 
    rayon = Rayon.find(params[:id]) 

    unless rayon.update_attributes(params[:rayon]) 
     flash[:error] = "Rayon not updated." 
     return redirect_to :back 
    end 

    puid = params[:user_id] 
    empty = rayon.users.empty? 

    if puid == "" or (not empty and rayon.users.last.id.eql?(puid)) 
     msg = "Rayon updated.", 
    else 
     msg = "Rayon #{rayon.name} assigned to #{rayon.actual_user.name}.", 
     rayon.colporteur_in_rayons.last.update_attributes(
      :until_date => Time.now) unless empty 
     Rayon.assign_user(rayon.id, puid) 
    end 

    flash[:success] = msg[msg_i] 
    return redirect_to rayons_path 
end 

Answer 2

以下是我对它的看法。我在内部写了一堆评论，描述了为什么我改变了我所做的。

def update 
    rayon = Rayon.find(params[:id]) 
    if rayon.update_attributes(params[:rayon]) 
     user_id = params[:user_id] # using same value a couple times 
     # if-else strongly preferred over unless-else... 
     # so replace `unless foo.empty?` with `if foo.length > 0` 
     if user_id.length > 0 # then string != "" 
      # `if A && B` more readable than `unless X || Y` in my opinion 
      if rayon.users.length > 0 && rayon.users.last.id != user_id 
       rayon.colporteur_in_rayons.last.update_attributes(:until_date => Time.now) 
      end 
      # same line in if-else; pull outside 
      Rayon.assign_user(rayon.id, user_id) 
      flash[:success] = "Rayon #{rayon.name} has been successfully assigned to #{rayon.actual_user.name}." 
     else 
      flash[:success] = "Rayon has been successfully updated." 
     end 

     # all branches in here return this value, pull outside 
     return redirect_to rayons_path 
    else 
     flash[:error] = "Rayon has not been updated." 
     return redirect_to :back 
    end 
end 

我注意到你的代码最大的问题是你在第一个if块中有很多重复。真正的部分中的所有分支都达到了return redirect_to rayons_path，因此被拉到了块的末尾。你有两次相同的flash[:success] = ...，所以也被拉出。

if-else比unless-else更可读，所以尽量避免使用后者。我不是一个粉丝，除非立即嵌套在另一个中，所以我将它改为复合if语句。

Rails中的一个主要成语是DRY（不要重复自己），因此请尝试确定做的有重复代码的地方。

_{（另请注意，我没有测试过这个重构 - 我敢肯定，逻辑仍然是相同的，但我不能证明这一点）}

Answer 3

def update 
    rayon = Rayon.find(params[:id]) 

    unless rayon.update_attributes(params[:rayon]) 
    flash[:error] = "Rayon has not been updated." 
    return redirect_to :back 
    end 

    if params[:user_id].empty? 
    msg = "Rayon has been successfully updated." 
    else 
    if rayon.users.empty? 
     Rayon.assign_user(rayon.id,params[:user_id]) 
     msg = "Rayon #{rayon.name} has been successfully assigned to #{rayon.actual_user.name}." 
    elsif not rayon.users.last.id.eql?(params[:user_id]) 
     rayon.colporteur_in_rayons.last.update_attributes(:until_date => Time.now) 
     Rayon.assign_user(rayon.id,params[:user_id]) 
     msg = "Rayon #{rayon.name} has been succesuly assigned to #{rayon.actual_user.name}." 
    end 
    end 
    flash[:success] = msg 
    return redirect_to rayons_path 
end 

Answer 4

有一些双重代码。所以删除。而且，控制器中有一些业务代码，不应该在那里。

def update 
    rayon = Rayon.find(params[:id]) 

    if rayon.update_attributes(params[:rayon]) 
    if params[:user_id] != "" 
     rayon.handle_update_user(params[:user_id] 
     flash[:success] = "Rayon #{rayon.name} has been succesuly assigned to #{rayon.actual_user.name}." 
    else 
     flash[:success] = "Rayon has been successfully updated." 
    end 
    return redirect_to rayons_path 
    else 
    flash[:error] = "Rayon has not been updated." 
    return redirect_to :back 
    end 
end 

控制器方法现在显然与需要采取的行动的交易，并在必要时进行相应的设置闪光方法：所以我如下应重构控制器代码。

在丽阳模型，你写的时候更新由用户完成做什么需要的代码：

class Rayon 

    def handle_update_user(user_id) 
    if (!users.empty? && users.last.id.eql?(params[:user_id])) 
     # do nothing! 
    else 
     colporteur_in_rayons.last.update_attributes(:until_date => Time.now) unless users.empty? 
     Rayon.assign_user(rayon.id,params[:user_id]) 
    end 

    end 

end 

这清楚地方式隔开的关注。人造丝应该知道用户更新它时会发生什么（功能的名称可以改进到你真正想要的含义，因为这对我来说并不完全清楚）。 它可能更短，但我喜欢明确写出，如果最后一个用户与当前相同，则不需要执行任何操作。否则，需要采取行动。如果我理解正确。