幫我重構這個令人討厭的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 

這清楚地方式隔開的關注。人造絲應該知道用戶更新它時會發生什麼（功能的名稱可以改進到你真正想要的含義，因爲這對我來說並不完全清楚）。 它可能更短，但我喜歡明確寫出，如果最後一個用戶與當前相同，則不需要執行任何操作。否則，需要採取行動。如果我理解正確。