複数のリファクタリング方法がもつ各々のメリット・デメリットを知りたい

Question

知りたいこと

2つの異なるリファクタリング方法について、それぞれのメリット・デメリットと使うべき場面を比較しつつ理解したい。

元コード

def casual_introduce(first_name, age)
  p [
    "my name is #{first_name}.",
    "i'm #{age} years old."
  ]
end

def formal_introduce(first_name, family_name, age)
  p [
    "my name is #{family_name}, #{first_name} #{family_name}.",
    "i'm #{age} years old."
  ]
end

実行結果は以下となる。

casual_introduce("taro", 78) #=> ["my name is taro.", "i'm 78 years old."]
formal_introduce("taro", "yamada", 78) #=> ["my name is yamada, taro yamada.", "i'm 78 years old."]

リファクタリング1

以下のコードは共通部分を別メソッドに抽出している。

def casual_introduce(first_name, age)
  p [
    "my name is #{first_name}.",
    common_introduce(age)
  ]
end

def formal_introduce(first_name, family_name, age)
  p [
    "my name is #{family_name}, #{first_name} #{family_name}.",
    common_introduce(age)
  ]
end

private

def common_introduce(age)
  "i'm #{age} years old."
end

こうすれば、共通部分を同時に変更したいときに手を入れる箇所が１箇所のみとなるため、変更のコストが少なく、変更漏れのリスクも小さくできる。

リファクタリング2

以下のコードは汎用的なメソッドを作成している。

def casual_introduce(first_name, age)
  introduce(first_name, age)
end

def formal_introduce(first_name, family_name, age)
  introduce(first_name, family_name, age)
end

private

def introduce(first_name, family_name = nil, age)
  introduce =
    if family_name.nil?
      ["my name is #{first_name}."]
    else
      ["my name is #{family_name}, #{first_name} #{family_name}."]
    end
  introduce.push("i'm #{age} years old.")
  p introduce
end

これもまた、先程とは異なる方法によって、共通部分を１箇所にまとめている。
共通部分の修正コスト及び修正漏れリスクを低減するというメリットも生じているので、リファクタリングと呼べると思う。

知りたいこと

上記のリファクタリング1とリファクタリング2について、それぞれのメリット・デメリットと、「どのような場面でどれを使うべきなのか」という判断基準をもちたい。

ご教示の程よろしくお願いします。

Answer 1

前提として、こういった問題は各個人の好みやプロジェクト方針等で大幅に変わる物だというのは承知おきください。「プロジェクトが変わったらAさんの意見が翻った」、「同じプロジェクトのAさんとBさんでまったく逆のことを言っている」というようなことがよく起きます。
また、個人的には、先にリファクタリングの手法を挙げて「どの場合にどれを使う」を気にするよりも、その時々で手法を考えたほうが、そのコードに合った方法をとりやすいと考えています(実際にやってみて、可読性、保守性が高いか等を考えてみて、自分なりに"イケる"と思った方法を取ればよいと思います)。質問者さんが挙げた他にもいくらでも方法はあると思いますので。

以上を踏まえたうえで、最近のメイン業務がコードレビューになってしまっている者の私見をご参考までに。

• リファクタリング１

共通化できない処理が全く別もの、且つその処理が長い、そして複数ある関数の中の一部だけ(3関数中2つだけとか)共通化できるものがある場合に有用な方法だと思います。または、一部だけは絶対に変わらない場合とか。
ただ、例の"My name is #{family_name}"ように同じ文字列定義が残ってしまうのは余り良くないと思います。
この方法の難点は、同じ形式の関数(hoge_introduce)が増えた時、名前の定義以外の行を全てコピーしなければいけないことです。そのとき、common_introduceが不要なのに入ってしまったり、必要なのに入っていなかったりということが起こりうる(バグが起こりやすい)と思います。

• リファクタリング2

共通化できない処理が似ている、且つその処理が短い、そしてそれ以降追加されない場合に有用な方法だと思います。
共通化できない箇所が長い場合は、この方法だと関数のほぼ全てがif-elseに含まれることになり、可読性が低いです。
種類が増えたときにif-else地獄になりやすいので、拡張性も低いと言わざるを得ないと思います。
もし呼び出し側が苗字の有無によって呼び出し方を変えなければいけないならば、私は却下すると思います。呼び出し側でも、呼び出し先でも同じ判定を繰り返しているからです。

• リファクタリング3

例えば、リファクタリング2とは反対に名前の方を追い出してみる
この場合は呼び出し側は「苗字があるからこっち」とか気にせず同じ関数を使えます。ただ、名前のところを片方だけ変えたい(e.g. casualだけ"I'm"にしたい)場合等にどんどん処理が複雑になってしまう、また、使ったり使わなかったりする引数が多いと可読性が低くなります。

def generateNameString(first_name, family_name = nil)
    name = "my name is #{first_name}"
    if family_name then name << ", #{first_name} #{family_name}" end
    name << "."
end

def introduce(first_name, family_name = nil, age)
    [generateNameString(first_name, family_name), "i'm #{age} years old."]
end

familyName = "Yamada"
p introduce("Taro", familyName, 99) # => ["my name is Taro, Taro Yamada.", "i'm 99 years old."]
familyName = nil
p introduce("Taro", familyName, 99) # => ["my name is Taro.", "i'm 99 years old."]

Comments

[k-tokitoh]

丁寧な回答ありがとうございます。

> こういった問題は各個人の好みやプロジェクト方針等で大幅に変わる物

そうですよね。誰もが必ず首肯する見解を探している訳ではないのですが、自分なりの答えをもっておきたいと考えて質問させていただきました。

> 先にリファクタリングの手法を挙げて「どの場合にどれを使う」を気にするよりも、その時々で手法を考えたほうが、そのコードに合った方法をとりやすい

確かに、実際にリファクタリングを適用するコードは大いに個別性をもっているので、適用すべきリファクタリング方法についても個別具体的に検討した方がよりよい解に辿り着けそうですね。
今回伺ったメリット・デメリットについては「伝家の宝刀的判断基準」ではなく「自分の中の引き出しを整理して道具を取り出しやすくするための補助的な認識」に留めておこうと思います。

以下本題。

いただいた回答をもとに改めて考えを整理したところ、リファクタリングnについて、実は複数のリファクタリングをいっぺんに適用しているゆえにその特徴がみえにくくなっているのではないかと思いました。

リファクタリング1: 共通部分を別メソッドに抽出
リファクタリング2: メソッドを抽象化 及び ラッパーの導入
リファクタリング3: メソッドを抽象化 及び 条件分岐を別メソッドに抽出

1 メソッドを抽象化する

1-1 メリット
メソッドを1つに集約することができる
コードが散在することを防ぎ、その点において修正時のコスト&リスクを低減できる

1-2 デメリット
リファクタリング前の2つのメソッドの構造が相当似ている必要がある
そうでないと抽象化したメソッドの内部でややこしい分岐をする必要がある

1-3 デメリットが少なくメリットが大きくなる場面
リファクタリング前の2つのメソッドの構造が相当似ている場合

2 共通部分を別メソッドに抽出する

2-1 メリット
多くのケースに適用できる/しやすい
リファクタリング前の2つのメソッドの構造が全く異なっていても、共通部分さえあれば適用できる

2-2 デメリット
メソッド数が減らない、むしろ増える。
コードの散在を助長し、その点において修正時のコスト＆リスクが増大する

2-3 デメリットが少なくメリットが大きくなる場面
リファクタリング前の2つのメソッドの構造が全く異なっている場合

上記の理解についてもしコメントあればぜひお願いします。

「ラッパーの導入」及び「条件分岐を別メソッドに抽出」は改めて考察しようと思います。
取り急ぎの御礼と、いったんの理解の整理まで。

[moredeep]

>誰もが必ず首肯する見解を探している訳ではない
明確な判断基準を持っていると「どんな時も自分が考えが必ず正しい」と考える方も一定数いらっしゃいますので念のため付け加えましたが、余計なお世話だったようですね。失礼しました。

>実は複数のリファクタリングをいっぺんに適用しているゆえにその特徴がみえにくくなっているのではないか
この考え方は目から鱗でした。例に出されたケースについての最終系しか考えていませんでした。都度考えろとか偉そうに言っておいて、自分のほうが考え方が固まってしまっているとは。。。

各手法に対する整理については特に問題があるようには見えません。こういうのをきちんと調査・整理することは素晴らしいと思います。(私は「なんとなくこうなったら綺麗じゃない？」とか、感覚的にレビューしてしまっています・・・反省しないとですね。)

[k-tokitoh]

> 各手法に対する整理については特に問題があるようには見えません。
ありがとうございます！引き続き整理してみます。

3 ラッパーの導入

3-1 メリット
インターフェースが改善できる
（＝メソッド名や引数の数・名前を呼び出し元の文脈に適合させられる）

3-2 デメリット
メソッドの数・コードの量が増えるため、メンテナンスコストの増大や可読性の低下を招く場合がある。

3-3 デメリットが少なくメリットが大きくなる場面
ラップする前のメソッドと呼び出し元の文脈とが適合的でない場合。
（＝メソッド名から何をやっているのかが分かりにくい、呼び出しごとに変更する必要のない値が引数として設けられている）

4 条件分岐を別メソッドに抽出

4-1 メリット
メソッドの可読性が向上する。
再利用可能性が高まる。

4-2 デメリット
メソッドの数が増えるため、メンテナンスコストの増大や可読性の低下を招く場合がある。

4-3 デメリットが少なくメリットが大きくなる場面
条件分岐が複雑な場合。
条件分岐による処理が長大な場合。

以上の理解を踏まえて今回の質問を振り返ってみます。
「共通部分をもつ2つのメソッドa, b」に対してリファクタリング1, 2を行った結果「a, b両方に関わるメソッド1つと、a, bそれぞれに関わるメソッドが1つずつ」という状態になりました。
この結果の相同性から、リファクタリング1, 2の比較を試みたいと考えましたが、それは困難なことでした。なぜならリファクタリング1は1つの方法を、リファクタリング2は役割の異なる2つの方法を適用した結果としてたまたま最終的な形が似ただけであり、本来並列的なものではなかったからです。

上記の理解に誤り、不足点、意見等あればご指摘ください。
今回理解の助けをいただいたmoredeepさんをベストアンサーとさせていただきます。
ありがとうございました！