コードレビュー観点とレビュー時に気をつけていること

Java php コードレビュー 技術メモ

レビューで気をつけていることをまとめてみた

最近、人の書いたコードをレビューする機会が多くなってきているのですが、そのわりにきちんとした基準とか、観点を考えられてない(なんとなくの感覚でレビューしてしまっている気がする)ので、改めて、レビューの際に見るポイントをまとめてみました。

実態とあった変数の命名となっているか

引数というわけではないのに、 param という変数名が使われていないか、とか。あとは item, valueなど汎用的な変数名が、大きなスコープで使われていないか。

プログラムの処理で扱うものなら、たいていなんらかの「アイテム」なり「値」なので、もう一歩踏み込んで的確な名前がつくと素敵だと思います。

今後の拡張性を考えた命名となっているか

たとえば、「茨城県は××特区なので、○○処理をする」という仕様があり、以下のような処理が定義されているような場合。

if ($this->is_ibaraki($area_cd)) // 茨城県かどうか
{
    // ○○処理
}

この場合、××特区が茨城県以外に広がると、「is_ibaraki」のメソッドの命名まで変えなければならなくなってしまいます。名前を限定しすぎると、あとの改修で困ったりするので、注意して見てみるようにしています。

「あとの改修で困る」かどうかの判断は、ある程度経験があるメンバー、仕様を把握しているメンバーでないとむずかしい場合もあるので、まさにレビュワーの役割かなーと思っています。

むずかしすぎる英単語が使われていないか

「実態にあった命名をする」を徹底しようとするあまり、聞いたこともないような単語を辞書から探し出してくる人がいます。一見して意味が推測できないような単語が使われていると、読みにくいコードになってしまうので、より平易な表現で言い換え可能な場合は、言い換え表現で名前を変更してもらっています。

ただ、「むずかしすぎる」の線引きをどこにするかは微妙なところですね。いまは完全に自分のなかの感覚になってしまっています……

すでに存在する処理を独自実装していないか

既存のクラスの作りがイケてなくて、(新しく実装したくなる気持ち、わっかるー！)と思うこともあるのですが、「独自実装しないと実現できない仕様がある」「既存クラスに深刻な問題が存在する」という場合を除いては、基本的にすでに存在する処理を使ってもらうようにしています。

マジックナンバーが使われてないか

if ($code === '3') // '3'って何を表しているのだろう？？
{
    ...

べた書きの数字や文字列はとりあえず疑って掛かっています。

大抵のプロジェクトでは、定義値などはどこかのクラスにまとめられていますが、実装者がそもそも定数をまとめたクラスの存在を知らない場合もあるので、レビューコメント等で定義場所を教えたりしています。

環境ごと変わる部分がベタになってないか

  • リンクのドメイン部分
  • 接続先ホストとポート

などなど。環境毎の設定を使うはずのところが使われていなかったりするとあとあとトラブルの元になるので、チェックするようにしています。

自然に読めるコードになっているか

if ($not_empty_flg !== false) // 「「空でない」でない」とき？？
{
    ...

仕様上間違っていないとしても、コードとして流れが自然でないときがあります。自然に読める下のような形にしたほうがより良いかなと。

if ($is_empty === true)
{
    ...

リーダブルコードなどにも記述がありますが、「否定形のフラグ」はコードがややこしくなるので避けたほうがよさそうですね。

各言語特有のアレコレ

各言語ならではの、気をつけなければいけないポイントってあると思います。その言語での経験が少ないと、気づかないうちにバグの種を埋め込んでしまうことがある(しかもこういうのって限定的な場面でしか再現しないバグだったりする)ので、言語特有の書き方、みたいなところはできるだけ注意して見るようにしています。たとえば……

PHP

Java

  • 文字列比較(定数を先に書かないとNullPointerExceptionになるとか)
  • マルチスレッド環境下でスレッドセーフでないクラスを使っていないか(SimpleDateFormat, StringBuilderあたりが危険)

ほかにもいろいろありますがここでは割愛します(いつか別エントリで書くかも)。

その他、気をつけていること

ここまでレビューの観点について書いてきましたが、指摘以外でも、レビューのやり取りをする中で気をつけていることがあります。むしろ、ここ以降の項目が一番気を遣っているところかもしれないです。

ほめるポイントを探す

指摘されるばかりだとレビュイーもテンションがさがるので、「いいな」と思ったところがあったら積極的に褒めるようにしています。

たとえば「ここが影響範囲だってよく気づいたね！」とか、あと単純に「この機能よく作りきったね」とかでも良いと思います。たいていレビュワー＞レビュイーという形で上下関係があるので、「レビュワーがレビュイーを責め立てる」みたいな構図にならないように、レビュワーは留意する必要があるかなと。

こちらが伝えていなかったことが原因で、指摘が出てしまったときは一言謝る

「後だしジャンケン」的なタイミングで指摘を受けると、人間、多少なりとも腹が立つと思うんですよね。私自身、レビュイーとして以下のようなやりとりをしたことが何度かあります。

　レビュワー「○○の場合は××をする仕様ですので、この部分は修正をお願いします」 　レビュイー（その仕様、私、いま初めて聞いたんだけど？？！どうして教えてくれなかったの？？？！！！）

こういう部分で無駄な軋轢を生むことは避けたいものです。。私は指摘を書くときに「伝えきれていなくて申し訳ないのですが」とか軽くワンクッションの謝罪の言葉を入れるようにしています。

細かい書きっぷりの指摘は極力lintなどで自動化する

  • 改行コード文字コードが違う
  • インデントの幅が違う
  • タブ文字にするべきところがスペース4個になっている

などなど。細かい書きっぷりの指摘は、レビュワーがひとつひとつ指摘しようとすると、見つけるのもコメントを書くのもかなりの手間がかかってしまいます。できるだけ自動化したほうがレビュワーレビュイー両方にとってハッピーかなと思います。

ぱっと思いつくのは以上でしょうか。また思い出したり増えたりしたら追記していきたいと思います。