レビューで指摘されがちな点(命名のやり方、適切なコメント、マジックナンバー、メソッドへの切り出し)

概要

同じような点をレビューで指摘されないようにするために、自分がされたレビューをメタ的に振り返ってみようと思います。自分がレビューで指摘されがちな点を大きく以下のように分けられそうです。

単純にBugがある(間違っている)
間違ってないが、改善の余地がある
書き方
1. 命名が適切でない
2. コメントの適切性
3. マジックナンバー
4. 適切な単位でメソッドへの切り出しが行われていない
パフォーマンス
書くべきところに書いていない(RailsのMVC, Serviceクラスの使い分け etc.)

今回は「書き方」という観点で、自分のしがちなミス、それを避けるための考え方について考えてみます。

「書き方」が違うとは？

概要にも書きましたが、書き方が違うには色々なタイプの間違いがあります。

命名が適切でない

メソッド名、変数名、テーブル名などがミスリーディング、実態と異なっている

コメントの適切性

必要な箇所にされていない or コードを読めば十分なことが説明されている

それぞれのケースについて、

なぜそれがよくないのか？
どうすれば回避できるか？

を考えていきます。

命名が適切でない

変数名が実態と異なっている

以下のようなケースでは、user_id の配列を取得しているので、user_idsとすべきです。

users = User.where(type: hoge).pluck(:id)

このような書き方をしていると、usersにUserモデルのArrayが入っていることを期待して、以下のようなコードを書いてしまう可能性があります。

users.pluck(:name)

サンプルだけ書いていると、するはずがないようなミスに思えますが、コードが複雑になり、他のメソッドの引数としてusersが渡され…というようなことをしているうちに、すっかりusersの実態を忘れ、ミスを引き起こしてしまいます。

メソッド名が過剰

以下のようなケースでは、引数としてnameが渡されており、Userをnameによって絞り込んでいるのは自明なので、get_users_by(name)とすべきです。

def get_users_by_name(names)
  User.where(name: names)
end

以上のように書けば、絞り込みたい引数が増えた時でも、

def get_users_by(name, age, area)
  ...
end

といった形で拡張することが容易になります。

命名まとめ

以上の例をまとめると、「変数名は実態を必要十分に表していること」が重要だとわかります。

Rubyでは、明示的なデータ型の宣言がないので、変数名が唯一の手がかりになる

とう指摘が非常に納得できる理由でした。

コメントの適切性

必要なケースにコメントされていない

過度に複雑なロジックであったり、一時的な知識に基づくものには、適切なコメントをしてあげる必要があります。何をしているか？というロジックの部分は読めばわかることが多いので、なんのためにしているか？などを説明してあげると良さそうです。

# NOTE:
# TODO(ykamez):

過剰にコメントされている

以下のようなケースであれば、メソッド名で十分説明しているので、コメントは不要です。

# Get registered users only
def get_registered_users
  ...
end

『プログラマが知るべき97のこと』で、以下のように書かれている通り、理想はメソッド名や、変数名でやりたいこと、やっていることを表現してある状態が理想なので、必要なケースと見極めをつけて、コメントをしていく必要があります。

コードに書けないことのみをコメントにする

不適切なコメントが残っていれば、誰かがコードを見る度に集中力が削がれたり、誤った情報を与えてしまうことさえあります。

どうしてもコードに語らせることが不可能な時に、語らせたかったものとコードとのギャップこそコメントに書くのです。

コードに書けないことのみをコメントにする | プログラマが知るべき97のこと

コメントまとめ

コードに書けないことのみをコメントにする

コードに書くことが不可能な時のみ、コメントをする。

マジックナンバー

以下のようなコードをみたとき、10が何を表すのか知るのは難しいと思います。書いた本人は、表示上の問題として、10件しか必要でないと判断して、10と書いたとしても、後からみた人、そして将来の自分も何を意図して、10としたのかわからなくなる、ということが発生すると思います。

def get_hoge_users
  User.where(hoge).limit(10)
end

そのような時は、 displayable_user_size, displayable_user_limit など変数名や、定数名でその数字が何を意味しているのかを表現してあげると良いです。

メソッドへの切り出し

先ほど紹介したコードに書けないことのみをコメントにする | プログラマが知るべき97のことにも以下のようにありました。

関数が長くて分かりにくいせいでコメントが要るのなら、関数を小さく分割して、どういう関数かがすぐに分かる名前を個々につける方がいいでしょう

例えば、以下のようなコードを　みたとき、一見してなんの処理を行うかわかりづらくなると思います。

def get_hoge_users
  (処理1)
  if ...
    (複数行にわたる処理2)
  end
  if ...
    (複数行にわたる処理3)
  end
end

処理1, 2, 3とやりたいことが別になってるのであれば、それぞれをメソッドに切り出すことで、コードの見通しをよくすることができます。

def get_hoge_users
  処理1method
  処理2method if ...
  処理3metohd if ...
end

ykamezの技術ブログ

日々学んだことをアウトプットしていきます。Ruby/Rails/グロース/分析/施策提案