コードレビューで意識していること(する時/される時)

初めに

こんにちは。CYBIRDエンジニア Advent Calendar(http://qiita.com/advent-calendar/2017/cybird)
3日目の@keitarou です。2日目は@masatoshiitohさんの「TCPサーバーを書いてみよう」でした。

簡単な自己紹介

いまはCYBIRDでエンジニアをしており、主にスマホゲームのタイトル開発・運用などを担当しています。F2P型のよくあるソシャゲーってやつです。
一般的にはリードエンジニアや、ディレクターと呼ばれるような仕事をしています。
開発環境は、HTML5, Unity, iOS/Android, LAMP みたいなキーワードが多くを占めます。

今日書くこと

タイトルの通りですが、コードレビューの話をしようと思います。
普段はコードレビューとする立場が多いですが、コードレビューを人にお願いする時には今後こうせねば。っと自分への戒めと、普段レビューしている時に考えていること棚卸しです。

[ご注意]
※ チームの体制や、作っているソフトウェアの目的などにより人によって考え方や正解は異なると思いますので読んで戴いてる方は、あ〜こんなこと書いちゃい人もいるんだ。程度で受け止めていただけると幸いです。
※ 正直、個人的な理想論を語っていることがほとんどで、プロジェクトを進める上で出来てないこと、妥協していること・場合によっては効果的ではないことも沢山含んでいると思うので、現実的ではないかもしれません。

コードレビューの環境

• GitHubのPull Request上で行われる。
• レビューの指摘事項や質問などは基本的にはGitHubのPull Requetのコメントを利用
• 機能単位でレビューをすることが殆ど。(おおよそ2~3日問題なく集中できればきりが付く程度のものが多い、大きいものでは5~10日くらいの規模)※ bug fixなどに関しては、コード1行書き換えて終わりみたいなのも多い。
• 基本的にはコードレビューで何かしら、誰かしらのokサインがないとマージはしない。
• 基本的に、いつリリースしても良い状態が保たれるようにリポジトリ・ブランチは管理されている。
• プロジェクトやサービスによっては、Pull Requestに応じて自動でCI(テストコードの実行・静的解析)が行われる。もちろん、CIでグリーンな結果にならないことにはコードレビューは行われない。機械的にやり直し！になる。

根拠はないですが、同じような現場は多いんじゃないかな〜って印象があります。

レビューする時の話

1.仕様通りの出力結果であるか

言わずもがなですが、間違った仕様のものは軌道修正できるようにしないといけません。
あえて違うものを作ることはないと思うので、そもそもの仕様の伝え方などのコミュニケーションや仕様書や、指示書と呼ばれるようなものに問題があることがほとんどです。
レビュアーが勘違いしていることも多くあると思うので、まず息を整えてリラックスしましょう。

2.機能の「実行手段」「再現手段」の情報が埋められているか

極端な言い方をすると、テストコードが書かれているか(CIによる自動実行ができる状態で)です。
ゲームの開発などになると、長く保守したり、微妙な仕様変更などは発生して当たり前なので、テストコードによって作られた機能を簡単かつ迅速に実行できる状態であることは長期的な視点で価値がとても高いです。

ex.)
たとえば、ゲームのよくある「プレゼントBOX」から一括受取する機能があったとして、条件は100件のアイテムまで一括で受け取れる。101件の場合は一括受取後は、1件だけプレゼントが余ることになります。
この振る舞いを再現させることをテストコードなどにより自動化出来ていない場合、実行・再現により失われるコストは計り知れないです。

また、全ての機能・ソフトウェアの振る舞いをコード化、自動化させることはとても困難で、やろうと思えば出来ないことはないでしょうが、かけるコストを考えると合理的ではないことも多いです。
個人的な経験での話になりますが、GUIのアプリケーションの振る舞いを自動化させることはとても困難で満足仕組みを作れた試しはないです。
そういった場合は、自動化による再利用性などは一度諦め、ユーザーのデータがどのような状態の場合に、その機能が動くのか。あたりがコメントによって補足されていると良いかなと感じます。

また、「再現手段」に関しては、

• ビルドの実行方法
• 仮に、開発しているソフトウェアを流用して別のゲームタイトルを作る。としたときに差し替えないといけない情報(認証情報などがほとんど)であるのか。その時注意しないといけないことは何があるのか。
  など、いわいるナレッジといえるようなドキュメントをコードの実装と同時に用意されているか。なども合わせて見ることも場合もあります。

3.クライアント・サーバー間の通信仕様のドキュメント更新確認

開発途中や運用時に、サーバーAPIやマスタデータに対してkey/valueを追加する。
といったような事が多々ありますが、こういったAPIなどのブラックボックスなソフトウェアに関しては、入出力に関するドキュメントの更新もコードを書いた際に同時に修正をするようにしています。結構、これの更新漏れなどが多くてコードレビューのタイミングで更新を促すことが多いです。
先にドキュメントを固めることが殆どですが、更新漏れなど起こっていないかの確認です。
ちなみに今の現場ではサーバーAPIは、クライアント <-> サーバー , サーバー <-> サーバー
いずれも、フォーマット: JSON, プロトコル: HTTPなものがほとんどです。
マークダウンで入出力に関する情報を記載してGitHubでpublishするようにしているため、対象のマークダウンとの乖離の確認をすることになります。

4.仕様変更の発生、運用上での目的・仕様の変更時に耐えれるかどうか。耐えれなかった場合の軌道修正のヒントが埋め込まれているか

今日は良くても、明日は駄目。
長くユーザーに楽しんでもらうことが求められるF2P型のゲームでは、運用上での目的・仕様変更なんてものはあって当たり前で、ゲームをする側も作る側も楽しみの醍醐味でもあります。
プロダクトを考えている人との普段のコミュニケーションから、感を働かせて勝負をするところになります。
いきなり偉い人に肩を叩かれて、「次のアップデートで、XXXの機能をYYYな感じにできないかな？」といわれて、
「あー。それならマスタデータの入れ替えだけで出来るよ！」など返事ができるとベストです。
非エンジニアの人とエンジニアの中間役としての立場でコードレビューをする際などは特に意識をしてレビューをします。
【抽象化】などのキーワードがありますが、こういうところで活かすものだと感じます。

※ 注意は、予め担当者のエンジニアが実装に入る前に、このあたりの、今はこうだけど今後はね...みたいなネゴは必要です。ネゴ無しで自分で課題を抽象化して納期に間に合わせるエンジニア(実装者)は、おそらく超優秀な人！

といいつつも、頭ではわかっているんだけど、時間の都合上できないことも多いです。「技術的負債」であることと、例えばの解決手段など、未来の自分たちのために何かしらメッセージを残せるように誘導することが多いです。
コメントやドキュメントにその旨を記載することなど指摘することになります。

5. ライブラリ・フレームワークの使い方のヒント

ライブラリや、フレームワークの高級な機能を利用して、どのような評価や出力になるのかが分かりづらいところは、何かコードを保守する際にヒントになる情報が埋め込まれていると便利です。
ドキュメントのURLや、参考にしたサイトのURLなどコメントしておいてもらえるよう誘導するようにしています。

6. コーディング規約/その他標準化

• CamelCase, snake_caseの基本命名規則
• インデント
• 言語によっては「var」などの暗黙的型変換の利用のうんぬん

などの基本的なことはサラッと確認し

• 変数やメソッドの命名が、普段のコミュニケーションで用いているキーワードが使われているか
• クライアント・サーバー間での命名規則の乖離の確認

などを集中して確認するようにしています。

UnityなどGUIでポチポチ系だと、ヒエラルキーの構成順序や、プロパティの値などもチームでルール化していたりするため、確認をするとこもあります。(こればっかしは手元にソースコードを持ってきてGUIで確認しないといけないのがとても面倒。確認用のスクリプトとかでツール化自動化すれば採算が取れると思う)

7. 評価されないコードが埋め込まれていないか。敢えて埋め込んでいる事が表記されているか。

トライアンドエラーを繰り返して作られたコードなんかだと、トライアンドエラーの痕が評価されないのに残してしまったりしてしまうこともあります。
もしかしたら、使う機会が今後あるかも。や、実装者の意図など確認して残すこともあります。
残す際は何故、残したかのコメントを書くように誘導しましょう。
もちろん、使う機会が無い場合や、単純な消し忘れの際は消すようにしましょう!

8. 条件が揃う事でパフォーマンスの劣化が著しく発生しないか

• データ量に対してSQLの実行回数が増えるような実装
• データの探索ループが、余分だったり、非効率なものになっている。

など良くあるケースです。
リリースの都合などもあるので満足いくところまでチューニング出来ることの方が少ないと思いますが、ダメならダメで、例えばどんな解決策があるかをコメントで残すことや、後の方のタスクとして積むよう誘導するようにします。

9. 目的に最短経路で目的に辿り着いているか。課題解決手段の合理性の確認

もっとシンプルな解決手段があるけど、これではどうなの？ってコミュニケーションをするようにします。コード量が少なくシンプルであり、抽象的である形に持っていけるようサポートするように意識します。
役割重複の排除(車輪の再発明)もこれにあたります。

10. 実行環境の変化に耐えるコードか、特定の環境でしか動かないのか

プラットフォームやOSやミドルウェア、ランタイムのバージョンなどによって、動く動かない/振る舞いが変わる。のようなコードは避けれるようにする事と、仕方ない場合はそれが仕方ない事であることをコメントやドキュメントとして残せるように誘導する必要があります。
※ マルチプラットフォーム対応のフレームワークなどを利用するプロジェクトでは、このあたりは要注意。

11. 例外処理が不足なく表現できているか

例外処理は書いて当たり前って進め方が多いですが、あら？こういうケースの例外もありえんじゃない？という感じで、第二者視点で気づくことがあるはずなので、一度落ち着いて考えてみましょう。

12. スキルアップ・インプットを増やす

要件を満たし、かつそんなにツッコミもないのであれば、もっと抽象化して再利用性を高めることが出来ないか？少ないコード量で実現することは出来ないかを考えます。
ストレスが少ない環境下である時や、若手のエンジニアの教育用に無茶振りしたりする時には、ワンランクアップした実装を意識して、チームの引き出しを増やすチャンスとして利用することがあります。

13. 細かい単位・きりの良い単位でレビューする

• 工数が大きい機能
• 影響度が大きい機能
• チームにまだ慣れていないメンバーが担当する機能

などの、要は心配系に関しては完成していなくとも、何か動く状態である時や、考えている事をレビューさせてもらうようにします。
認識違いがあれば早々に立て直さないと勿体ないという事と、多分実装する側も相談しながらやりたいはずとなので。

コードレビューの際にあまり意識しなくてもいいこと

1. コーディング規約
   IDE・静的解析ツールなどが今だと強力で、わざわざ人間の工数を使わなくても機械的に行うことができます。もちろん、そういった面倒なことは機械的に自動化できる環境であることが大前提。

2. バグ探し/仕様通りかの確認
   内容にもよりますが、QAなどのコードレビュー後の後工程で確認するような内容に関しては、あまり意識はして粗探しはするようにはしていないです。自然と気づいたことを報告する程度が多いです。
   また、仕様に関しては後で変わって当然であるっていう考えもあるので、正解・不正解の評価より、状況が変化した時の対応の容易さのほうが価値があると、個人的には感じます。

[ご注意]
条件付きであること。

レビューされる時の話

では、少し文脈が変わり「レビューされる/してもらう」ときの話です。
基本的には上で、長々と話をした「レビューする時」の話の裏返しになりますのでざっくりです。

レビュワーの時短

• 再現手段の明記
• 画像や動画を有効に使う

レビューする際に必要な情報や、再現させるために必要な情報をできるだけ簡潔にさらっと準備する。
きっちり準備しすぎても、被レビュワーのコストが大きくなってしまうので、丁度バランスが良くコスト(時間)の採算が取れる位で良い。
GitHubのマークダウンなんかは丁度良く、画像や動画(gif)など、非常に使い勝手が良いので利用してみると早いかも。※ 特にGUIのアプリケーション開発などでは良くお世話になります。

レビューの精度向上

• 方向性を決めるコミュニケーション
• 大き過ぎず・小さ過ぎずの単位(1日1度は見せてフィードバックを得るように)

そもそも仕様が固まっていない場合のトライアンドエラーの結果のコミュニケーションや
どれくらい抽象化する必要ががあるか、目の前の課題に対してだけではなく、今後時間が経つと何が必要になりそうかなどの相談を、前もって話をして継続すること。
レビューワーをチェックする「門番」として扱うのではなく、色々な情報を引き出すための「先生」として利用することを心がけましょう。

立場の逆転

• 何故のツッコミどころを探す
• 余分なコードが残っていないか。チーム内のルールが適用されているかなどの、「そもそも系」の最終確認

自分がレビューするとしたら、このコードのどこに違和感を感じるだろうか。指摘したくなるだろうか。を考えてレビューよろしく！って言う前に一度落ち着いて見渡してみます。
不要なファイルcommitしちゃってる！みたいなことの多くもこのタイミングで潰すことが出来ます。

コードレビューの体制について

最後に、すこしだけコードレビューの体制に関しても少し整理しておきます。

カテゴリ

ざっくりカテゴリとしては、2つのパターンを見てきました。
切り分けとしては、

• 1つのソフトウェアに対して1人のレビュワーを置く「集中型」
• 1つのソフトウェアに対して複数人(できれば全員)がレビューワーとして機能する「分散型」

それぞれに優劣は特に無いと感じますが、それぞれにメリット・デメリットはあるので、用法用量を上手く使いましょう。ってことを感じます。

集中型

メリット

• 実装の標準化が容易
• コードを書く人が自分の実装に集中できる
• 要件と成果物を1点に集中できるためリリースやスケジュールの管理・調整がし易い
• レビューワーに溜まるノウハウの濃度が高いため、短期的に成果を出す必要がある際にレビューワーがコード書くと対応早い事が多い。※他の工程が止まるので危険牌(バーサーカー化)

デメリット

• ノウハウの分散が困難。
• 可用性の低下。レビューワーが他の割り込み対応などで止まって渋滞発生など

分散型

メリット

• 結果的に、ノウハウも合わせて分散することができる
• 1:1での合意形成ではなく、レビューワーが多人数になり多方面での意見を取り込めるため、実装の精度上がる。

デメリット

• 実装の標準化・ワークフローの標準化への対応が難しい
• 書いたり、見たりのコンテクストスイッチングによる集中が途切れる

ざっくり、私の経験から思い浮かぶことを書いただけですが。
結局はそれぞれに長短があるので1つのプロジェクトや、1つのソフトウェアであっても、必要に応じて価値の高い方法をこまめに選択するのが良いと感じます。
例えば、時間をかけてでもじっくり品質を上げたい場合は、「分散型」で多様な意見を取り込む。単純作業でしか無いため出口管理のために集中型で囲い込むとか。

ここ1年くらいは限られて時間の中でどうしておくのが、コスパいいか。
みたいな観点で物事判断する事が多かったので、今後の現場の雰囲気やサービス次第では、自分の価値観も変わるかも。

最後に

以上!!
長々とやってしまいましたがこれで、おしまいです!!
個人的には自分の振り返りにできたので書いてみて良かったです。

CYBIRD エンジニア Advent Calendar(http://qiita.com/advent-calendar/2017/cybird) 明日は、@ntrvさんの「golang 知っておくと役立つ5つのTips」らしいです！楽しみですね！