🧊

今日から始めるセルフレビュー

2022/04/19に公開

前書き

プログラマの皆様、レビュワーに優しい PR 出せてますか？
私は結構やらかしてます。
やらかしの原因は単純で、ケアレスミスだったり、デッドコードがいたり、無意味な処理があったり。
基本的に PR レビューはチームで機能やソースコードを担保するための工程ですが、上のような単純なミスはできれば避けて、本質的なレビューを進めるのが本来あるべき姿だと思っています。
PR をあるべき姿へする、そのためにセルフレビューのお話をします。

セルフレビューとは

セルフレビューは言葉の通り、自分で自分の書いたコードをレビューすることです。
セルフレビューをすることで、PR レビュワーの負担を減らすことができる（あらかじめ単純なミスを拾える）、PR を出した側も指摘を減らせてお辛い気持ちにならなくなるというものです。
今回自分がおすすめするのは、チェックリストを作ってしまうことです。
利点としては

指摘を溜めることでレビュワーの負担を減らせる&&指摘がたくさん来て辛いお気持ちにならない（どっちも win-win）
もし新規でプロジェクトに入ってくる人がいた時にあらかじめ展開できれば双方指摘が減って辛い思いをしない
自分の主観でないレビュー観点を書き出しておくことで、客観的にセルフレビューができる（おそらく）

というところがあるかなと思っています。
指摘を溜められるところが個人的には結構いいかなと思っていて、自分の書き方よりベターなコードの書き方を知る → レビュー指摘を減らすだけでなく、自身の実装・レビュー力を上げることにもつながるかなと思ってます。

個人的セルフレビューチェックリスト

続いて、個人的なセルフレビューチェックリストを書いていきたいと思います。
※今回のリストには「案件内のコード規約に沿っている」という箇所は一旦取り除いています。
あくまで個人的にこれを気をつけるようにしてるよ、程度の話です。

変数/関数の命名が一目でわかるものになっているか
説明のつかないコードが含まれていないか
不要なステップが含まれていないか
コメントは適切か
関数が大きくなりすぎていないか
同様の処理が既存に存在していないか
美しくない=わかりにくいコードになっていないか

ここからは、各項目ごとに少しだけ説明をしていこうと思います。

変数/関数の命名が一目でわかるものになっているか

非常にシンプルです。命名がしっかりできているかというところです。
変数や関数の命名は、思ったよりも情報を得るソースになります。
途中からプロジェクトへ入った経験があったりするとわかると思うのですが、何をやっているかを追う際に命名がきちんとされていると、何が起きているかはわからんが、とりあえずは何をしたいかなんとなく理解できる、という状態にしてくれます。
逆に命名が適当だと初見で得られる情報がなさすぎて、何をやっているかコードから掴んで、さらに全体の処理を読んで、、、と辛い思いをさせられます。
雑な例を挙げてみます。

const tmpFunction = (value: string): void => {
  alert(`Hello, ${value}`);
};

この関数は「Hello, <名前>」とアラートを表示する関数です。
が、まず命名では何を行う関数かぱっと見でわかりません。また、引数の value も一体何かわかりません。

const greeting = (name: string): void => {
  alert(`Hello, ${name}`);
};

関数名と引数を具体的にしてみました。だいぶわかりやすくなったかなと思います。
今回の雑なサンプルは関数自体が小規模、単純なものだったのでそれほど問題はありませんでした。が、これが大規模になってくるとあとで見返すと何が何だかわからなくなってしまいます。
また、レビュワーも命名がしっかりとしていないコードは可読性が低く、読んでいて辛くなってきてしまいます。
みんなの幸福のためにも、命名はしっかりしましょう。

<余談>命名が長くなりすぎる場合は大体関数などの責務が大きくなりすぎている可能性があります。
命名がしやすい粒度に分割して上げると、関数が綺麗になることがあるので、命名しづらくなったら一度立ち返ってみましょう。

説明のつかないコードが含まれていないか

こちらについては書かれている通りです。多くはコピペしたコードなどで発生しがちなイメージがあります。
大体コピペしたり、訳はわからんがとりあえず書いた、みたいなコードはレビューで指摘が来ます。そして説明ができずに色々調べて読み解いて、、、と無駄な時間を使うことになってしまったりしまいます。
コピペは全くするな、とは言えないですが、自分が理解できないコードはなるべくやめましょう。
しっかりとなぜこうしたか、どうしてこうなったかを自分で説明できるようにしておけば、レビュワーにとってもわかりやすいコードになってくれると思います。

コメントは適切か

コメントがなくても読めるコードであるべき、という話もありますが、個人的には最小限のコメントは良い影響のみ与えてくれると思っています。
コードを変に汚すこともないですし、未経験なメンバーが入ってきたときに、コードを理解する補助になってくれます。
ただコメントは多く書きすぎないことにも注意すべきです。読めばわかることにコメントを入れ続けても、特に意味はありません。
どうしてもコメントが長くなる場合はそもそものコードの書き方が間違っているかもしれないです。
仕様が複雑で関数の処理が複雑になってしまう時などは、コメントとは別にドキュメントを残すのも一つの手段でしょう。JSDoc の @see のようにしてコメントに仕様のリンクを残すのも手だと思います。

不要なステップが含まれていないか

例えば、if を書かずとも実現できるものに if を書いてしまったりしないかがこれに当たります。

const yyy = (array: array<string>): void => {
  if (!array.length) {
    return;
  }

  array.forEach((v, i) => { ... })
}

上記の例だと Array の存在チェックは処理にもよりますが大体の場合不要です。
Array そのものが undefined などにならない限り、forEach の中での処理が行われないだけになりますので、不要な処理を含めるのは避けましょう。

関数が大きくなりすぎていないか

関数が膨れ上がっていることはないでしょうか。私はよくあります。
だいた関数が膨れ上がっているときは責務が 2 つ 3 つになってしまっている時な印象があります。
A も B も C も一気にやって、、、とすると、まとまりが良くなるどころか、かえってわかりにくくなります。
ABC はそれぞれ責務を分けてしまって、それらを 1 つの関数で呼び出すとステップとしてもわかりやすくなりますし、実は他のところで B みたいなことを実現したかった、というときにモジュール化されていると使い回しもききます。
大きくなったらまずは分割できないかを考えてみましょう。

同様の処理が既存に存在していないか

自分が実装していたものが Util や別のファイルですでに書かれていることがしばしばあります。
2 箇所ででくるコードは後々他の箇所でも出てくるようになるというのが個人的な経験にあります。
なんでも共通化しろとは言えないですが、できうる限り、余分な実装コストを増やさないためにも共通でつかえるモジュールにできないかなど考えられるといいかなと思います。

美しくない=わかりにくいコードになっていないか

自分で見返した時に、美しくないなぁと思うコードは大抵の場合レビュアーから見ても良くわかりません。
例えば、if のネストなどがこれに当たるかなと思います。

if (xxx){
  const yyy = ...;
  if (yyy) {
    const zzz = ...;
    if (zzz) {
      return true;
    } else {
      return false;
    }
  } else {
    return false;
  }
} else {
  return false;
}

流石にこんなネストの仕方をするものはほとんど見ないですが、3 階層くらいになってしまうこともあったりしてしまうのかなと思います。
このようにネストしてしまうと、どこからどこまでで何をしたいのかが追いづらくなります。

if (!xxx) {
  return false;
}

const yyy = ...;
if (!yyy) {
  return false;
}

const zzz = ...;
return !!zzz;

こちらが最初に挙げた例を修正したものです。
それぞれがブロック化されていて、読みやすくなったかと思います。
また、視覚的にもごちゃごちゃ感が薄れたかなと思います。
このように、自分で違和感を覚えるコードなどは綺麗にできないかを考えてみるといい感じに修正ができたりします。
完璧というものはないですが、少しでも綺麗なコードを目指しましょう。s

終わりに

レビュワー、そしてひいては自分自身のためにセルフレビューは少しでもいいのでしていきましょう。
レビュワーにも負担をかけず、自分自身もレビューでタコ殴りにされて苦しまないように、最低限、抑えることを抑えてみんなで幸せになれるといいなと思っています。

前書き