このコントローラーメソッドの重さを軽減する方法を教えてください。

Question

私は次のコントローラとメソッドを持っているとしましょう。私はこのコントローラのメソッド（リスト）が重すぎるように感じる。私はこれをどのように分割すべきですか？ビューレイヤーに移動する対象と、モデルレイヤーに移動する対象は何ですか？

注：返される形式（text、：leaf、id、：clsを含むハッシュ）には、ProjectFileデータベーステーブルのフィールドとは異なるフィールドが含まれているため、実際にこのコントローラメソッドをどれだけ移動させるべきかモデルレイヤーに追加します。そして、より管理しやすい方法にnode_listを破る

ProjectFile < ActiveRecord::Base 
    def node_list(project_id, folder_id, user_id) 
    node_list = [] 

    # If no project id was specified, return a list of all projects. 
    if project_id == nil and folder_id == nil 
    # Get a list of projects for the current user. 
    projects = Project.find_all_by_user_id(user_id) 

     # Add each project to the node list. 
     projects.each do |project| 
     node_list << { 
      :text => project.name, 
      :leaf => false, 
      :id => project.id.to_s + '|0', 
      :cls => 'project', 
      :draggable => false 
     } 
     end 
    else 
     # If a project id was specfied, but no file id was also specified, return a 
     # list of all top-level folders for the given project. 
     if project_id != nil and folder_id == nil 
     # Look for top-level folders for the project. 
     folder_id = 0 
     end 

     directory_list = [] 
     file_list = [] 

     known_file_extensions = ['rb', 'erb', 'rhtml', 'php', 'py', 'css', 'html', 'txt', 'js', 'bmp', 'gif', 'h', 'jpg', 'mov', 'mp3', 'pdf', 'png', 'psd', 'svg', 'wav', 'xsl'] 

     # Get a list of files by project and parent directory. 
     project_files = ProjectFile.find_all_by_project_id(project_id, 
                :conditions => "ancestry like '%#{folder_id}'", 
                :order => 'name') 

     project_files.each do |project_file| 
     file_extension = File.extname(project_file.name).gsub('.', '') 

     if known_file_extensions.include? file_extension 
      css_class_name = file_extension 
     else 
      css_class_name = 'unknown' 
     end 

     # Determine whether this is a file or directory. 
     if project_file.is_directory 
      directory_list << { 
      :text => project_file.name, 
      :leaf => false, 
      :id => project_id + '|' + project_file.id.to_s, 
      :cls => css_class_name 
     } 
     else 
      file_list << { 
      :text => project_file.name, 
      :leaf => true, 
      :id => project_id + '|' + project_file.id.to_s, 
      :cls => css_class_name 
      } 
     end 
     end 

     node_list = directory_list | file_list 
    end 
end 

：

class ProjectFileController < ApplicationController 
    before_filter :require_user 

    def list 
    @project_id = params[:project_id] 
    @folder_id = params[:folder_id] 

    current_user = UserSession.find 
    @user_id = current_user && current_user.record.id 

    node_list = [] 

    # If no project id was specified, return a list of all projects. 
    if @project_id == nil and @folder_id == nil 
     # Get a list of projects for the current user. 
     projects = Project.find_all_by_user_id(@user_id) 

     # Add each project to the node list. 
     projects.each do |project| 
     node_list << { 
      :text => project.name, 
      :leaf => false, 
      :id => project.id.to_s + '|0', 
      :cls => 'project', 
      :draggable => false 
     } 
     end 
    else 
     # If a project id was specfied, but no file id was also specified, return a 
     # list of all top-level folders for the given project. 
     if @project_id != nil and @folder_id == nil 
     # Look for top-level folders for the project. 
     @folder_id = 0 
     end 

     directory_list = [] 
     file_list = [] 

     known_file_extensions = ['rb', 'erb', 'rhtml', 'php', 'py', 'css', 'html', 'txt', 'js', 'bmp', 'gif', 'h', 'jpg', 'mov', 'mp3', 'pdf', 'png', 'psd', 'svg', 'wav', 'xsl'] 

     # Get a list of files by project and parent directory. 
     project_files = ProjectFile.find_all_by_project_id(@project_id, 
                 :conditions => "ancestry like '%#{@folder_id}'", 
                 :order => 'name') 

     project_files.each do |project_file| 
     file_extension = File.extname(project_file.name).gsub('.', '') 

     if known_file_extensions.include? file_extension 
      css_class_name = file_extension 
     else 
      css_class_name = 'unknown' 
     end 

     # Determine whether this is a file or directory. 
     if project_file.is_directory 
      directory_list << { 
      :text => project_file.name, 
      :leaf => false, 
      :id => @project_id + '|' + project_file.id.to_s, 
      :cls => css_class_name 
      } 
     else 
      file_list << { 
      :text => project_file.name, 
      :leaf => true, 
      :id => @project_id + '|' + project_file.id.to_s, 
      :cls => css_class_name 
      } 
     end 
     end 

     node_list = directory_list | file_list 
    end 

    render :json => node_list 
    end 
end 

Answer 1

私はあなたのモデルProjectFileまたは任意の適切なモデル名であるにそのロジックのほとんどを置くことができると思います。私が上で定義した方法は長く、複数のこと（低凝集性）を行うので、それを分解することはそれらの欠点に対処するのに役立ちます。

あなたのコントローラでは、このような呼び出します。

は

class ProjectFileController < ApplicationController 
    before_filter :require_user 

    def list 
    @project_id = params[:project_id] 
    @folder_id = params[:folder_id] 

    current_user = UserSession.find 
    @user_id = current_user && current_user.record.id 

    nodes = node_list(@project_id, @folder_id, @user_id) 
    render :json => nodes 
    end 
end 

今、あなたのコントローラが読みはるかに簡単ですし、ビジネス・ロジックが抽出されます。これは、マントラの「スキニーコントローラ、脂肪モデル」に従います。

projects = Project.find_all_by_user_id(@user_id) 

：

Answer 2

あなたは、あなたはプロジェクトの配列は、これに代えて

current_user.projects 

でobejcts得ることができます

has_many => :projects 

のようなあなたのUserモデルに協会を持っている必要がありますつまり、user_idも取得する必要はありません。

node_listをUserモデルに取り込むためのすべてのロジックを配置することもできます。 folder_idがnilとゼロに設定されている場合def node_list(project_id, folder_id=0)でfolder_id=0が自動的にそれを行いますので、これはまた、あなたのチェックを外すことができ

def node_list(project_id, folder_id=0) 
    if @project_id == nil 
    list = self.projects.map do |project| 
     { 
     :text => project.name, 
     :leaf => false, 
     :id => project.id.to_s + '|0', 
     :cls => 'project', 
     :draggable => false 
     } 
    end 
    else 
    ... # the rest of your code here, etc 
    end 
    return list 
end 

お知らせ：ちょうどあなたのモデルに置きます。 ：あなたはとにかくdirectory_listとfile_listのを組み合わせているので、なぜだけに、もし-else文を組み合わせない

：また

def list 
    @current_user = UserSession.find 
    render :json => @current_user.node_list(params[:project_id], params[:folder_id]) 
end 

：

次に、あなたのコントローラは、ちょうどこのようになります。

{ 
    :text => project_file.name, 
    :leaf => project_file.is_directory, 
    :id => @project_id + '|' + project_file.id.to_s, 
    :cls => css_class_name 
} 

必要に応じて、後でアレイを並べ替えることができます。

Answer 3

@Chad、

私はあなたが私の前に投稿した人からあなたのコードのリファクタリングの良い例を持っていると思います。

あなたはルビーコードを書く能力があるようですので、私はあなたの質問に「どのようにリファクタリングするか」の観点から答えようとします。

• 各コントローラのエンドポイント（アクション）は、コントローラクラスのメソッドであり、それは「単数」責任を持つべきで適用

• フィルタ前
  • 使用スキニーコントローラを保管してください。
  • "list"メソッドは関連するデータを（適切なモデルクラスへのメソッド呼び出しを使用して）検索し、Railsにデフォルトのレンダリングをさせます。あなたはjsonを返しているので、そのための1行は問題ありません。
  • これで、モデル上で最大で1つ、例外的に2つのメソッドを呼び出す必要があります。
  • 私は「10行以上のアクション方法が間違っているなら、私のためにこの格言を思いつきました。小さな問題にあなたの問題ダウン

• キープモデル脂肪

  • ブレイク。
  • リストメソッドにコードが多すぎます。
  • おそらく高レベルでは、モデルの "セマンティック"コントローラタイプのメソッドに分解し、if then elseのプライマリジャグリングを行うことができます。
  • その他の小さな方法では残りの処理が行われます。
  • モデルメソッドにも小さなパラメータセットが必要です。パラメータが多すぎると脆くなり、パラメータがないと、あまりにも多くの状態に結びついています。
  • 1つまたは2つのパラメータを持つメソッドが良好です。オプションの3つのパラメータを持つメソッドを持つことができます。 3つ以上のパラメータがリファクタリングのために熟しています。
  • 現在のユーザーオブジェクトをモデルに渡すことはできますが、それには何も問題はありません。
  • また、そこにメソッドを追加してユーザーモデルを充実させることもできます。 （has_many：projectsを使い、関連が提供するメソッドを使用する）。
  • 私は関連文書をいつでも手元に保管しています。
  • 私はいつも私と一緒に便利なenumerablesドキュメントを保ちます。
  • Enumerablesでどのマップ、選択、拒否、検索を行うかを学習します。彼らはあなたの友達です！

幸運！ :)