2019/12/22

より良いコードレビューをするために気をつけていること

Author:: oinume

, 2019/12/22

Merpay Advent Calendar 2019 の22日目は、メルペイスマート払いチーム/Backend Engineer の @oinume がお送りします。今日はコードレビューについて自分が普段から実践していることを書いてみたいと思います。

はじめに

世の中にはコードレビューをする時の観点については数多く共有されていますが、より良いコードレビューをするためにはどうするのが良いか、というHOWについてのノウハウはあまりシェアされていないような気がしています。そのため、今日は自分なりに心がけているコードレビューのやり方と、ついでに気をつけている観点について書きたいと思います。

Slackを閉じる

(これが本当に一番大事だと思っているので最初に持ってきたのですが)私は極端に集中力がないため、SlackのDesktop通知が来るとついついそれが気になって見てしまいます。コードレビューの時は集中することが大事なので、Slackを閉じて集中できる環境を作っています。

WHATやWHYをきちんと書いてもらう

typoを直すような単純なものであれば話は別ですが、きちんとしたコードレビューをするためには

その変更で何が変わるのか
なぜ変える必要があるのか

を理解する必要があります。ですので、Pull requestの説明には変更内容となぜその変更をするのかを明記してもらっています。例えば「バグ修正」という説明だけでは、どういうバグがあったのか、そのバグがどういう影響を与えているのかはわかりません。コードレビューの優先度・重要度を判断するためにもこのような情報は大事だと思っています。

コードを自分のPCにpullしてIDEで見る

より効率的かつ質の高いレビューを行うために、自分のPCにコードをpullしてIDEでコードを見ています。理由としては…

コードジャンプが使える
IDEやエディタの機能でwarningを見つけられる
IDEまたはターミナルから実行できる

になります。詳しくは後述しますが、大きなPull requestの場合は全体像を把握するためにも特にコードジャンプは有用だと思っています。

f:id:oinume:20191218211759p:plain — IntelliJでのwarningの例

CIでlintをかける

「インデントはスペースではなくタブにして下さい」というような不毛なやり取りをなくすために、gofmtやlintを有効活用すると、より本質的なコードレビューに集中できると思っています。Goであればgofmtをかけるのは当然ですが、私の担当プロジェクトではgolangci-lintを使って主に以下のlintをかけています。

goimports
govet
errcheck
staticcheck
unused

また、Spannerを使っているのでReadOnlyTransactionのClose忘れなどを警告してくれるzaganeも最近導入されました。Goでは静的解析ツールが充実しているため、コンピューターにできることはどんどん任せていくのがいいと思います。

全体像を把握する

A->B->C->D という関数呼び出しの構造になっていて、Dのコードの深いところが修正されている場合は、その修正がどういう影響を及ぼすのか、呼び出し元が複数あった場合に本当にその修正で全て問題ないのかを確認する必要があります。そのため、私は以下の順番でコードを読み進めて行くことが多いです。

Dの部分で具体的に修正された内容を把握する
修正された箇所が呼び出されるAPIの入り口を調べる(Aの部分)
入り口からコードを読みつつ徐々に深く潜っていき、修正された箇所をあらためて見る
ユニットテストなどのコードを見て不足しているポイントがないかを見る
Dに関わる部分で他に影響がないかを見る

大きな修正の全体像を把握する

また、大きなPull requestの場合は全体像を把握することがより困難です。修正内容にもよりますが、修正範囲が大きい場合は、修正前のソースコードで関連する部分を、APIの入り口から丸ごと読んでざっと把握することが大事だと思っています。その上で修正された内容がどういうことなのかを把握し、修正漏れはないかなどの調査を行います。

panicを入れてテストを全て実行する

この関数は入り口も含めてどこから呼ばれているのか？ということをソースコードだけで調べることはなかなか難しいです。そのため、修正箇所にpanicを仕込んで全てのテストを実行し、失敗するテストを眺めて「なるほどこの修正はこんなところにも影響があるのか」ということを調べたりします。

実行時の状態把握のためにデバッガを使う

修正箇所が実行される時に、使われている変数が多いとそれらにどういう値が入っているかを知ることが難しいです。そういう場合はデバッガを使いましょう。デバッガを使うことで具体的な変数の中身を知ることができるため、コードを読む時の助けになります。もちろん変数名がわかりやすいに越したことはありませんが…

大きすぎるPull requestは分割してもらう

当たり前ですが大きすぎるPull requestはコードレビューが大変です。大きな機能でかつそれが既存のコードに影響を与える場合は、環境変数などを使ってfeature toggleできるようにして、分割してPRを出すようにお願いしています。これはコードレビューが楽になるだけではなく、他のPull requestとコンフリクトしにくくなるというメリットもあります。

過去の経緯を調べる

コードレビューをしている時に「なぜもともとこういう実装になっていたのか？」という疑問が生じることがあります。そのような場合は以下の流れで経緯を調べています。ただし、過去のPull requestにちゃんとその修正の目的(WHAT)や理由(WHY)が書かれていることが前提となるので、先ほど紹介したようなPull requestの説明をきちんと書くということが大事です。

IntelliJのOpen on GitHubの機能を使って、IDEのエディタからGitHubを開く
GitHubでBlameして修正されたコミットやPull requestを調べる
Pull requestにあるWHATやWHYを確認する

わからなければ質問する

どんなにコードや仕様を読んでもわからないことはあるのは当たり前なので、私は割と気軽に質問をしています。そういう意味では気軽に質問できる雰囲気を普段から作っておくことはとても大事だと思っています。いわゆる心理的安全性というものですね。

また、自分がPull requestを出す時に「ここは意図がわかりづらいかな」と思う時は、コードの中にコメントを書いたり、Pull requestに自分で補足コメントを入れたりします。

レビュー時に特に気をつけていること

コードレビューの観点についても併せて書いておきます。長くなるので重要な2つだけピックアップしています。

APIが冪等になっているか？

メルペイではmicroserviceのAPIを作る時に冪等になっているかどうかを重視しています。意外とこれを忘れて実装していたり、冪等にしたつもりが実はなっていなかったというようなケースがあるので、APIが冪等になっているか、APIを2回実行して冪等になっているかをテストするコードがあるかをコードレビューで見ています。
また、既存のAPIに処理を追加することで、もともと冪等であったものが冪等でなくなってしまうということもあるので、冪等になっているかのテストを書いておくことが大事です。

影響範囲の調査ができているか

structに何かフィールドが追加された
statusのようなenumに新しく項目が追加された

上記のような場合は、修正漏れがないかをレビューワーとしてチェックしています。もともと「影響範囲の調査を行うのはコードを書いた人の義務」だと思っていてこのようなことはやっていませんでしたが、意外とこれが漏れているケースが多いためです。また、これで問題が指摘できるとレビューされる側からも感謝されることが多いです。

最後に

自分が普段行っているコードレビューについてつらつらと書いてみました。「自分はこんなやり方をしているぞ」などがあれば、ぜひTwitterなどでシェアしてもらえると嬉しいです。

明日のMerpay Advent Calendar 執筆担当は、 Backend Engineer の @kokukuma さんです。引き続きお楽しみください。