リファクタリングの判断基準にメソッドの呼び出し階層の深さはあるか知りたい

前提・実現したいこと

元々Java開発をしていましたが、現在Rails開発に携わっています。

メソッド階層が多いと個人的には読みづらいと思うのですが、リファクタリングの判断基準にそのような視点はありますでしょうか？

例えば以下のコードがあるとき

ruby
1  def delete_foo(x, y)
2    (1..5).each do |i|
3        {
4          "hoge_id_#{i}": x,
5          "hoge_value_#{i}": y,
6        }
7      end
8    end
9  end
10
11  def update_foo(x, y, z)
12    (1..5).each do |i|
13      if z == hoge.public_send("hoge_#{i}_id")
14        {
15          "hoge_id_#{i}": x,
16          "hoge_value_#{i}": y,
17        }
18      end
19    end
20  end
21
22  def create_foo(x, y, z)
23    (1..5).each do |i|
24      unless z.include?(i)
25        {
26          "hoge_id_#{i}": x,
27          "hoge_value_#{i}": y,
28        }
29      end
30    end
31  end

この部分をまとめられるので

ruby
1 {
2      "hoge_id_#{i}": x,
3      "hoge_value_#{i}": y,
4 }

以下のように修正すると、メソッドの呼び出し階層が深くなり、読みづらい印象を受けました。(まだ下のコードはシンプルなのでそこまで感じないかもですが、)

ruby
1  def delete_foo(x, y)
2    (1..5).each do |i|
3        params
4      end
5    end
6  end
7
8  def update_foo(x, y, z)
9    (1..5).each do |i|
10      if z == hoge.public_send("hoge_#{i}_id")
11        params
12      end
13    end
14  end
15
16  def create_foo(x, y, z)
17    (1..5).each do |i|
18      unless z.include?(i)
19        params
20      end
21    end
22  end
23
24  def params
25    {
26        "hoge_id_#{i}": x,
27        "hoge_value_#{i}": y,
28     }
29  end

試したこと

・「リファクタリング　メソッド呼び出し階層深さ」などで調べる

Zuishin

2021/10/02 22:47 編集

読みにくくなったようには見えませんが、なぜそう思ったのでしょうか？逆に params という定義が一か所に収まったおかげで、三か所すべてで間違い探しをせずに済んで読みやすくなりました。 (引数を正しく使い、まともに動くものができたと仮定しての話です) 強いて言うなら、params という名前が問題かもしれません。 hoge_ なんとかをまとめた無意味なデータなので、この例ではまともな命名はできませんが、これがちゃんとしたデータであれば、person や commodity など読みやすい命名ができると思います。

k499778

2021/10/03 01:05

ご返答ありがとうございます！たしかに上記コードですと、メソッド化した方がむしろ読みやすいと思うのもわかる気がします。実際に自分が修正しているコードは、もう少し複雑で、delete_fooなどを呼び出す処理自体も複雑だったりして、呼び出したメソッドから、さらに呼び出されていると直感的にわかりづらい印象を受けました。たしかに客観的に考えるとメソッド化した方が良さそうな気はしたのですが、主観は微妙かなと思い、決め手となる判断基準を調べておりました。

k499778

2021/10/03 01:05

たしかにメソッド名は大事ですよね