コードの再ファクタリング - メソッドを分離するためにコードを移動する必要がありますか？

Question

私は、解析された文書をキーワードのデータベースと比較するコントローラを持っています。典型的なスニペットは、解析された文書をループするカプセル化されたループ、一致する単語の数を数え、一致する単語の配列を構築するキーワードのデータベースです。ここに例があります：

@key_words = KeyWords.all 
@count = 0 
@array_of_matching_words = [] 
@parsed_document.each do |a| 
@key_words.each do |word| 
    if a =~ /#{key_word.word}/ 
    @count = @count+1 
    @array_of_matching_words = @array_of_matching_words.push "#{key_word.word}" 
    end 
end 

@countと@array_of_matching_wordsのインスタンス化された変数の両方がビューに渡されます。私は単語の他のデータベースのためのそれらの断片の数を蓄積して、私は別の方法にこのコードのいくつかを移動してコードを再因子化しようとしています。私が検討しているオプションの

一つは、私は、ちょうど私のアクションコントローラに

get_the_number_and_the_list_of_matching_words 

を残すコントローラ

def get_the_number_and_the_list_of_matching_words 
    @key_words = KeyWords.all 
    @count = 0 
    @array_of_matching_words = [] 
    @parsed_document.each do |a| 
    @key_words.each do |word| 
    if a =~ /#{key_word.word}/ 
     @count = @count+1 
     @array_of_matching_words = @array_of_matching_words.push "#{key_word.word}" 
    end 
    end 
end 

でプライベートメソッドにコードを移動することで、ビューが取得されます@countと@array_of_matching_wordsの両方

私はこのコーディング "スタイル"がひどいことがわかりました。 「get_the_number_and_the_list_of_matching_words」メソッドは青色で表示され、何もインスタンス化されず、パラメータは渡されず、インスタンス化された変数（@count、@array_of_matching_words）は表示されません（私の.rbファイルの末尾にプライベートメソッドのセクションを参照してください）。これは本当にコードを再因子化する良い方法ですか、それとも別のオプションがありますか？メソッドがインスタンスメソッドでも、パラメータを渡して新しい変数を返す必要がないメソッドでも、コードをメソッドに移動することは意味がありますか？

Answer 1

get_the_number_and_the_list_of_matching_wordsのようなメソッド名はあまりにも長く、それが何をしているのかを伝えるのにそれほど長くする必要がある場合、あなたのメソッドは設計によって過度に野心的です。小さな塊に分割してください。

あなたのサンプルメソッドは、引数として渡されないものに依存する3つの異なるインスタンス変数を設定します。魔法のように仮定されているだけで、@array_of_matching_wordsを定義する以外の明確な目的はありません。

一般的に、変数名の中に何かの種類のようなものは、無関係の情報です。ここでは@matching_wordsが非常に好ましい。

@key_wordsと@parsed_documentの両方の比較が行われるロジックを、パラメータとして渡します。Railsは、そのようなメソッドを「懸念」に入れることを奨励しています。特定のモデルに適用できる場合は、そこに適切な方法があるかどうかを確認してください。あなたはその後、別の@count変数を持つのではなく、@matching_words.lengthを使用することができ

def prepare_key_words_matching 
    @key_words = KeyWords.all 

    @matching_words = KeyWords.matching(@parsed_document) 
end 

：

所望の出力を生成するように拡張することができているものは何でもKeyWords、例えば、あなたのコードを作ることは、このように多くの見えます。

Answer 2

これは個人的な好みの質問かもしれませんが、ここに行きます。

可能な限り小さく分割することをお勧めします。メリットは2倍です：

• 新しいメソッドを読み込んでいる人を考えてみましょう。わずか数行であれば（他のいくつかの方法にかかわらず）、従うのがより簡単になります
• 論理を他の方法に委任すれば、テストがより簡単になります。メソッド内でメソッドが呼び出されるのは、そのロジック全体をテストするのではなく、テストする必要があるだけです。

メソッド内のさまざまなメソッドを使用すると、特にインスタンス変数の場合は難読化されますが、そのメソッドを見て、何が起きているのかを知るためにそれを呼び出す必要があるだけです。