ruby-on-rails - Rails リファクタリング コード

Question

以下のコードがありました。

  @brand = Brand.find_by_id(params[:id])
  if @brand.nil?
    flash[:error] = Flash.record_not_found
    redirect_to admin_brands_path
  end

そして、以下への別の変更：

@brand = Brand.find_by_id(params[:id])
return(flash[:error] = Flash.record_not_found and redirect_to admin_brands_path) if @brand.nil?

どのコードがより効率的で説明的だと思いますか? 別の提案がある場合は、共有することもできます。

前もって感謝します。

Answer 1

私は次のようにします：

def action
  @brand = Brand.find(params[:id])
rescue ActiveRecord::RecordNotFound
  redirect_to admin_brands_path, flash: {error: Flash.record_not_found}
end

Answer 2

上の方のコードの方が分かりやすく綺麗なので良いと思いますが、以下のように書いても構いません。

unless @brand = Brand.find_by_id(params[:id])
  flash[:error] = Flash.record_not_found
  redirect_to admin_brands_path
end

Answer 3

最初のオプションは間違いなくはるかに優れています-疑いの余地はありません。読みやすく、内部のロジックが多すぎず、コントローラーが行うべきことだけを行います。コードの行数を最小限に抑えることは、実際には良い指標ではありません。

リファクタリングに関しては、そのままにしておきます。#find_by_id を #find に変更するかもしれませんが、それだけです。