2014年8月4日
眼鏡なしのコードレビュー
本記事は、原著者の許諾のもとに翻訳・掲載しております。
例えば、あなたが驚くほど聡明な開発チームのメンバーで、コードレビューのみに一日の時間を確保しているとします。しかし作業を開始して2時間後、眼鏡を忘れてきてしまい、午前中はぼんやりとしたカラフルな表示を見つめていただけだったということに気づいたとします。さて、あなたはどうしますか?
家まで歩いて10分もかからないし、天気も良ければ、眼鏡を取りに帰るのが一番です。でも朝家を出るとき、攻撃的なスズメバチの群れが眼鏡の置いてある部屋に巣を作って、邪魔されたくない様子だったらどうしますか？
そういう時はもちろん、コンタクトレンズを付けてきたふりをして、恥ずかしい思いをしないようにするのがよいでしょう。実際に読むことなく膨大な量のファイルを見分けることができるということを覚えておいて下さい。
参考コード 1
不安の種は隔離するべきだということに誰も異論はないでしょう。そしてもちろん、あらゆるクラスは一つのことを行うためだけに責任を持つべきです。しかし、あなたがここで作成したこの UserCreator オブジェクトは、たぶん少しやり過ぎです。 UserCreator がやらなければならないことがこれだけであれば、さしあたって Users が自分自身を作成することができます。さもないと、かつてシンプルだった User.new が、いたずらに地獄のような悪夢に変わります。あなたが何かを変更したり、fooがどういう挙動をしているのか理解したい時はいつでも、たくさんの小さなファイルの中を地球の裏側までgrepしなければならなくなるのです。
参考コード 2
クラスに偽装したこのかなり大きなメソッドを見ると、技術的に言うとDRYの典型で、言葉の文字通りの意味でこれ以上分解できることはなにもないことがわかります。しかし、どうやらあなたはユニットテストをする人ではないことぐらいはわかります。そして、もしあなたが濃いコーヒーを出してくれたなら、私は中央の20行のコード ブロックがメールを送る必要のあるユーザーを決定するものだと見破ることができたのですが、「 def users_to_send_emails_to の中をよく見た方がいいですよ」とそっとあなたに勧めておきます。私がそうしなくてすみますから。
参考コード 3
オーケー。このクラスのメソッドはずっと短くなっています。たぶんこれは進歩でしょう。しかし、過ぎたるは及ばざるがごとしです。行が変わるごとにあなたがメソッドの間をぴょんぴょん跳び回るのを、Rubyインタープリターは気にしませんが、ほとんどの人間インタープリターは気にします。ファイルをちょっとスクロールさせたりしている間は私も他の人と同じように満足しています。でも、どこから跳んで来たのか忘れないように自分の腕にスタック トレースを書き込まなければならなくなると、おそらくその時にはいくつかのメソッドをくっつけてごちゃまぜにしてしまうでしょう。
参考コード 4
あなたがこのクラスが正しく厳密な名前空間を使っているようにしようと心がけているのはわかります。いいでしょう。名前空間を使うのはいいです。しかし、第6レベルに到達するということは、あなたが小さなスペースにあまりにも多くのことを詰め込もうとしているからかも知れません。名前空間を細かく分割するのをやめるか (たしかにその2つのヘルパークラスは自分の名前空間を持てたと思いますが、次の1階層上のなにかとぶつかったりしているでしょうか?)、コードを分離分割してまったく別のベース名前空間に全て入れてしまうことを考えてみましょう。
参考コード 5
明確化のためのコメントには賛成! 理解するのに何章にもなる作文が必要なコードには反対!
参考コード 6
2つ目のメソッドに目を凝らしてみてください。もしもあるメソッドが自分の仕事とそのやり方を知るために8つの引数を必要とするなら、そのメソッドはかなりの過労状態です。春のようなリファクタリングで、負荷を分散して、その肩にかかる重荷のいくぶんかでも取り去ってやりましょう。メソッドを2つ (か、それ以上) に分けるか、この引数のいくつかをイニシャライザー内のインスタンスに持って行くのが理にかなっているかも知れません。そもそもあなたは8つの引数を同時に扱えますか?それならあなたのメソッドにも同じ期待はしないでください。
さてこれが、眼鏡を忘れてしまったか、医学的に望ましくないくらい長く太陽を直接見つめてしまったか、そういう時のためのコード レビューのやり方です。もしも私がもっと優れたプログラマーなら、ずっとはるかに鋭い例を思い付くことがきっとできたでしょう。一方で、取るに足らないようなことが時に興味深くなることもあるし、ほとんどの場合、私たちが考えるよりも重要だともいえます。あなたのデザインがどんなに明確で、シンプルで、よくパターン化されていても、セメントや釘を使わなかったらそれは無駄に終わります。
