トレタのコードレビューへの取組

始めに

皆さま、こんにちは！
トレタのサーバサイドエンジニア兼佐久間まゆちゃんのプロデューサーの@hiroki_tanakaです。
先日、コードレビューに関する議論が社内で巻き起こり、その時話に上がった内容や実際にトレタで行っているコードレビューの取組に関してご紹介します！

コードレビューの目的

そもそも何故、コードレビューが必要なのでしょうか。
前提としてシステム開発は何もしないと下の”あるある”現象に苛まれます。(身に覚えのある人が多いのではないでしょうか。)

• コピペの横行
• 動けば良しのコードの乱立
• //TODO:後で直す と書かれているコードの散乱
• 単一責任の原則を無視した巨大クラス・メソッドの発生

ソースコードは継続的にメンテナンスをしないと品質は低下し続けます。
コード品質が低下している状態では、訳の分からないコードや処理を追うのが困難なコードが壁となってちょっとした機能追加や修正がとても困難になります。
そうなると、開発速度の鈍化と既存機能での保守コスト増大でビジネスとして大きな痛手となります。
そのため、コードレビューの目的はチーム全員でシステムのコードの品質を上げることです。大切なのはチーム内の誰か一人の力で行うのではなく、チーム全員で立ち向かおうということです。
(「そんなの当たり前じゃん」と思う人が多いかも知れませんが、チームとしてコードに当たっていくというは意外と盲点なように思います。)

コードレビューでやってはいけないこと

コードレビューでやっていけないことはそれほど多くはありません。ですが、これらはやってしまうと個人間の仲間割れからチームに亀裂が入るかも知れません。

1. 個人攻撃や人格否定を行う。
2. 強い言葉を使う。
3. 感情(特に怒り)でレビューする。
4. 主観による粗探しをする。

まず1〜3に関して、大前提ですがコードレビューは個人攻撃の場ではありません。
上述した通り、コードレビューの目的はチーム全員でシステムのコードの品質を上げることです。
そのため、強い言葉で個人攻撃のような指摘をするのは意味がありません(例：「こんなクソコードを書くなんてあり得ない」「(キレ気味に)なんでこんなことも知らないの」など)。
強い言葉を使った議論やコードからの個人攻撃をしても良いシステムは出来ません。コードの品質も向上しません。
それどころか人格攻撃された結果、モチベーション低下で書いたコードが将来の負債になる可能性やレビュアーとレビュイーの関係悪化でプロジェクトが進まなくなる可能性を考えると、百害あって一利無しです。
(そして、レビュアーはパワハラで訴えられる可能性もあります。)

4の「主観による粗探しをする」ですが、良くないコードを改善するのが目的なので指摘は必要ですが、指摘に主観を入れてはいけません。

NGな例
「XXXの処理が読みにくく、良くありません。」

「RSpecが…」

どうでしょう、これらレビューは「読みにくい」や「書き方が良くない」は完全に主観なため、具体的にコードをどのように修正すればよいか分かりません。
レビューは客観的根拠に基づいて行う必要があります。
客観的根拠とはチーム内のコーディングルールや過去のエンジニアリング哲学(『リーダブルコード』に載っているようなこと)を指します。

リーダブルコード ―より良いコードを書くためのシンプルで実践的なテクニック (Theory in practice)

それを踏まえて、先程のNGな例を修正してみます。

OKな例
「XXXな処理がネストが深くなってしまっています。また、同じような処理のYYYとZZZがあり、DRYじゃないコードになっています><
XXXの処理は下記のURLを参考にするとネストが浅くなって、メンテナンスしやすくなると思います！YYYとZZZは〇〇Utilクラスに切り出し、共通化することを検討してみてくださいm(_ _)m  参考URL：http://~~」

「CIでRSpecが失敗しています。修正後に再度、レビュー依頼をお願いします。」

このように記載すれば具体的にコードの修正方法やレビュアーが何を根拠に指摘しているのがわかりやすいため、レビュイー側も納得感を持って、修正に入れます。
このようにコードレビュー時にやってはいけないことは4点と多くなく、どれも当たり前のことですが、どれも大切でしっかり意識していく必要があると思います。
(詰まるところ、自分がされて嫌なことを相手にしないという相手を思いやる気持ちです。システムの仕事をしているのに、一番大切なのは人なんですよね。)

コードレビュー時に心掛けること

レビュイー編

まず、レビュイーがコードレビュー時に最も気をつけることは、下記を明記することです。

• PullRequestにソースコードの変更の理由・何故必要なのか。(Why)
• 具体的にソースコードの何を変更したのか。(What)
  (Viewの改修ならば、Before/Afterで画面がどのように変化したのか。)

当たり前ですが、発行したPullRequestの背景・仕様・実装に一番詳しいのはレビュイーです。
そのため、レビュイーはPullRequestを数年後に自分を含む誰かに読まれても、正しく意図が伝わるようにすべての情報が含まれているように書くのが理想です。
タイトルの付け方も読んだだけで、PullRequestでどのような変更をしているのかパッとわかると良いですね。

また、レビュイーは「レビュアーに直してもらおう」や「良くない点はレビュアーが気づいてくれるだろう」という考えは捨てた方が良いです。
レビュイーが明らかな仕様ミスや設計ミスをPullRequest発行段階で発生させてしまうとすれば、タスクの進め方が間違っているか有識者との相談不足が原因かと思います。 そのため、レビュイーは常に「考え抜いて不足はない、テストも完璧だ。」という状態で初めてPullRequestを作成するのがベストだと思っています。
(私は「レビューで上げられる点数のMAXは20点であって、PullRequest時点で80点ないと100点にならない。」と昔、先輩社員に教わりました。)

レビュアー編

1. 読んでみてよくわからないと思った部分を質問してみる・読んで感じたことを率直に聞いてみる

1つ目は、『読んでみてよくわからないと思った部分を質問してみる・読んで感じたことを率直に聞いてみる』です。
まず、この観点のレビューなら熟練者だけでなく、初心者でも出来ます。初心者でも読んでみてよくわからないと思った箇所は、大体怪しい挙動をするかバグが潜んでいます。
(個人的には、初心者が読んでわからないということはシンプルなコードになっていないということだと思っています。)
また、質問的に聞くことによって、バグを聞かれた側が発見することが多いです。
そしてプラスアルファ、初心者はPullRequestのレビュアーを通じて、コードを読むようになりコーディング知識やFWへの理解も深まります。良いこと尽くしですね。

2. こんなこと言ったら細かい・神経質って思われるかな…という恐怖を捨てる

2つ目は『こんなこと言ったら細かい・神経質って思われるかな…という恐怖を捨てる』ことです。
良くないコードのスタイルを放置しておくと他の人がそのコードを見た際に「このコードでも許されるなら、多少雑なコードを書いても大丈夫だろう」という気持ちになり、どんどん良くないコードが量産されていきます。(『割れ窓理論』：「建物の窓が壊れているのを放置すると、誰も注意を払っていないという象徴になり、やがて他の窓もまもなく全て壊される」という考え方。)
そのため、コードスタイルの改善は細かいことでも出来るだけ気づいた際に指摘してあげるのが良いです。もし細かいことを言いたいけど角が立ちそうと[NITS] (NITPICK)や[IMO] (In my opinion)と付けてあげればよいかと思います。

3. テストで行ったことの確認

3つ目は『テストで行ったことの確認』です。
そもそもテストコード(Viewの修正だけならば画面打鍵)の結果のエビデンスがPullRequestになければ、「テスト書いて」と言いましょう。
よく緊急度の高い改修は開発者のローカル環境での打鍵確認のみでテストコードは後追いで書くとなりがちですが、本当に後追いでテストが書かれたケースは稀です。その後一生書かれないです。
その上でテストを行った形跡があれば、私は以下のことを確認します。

• 正常系は仕様を満たしているか。
• 境界値チェックが行われているか。
• 異常系のテストが行われ、エラーが出ることを確認しているか。

特に異常系のテストは忘れられがちなのですが、大体これが抜けてて予期せぬ不具合が発生します。正常系と同等に大事なテストケースだと思っています。

4. コードに問題がないかの確認

4つ目はコードレビューで最もポピュラーな『コードに問題がないかの確認』です。
箇条書きになりますが、下記のような観点のことを指します。

• PullRequestの修正によって、意図せず他のコードに影響を出していないか。
• Classやメソッドは肥大化していないか。
• 単一責任の原則に則った単位で切り出しが行われているか。
• 各Classは期待される役割通りのコードしか存在していないか。(本来、Modelが持つべきロジックがControllerに書かれていないか等)
• 変数のスコープは適切で、グローバル変数やクラス変数がないか。
• if文やfor文でのネストが異常に深くなっていないか。
• 変数名やメソッド名は意味がわかりやすい名前になっているか。
• コメントやrdoc・ydocが正しく記載されているか、記載漏れはないか。
• エラーハンドリングは行われているか、無闇に握りつぶされていないか。
• ログは適切な位置で出力されているか。

コードの問題への観点はやはり経験によって左右されやすいかと思いますが、初学者でも『リーダブルコード』や『プリンシプルオブプログラミング』に載っているDRYやSOLID原則・YAGNIなどこれまでの先人たちの知恵を借りることである程度、指摘できるかと思います。

5. 積極的にLGTMしよう

5つ目は、『積極的にLGTMしよう』です。
良いPullRequestやコードレビュー指摘にしっかり対応したPullRequestにはLGTM!で褒めましょう！
私はLGTMoonというサイトで良く画像を取得しています。(やっぱり、LGTM貰えると嬉しいですよね！)

トレタで実際に行っているコードレビューのやり方

それでは実際にトレタで行っているコードレビューのやり方についてです。
現在、社内で一番大きいAPIリポジトリに対して、サーバサイドチームの人がコードレビューをする時は以下のようなルールに則って行っています。

• エンジニア8人で2人ペアを4チーム作る。
• Aチームの人はBチームの2人にPullRequestのレビュワーを依頼する。
• Bチームの人はCチームの2人にPullRequestのレビュワーを依頼する。
• Cチームの人はDチームの2人にPullRequestのレビュワーを依頼する。
• Dチームの人はAチームの2人にPullRequestのレビュワーを依頼する。

二人組みの組み合わせは「レビューに慣れているベテランやエースと新規参加メンバやレビューに不慣れな人」として、各チームのスキル・知識差やレビュー密度の偏りを防ぎ、均一化することを狙っています。

この相互レビュー制が導入される前は全てのPullRequestにベテランやエースエンジニアがレビュアーとしてアサインされるという方法が取られていましたが、開発チームの人数が増えるにつれて、レビュアーの負担が増加していったため限界を迎えました。
スタートアップの初期段階ではエースが全てのコードを確認するといった状態でも良いかと思います。
短いサイクルでシステムをリリースすることの優先度が高いことやシステム規模が小さいのでレビュー負荷がそこまで大きくないためです。
しかし、ある程度成熟したベンチャーや中〜大規模プロジェクトではこのような相互レビュー制が良いかと思います。(というか、そうしないと破綻する恐れがあります。)

この相互レビュー制となって思うことはアプリケーションやFW全体への理解の深まりとコードリーディングの重要性の再認識です。
私は普段、サブシステムの一部を担当しているのですが、担当領域外のAPIやFWのPullRequestを読むことによって、アプリケーション全体への知識のストックが増えます。
この知識のストックがじわじわと効いてきて、自身の担当領域でも「FWが提供している機能を使えば簡単にできるようになる」や「このロジックはアプリケーション全体で共通化した方が良いと思うけど、どこに書けば良いんだろう？」と考えられるようになりました。
また、コードリーディングの回数が増えたことによって、疑問に思う→ググる→理解するor質問するをいうサイクルが回るようになりました。
レビュアーではなかった頃は、疑問に思っても「動いているから良し！」で済ませることが多かったのですが、現在はコード一行一行を咀嚼し理解することでエンジニアとして少しずつレベルアップしていることを感じ、コードリーディングの重要性を再認識しました。

終わりに

ここまでつらつらとコードレビューに関して色々書いてきましたが、
コードレビューの本質はおそらく下記の言葉に詰まって、チームメンバ全員がこの言葉のように行動できたら心理的安全性の高い素晴らしいチームとなってコードレビューが楽しいものになると思っています。

わからないな。君はどう思う？
間違いや能力不足を見せることは弱さではない。
他人の意見を信頼すること。その正直さと強さによって、
みんなが尊敬してくれる。
~『Team Geek』~

Team Geek ―Googleのギークたちはいかにしてチームを作るのか

終わりの終わりに

トレタに少しでも興味を持っていただいた方がいれば、ぜひ遊びに来てくださいヽ(ﾟ∀ﾟ)ﾉ ﾜｰﾜｰ
仲間も募集しています！