👀

最低限のコードレビュー観点

2021/09/11に公開

1件

review

コードレビュー

tech

概要

仕事でコードレビューするとき、以下のようなフローで進めています。

実装者がローカルで開発する。
GitHub に、コードを push する。
Pull Request (PR) を作成する。
レビューする人にアサインする。
レビューして、コメントを付ける。
実装者がコメントを返信したり、コードを変更して push する。
4.に戻る
レビューでコメントが無くなると、マージして作業が終了する。

実装者に、レビュー依頼前に自分自身でチェックしてもらい、コードレビューの精度を上げていきたいと思って、基本的な「コードレビュー観点」を書きました。

もっと詳しく・細かい部分などは、オライリーのリーダブルコードを読んでもらうのが良いと思います。

以下、観点別に、意図やコード例を記載していきます。
（主に、JavaScriptで説明していますが、観点としては普遍的なものだと考えています。）

Write PR description

PRの「概要」欄に、そのPRで変更した理由と大まかな内容を記載する。
変更のきっかけとなった、issueやチケットのリンクなどは最低限必要。

説明

「どんな変更なのか」という、概要を一切知らない状態でコードを読み進めていくのは辛い作業です。
概要欄が書かれていることで、コード差分が想像でき、レビューの難易度はかなり下がります。

TODO comment

このPRの中で、完了しない部分があるのなら、TODOコメントとして残しておく。

説明

1つのPRで、1つの"タスク"が完了することが理想だと思います。

とはいえ、開発中には様々な理由で、「途中だけどレビューしてほしい」という状況が発生するかと思います。
その際に、レビューする側としては「何が終わっていて、何が終わっていないのか」を理解しながら読みすすめるのは難しく、コード中のTODOコメントがそれを助けます。

また、エディタやIDEには、フォーマットに従った"TODOコメント"をリスト化してくれる機能もあり、後から残課題を整理するのにも有効です。

good

function foo() {
  // TODO: foo API is not implemented. So I return dummy data.
  return {};
}

Delete comment out

デバッグ用に一時的に書いたコードや、削除予定のコードについては、特別な理由が無い限りは物理削除する。

説明

我々は、GitHub上で履歴管理された状態で開発を進めています。
PRの検索や git blame によって、変更は履歴として管理されているため、不要になったコードは削除しておくべきです。
無駄なコードがコメントとして多く残っていると、コードを検索するときのノイズとなってしまい、読みづらいコードになってしまいます。

Early return

return文をうまく利用する。

説明

コードを読む場合、基本的には上から下に読んでいきます。
とはいえ、if文があった場合には「else-ifが続いているか、それらが終わった後には何をしているか」を知ってから中身を読んでいくこともあります。
その際に、「実は、関数の中身がif文だけだった」となれば、読む側の時間のムダが大きいです。

return文で関数から抜けることが明記されていれば、読みやすくなります。

bad

function some_long_method() {
  if (foo) {                // 1. find if block. search end of the block.
    // some long process    // 4. read inside the block.
    if (bar) {
      return a;
    } else if (baz) {
      return b;
    }
  }                         // 2. find end block. read next line.
  return null;              // 3. find "return". So I know "if !foo, return null only". So scroll up to read inside the block.
}

good

function some_long_method() {
  if (!foo) {           // 1. find if block. Also find next "return" line.
    return null;
  }
  // some long process  // 2. read the process.
  if (bar) {
    return a;
  }
  if (baz) {
    return b;
  }
  return null;
}

boolean variable naming

boolean型の変数名は isXXX のように命名し、{class name} is XXX のような英文として読める形とする

説明

例えば、変数名が isUplaod だと、「be動詞 + 動詞」なので英文として意味がわかりません。
isUplaoding であれば、「Uploading（= アップロード中）である場合にtrue」のように英語として読むことができます。

とはいえ、boolean型であれば、必ず isXXX の形式であるべきというわけではありません。
canUpload や hasChildren なども英語として読めつつ、「どんなときに true が入っているか」が読み取れるので、良い命名です。

Commonization

税抜き・税込み金額の計算や、日付の特殊なフォーマットなど、システムの別の画面でも使いそうなものは、共通で利用できるように切り出しておく。

説明

税抜き金額の計算などは、処理ごと・画面ごとに変わることは無く、システムを通して同じ計算を行うべきです。
そういった計算処理が、コードの至るところに点在していると、計算が間違っていたとき・要件が変更になったときに見つけ出して修正するのが困難です。
そのため、同じ処理は1つの関数に切り出して共通化しておくべきです。

ただし注意点があります。
逆に、現時点で"偶然"一致している処理については、共通化しない方がよいです。
例えば、ゲームのスコア計算で1.1倍しているものと、消費税率10%の税込価格の計算は、同じ1.1倍でも共通化すべきではありません。

Reduce "state"

"状態"は可能な限り排除する。
他の"状態"から計算可能な値は、計算によって算出する。

説明

クラスには、 let foo; とするだけで、"状態"をもたせることができます。
ただし、"状態"として持ってしまうと、様々なことを考えなければなりません。

いつ初期化されるのか。
いつ更新されるのか。
この関数が呼び出されたとき、どんな値になっているのか。
別の"状態"が特定の値のとき、どんな値を取りうるのか。

これら管理する"状態"が増えれば増えるほど、コードを読みづらく、不具合が発生しやすくなります。
他の"状態"から計算可能な値は、計算によって算出しましょう。

bad

let hasError = false;
let errorMessage = null;

function setError(newMessage) {
  hasError = true;
  errorMessage = newMessage;
}

function printError() {
  if (!hasError) {
    return;
  }
  console.log('message is ' + errorMessage);
}

good

let errorMessage = null;
function hasError() {
  return errorMessage != null;
}

function setError(newMessage) {
  errorMessage = newMessage;
}

function printError() {
  if (!hasError()) {
    return;
  }
  console.log('message is ' + errorMessage);
}

Manage const values (even if the values havn't been determined)

実装時に未確定の情報であっても、意味に応じて定数化してまとめておく。

説明

外部のURLなど、実装時に値が未確定のこともあります。
そんなときでも、定数として定義しておき、確定されたときに一括で利用できるようにしておく。

bad

function method1() {
  location.href = ""; // TODO: Change value if tems URL is fixed.
}
function method2() {
  location.href = ""; // TODO: Change value if tems URL is fixed.
}

good

// TODO: Change value if tems URL is fixed.
const TERMS_URL = "";

function method1() {
  location.href = TERMS_URL;
}
function method2() {
  location.href = TERMS_URL;
}

可能な限り可視性を小さくする

不必要なものは、外部から見れないようにしておく。

説明

不要なものまで export されていると、「それがどこで使われているのか」「中身を変更した場合、影響範囲はどこなのか」を調べる手間が発生します。
そもそも、ファイル内でしか利用していないものであれば、「将来使うかもしれない」ものであっても、その時点では export しないようにしましょう。

同様に、変数の宣言についても、「そのブロック内でしか利用しないものは、ブロック内で宣言する」のように、参照できる行数はなるべく少なくしましょう。

Read your changes again

差分を読み直しましょう。

説明

PRのレビュー依頼を行う前に、もう一度差分を読んでみましょう。
GitHub をブラウザで見ると、 "Files changed" というタブがあり、発生している差分が見やすく表示されています。

特に、以下の観点で確認してください。

自分が意図していない変更が含まれていないか？（誤ってキーボードに触れてしまっている部分や、PRに関係無い差分などが無いか）
自分が意図している変更が、全て含まれているか？コミット漏れが無いか？
PRの概要に書いたものと、差分が一致しているか？

最後に

細かい部分の指摘や、単純なミスに対しての指摘は、コメント書く側も受け取る側も疲れます。
実装時・レビュー前に気をつけることで、コードレビューを有意義なものにしましょう。

追記

英語でも書いてみました。

Discussion

kanon_code

とても参考になりました！
有益な記事をありがとうございます！