はじめに
こんにちは。
プロダクト開発人材の副業転職プラットフォーム Offers を運営する株式会社 overflow のエンジニアの藤井です。
エンジニアであれば誰しも日頃からコードレビューをしたり、されたりしていることと思います。
健全な開発組織を育む意味でもレビューの文化を根付かせることはとても大切ですが、小規模な組織の場合「レビューアが足りない」という問題が往々にして起こり得ます。
もちろん、特定のエンジニアにコードレビューが集中してしまうのを防ぐために、チーム全体で負荷分散を図るのが本質的かつ王道的なアプローチではあります。
しかしときには、とにかく個人の力で乗り越える、という状況も避けられないでしょう。
そこで今回はコードレビューの生産性を上げるための Tips をいくつかご紹介します。
自分でも開発をしなければならないが、その片手間で一日に何本ものプルリクエストを確認しなければならない、そんな方の一助になれば幸いです。
コードレビューの目的を整理する
まずはコードレビューについて、目的を整理することから始めます。
一般的にコードレビューをする目的とは、以下のいずれか(または全部)になるはずです。
- バグの防止
- (バグとは言わないまでも)コードベースの品質保証
- チーム内でのナレッジの共有
- 若手メンバーへの指導・教育
これらのうち、「バグの防止」などはミッションクリティカルな性質を持った目的と言えるでしょう。
つまり、そこをしくじると事業全体に影響を及ぼす可能性があり得るケースです。
当然、こういったクリティカルな問題について「レビュー = 最後の砦」といった状況は可能な限り避けるべきです。
クリティカルな問題は機械的に防止できるようにする
そもそもレビューはヒューリスティック(経験や先入観によって直感的に、ある程度正解に近い答えを素早く得ること)な手法です。
つまり、必然的に一定の誤りからは逃れられません。
そこでレビュー以前に、テストやコード解析の自動化といった機械的なアプローチを取り入れることが重要になります。
最悪の場合、一才のレビューをせずにデプロイしても致命的な問題は起こり得ない、という状態が作り出せれば、レビューアの精神的負担は大きく軽減されます。
また問題の中には、どうしても最後はヒューリスティックに確認しなくてはならないものもあります。
たとえば"リーダブルなコードか"といった問題は、機械的な検出が難しいでしょう。
e.g. 関数や変数の名前が適切か
そういった問題にレビューアが集中できるようにするためにも、クリティカルな問題は機械的に防止できるようにしましょう。
確認範囲を小さく保つ
では、ヒューリスティックでのみ検出できる問題についてはどう向き合うべきでしょうか。
前提として、人間の認知能力には限界があります。
たとえば数百ファイル、あるいは数万行も修正された及ぶプルリクエストがあったとしたら、まずレビューは不可能でしょう。
コードレビューを効率的に回すためにも、レビューアに依頼するプルリクエストは細かい単位で作成するように、チーム内で事前に話し合っておくべきです。
観点を整理する
またレビューの効率を上げるためには、レビューア自身が明確な観点を持つことが重要です。
汎用的なレビュー観点については、
Google's Engineering Practices documentation(日本語訳)
が参考になるので一読をおすすめします。
そのほかにも、例として以下のような観点があり得ます。
- セキュリティ e.g. バリデーションが適宜行えているか？
- 性能、処理オーダー
- エッジケースでの振る舞い
観点はいろいろと挙げることできますが、絶対の正解はありません。
また観点を幅広く持ってレビューに臨めば、より多くの問題に気がつけるかもしれませんが、
常に考えうるすべての観点でレビューするのは(主に工数の観点で)現実的とは言えません。
そのため、レビューの際は適宜、観点を取捨選択することが必要になります。
逆に言えば、観点を適切に絞り込むことさえできれば、レビューの品質を落とすことなく工数を削減できます。
信用を評価する
レビューをする前に、あらかじめ信用を推しはかることも有用です。
若干、諸刃の剣であることも否めませんが、とは言え、工数と品質のトレードオフを考えるとむしろ現実的な手段と言えるでしょう。
マネジメントの心構えである「信頼しても信用するな」という言葉を聞いたことがあるでしょうか？
前者の「信頼」は、たとえ根拠がなくても相手を頼りにし、その心情を伝えるという姿勢そのものを意味します。
一方で後者の「信用」は、過去の実績などあくまで事実を根拠にして、どれだけ確実であるのか信じられる度合いを意味します。
レビューをする際に、事前に成果物の信用を適切に推しはかることができれば、不要な観点を捨てることができ、結果的に工数を大きく削減できる可能性があります。
この場合、考慮すべき信用は大きく分けて 3 つ考えられます。
- プロセスへの信用
- 人(作業者)への信用
- モノ・コトへの信用
以下、それぞれ深掘りしていきます。
プロセスへの信用
ソースコードという成果物そのものではなく、プロセスを確認することで、ソースコードの信用度合いを推定できます。
- コードを実装する以前の設計ができているか(ドキュメントや中間成果物があるか)
- 自動テストはすべて通っているか
- 開発者自身によるテストが行えているか
- セルフレビューがされているか
こういったプロセスを適切に通過しているならば、ソースコードへの信用も一定の水準を保っているはずです。
もしもプロセスが行えていないならば、作業者に対してそれらを全て行うよう要求できます。
また、こういったプロセスを仕組みとしてチームであらかじめ決めておくこともできます。
そもそもコードレビューもまた、製品やソースコード、成果物の信用を高めるための一工程にすぎません。
開発工程はいわばそのプロセス全体を通して、顧客に価値を提供できるようになるまで製品の信用を高める過程と言えます。
コードレビューという一工程であらゆる信用を担保しようとするのではなく、各工程でしかるべき基準の信用を達成するための工程設計が重要です。
人(作業者)への信用
信用を推定する対象としては当然、作業者であるエンジニアそのものも考えられます。
たとえば、経験豊かなエンジニアのコードに対して、膨大な認知コストを払い、厳格なレビューをするのはさほど効果的とは言えません。
一方でまだ若手のエンジニアのコードに対しては手厚くレビューし、教育を兼ねたフィードバックを与えることは組織運営の観点でも重要なことと言えるでしょう。
作業者の信用度合いによって、レビューの仕方を変えることは効果性を高める上でも重要です。
また当然ですが、レビューア自身の信用も常に他人から評価されていることを忘れるべきではありません。
誰が相手でも謙虚な姿勢で臨み、常に事実を根拠としたフィードバックを心がけるべきです。
モノ(開発対象)への信用
当然ですが、開発の対象物によっても信用は変化します。
システムに対する不可逆な変化を与える改修や、課金や認証といった事業の上でも影響の大きな構成要素に対する開発は当然、慎重にレビューされるべきです。
これらは対象物自身の信用度合いというより、対象の重要度が高いために相対的に作業者の信用度が低下した状態とも言えます。
チームにとっては未知が多い開発もまた、似たようなケースと言えるでしょう。
また、度重なる開発の結果、複雑度が増したモジュールは信用が低下した状態とも言えます。
そういったモジュールに手を加える場合も当然、慎重なレビューが必要になります。
逆に言えば、コードの複雑度を低くし、リーダブルに保つことはレビューのコストの節約にも繋がります。
読みやすい綺麗なコードは多くの問題を癒してくれます。
危険な兆候には立ち止まる
信用を推定することで、レビューのコストを下げることができる、という話をしましたが、当然ながらレビューの第一目的はコストを下げることではありません。
コストを下げることが目的なら、レビューそのものを止めればすみます。
レビューの費用対効果を最大にする、という観点で言えば、コードの流し読みでも危険な兆候に気がつくことができれば良いとも言えます。
必要な場合に立ち止まることができれば、あらためて厳格にレビューして、問題を拾い集めることも可能だからです。
この点で有名なのは、「コードの不吉な匂い」でしょうか。
言語化が難しいですが、私自身がコードを読んでいて引っかかる兆候を思い出してみると、以下のようなケースが思い当たります。
簡単に理解できない
流し読みしていて、簡単にプログラムの挙動が理解できないケースです。
「簡単に」というのがミソで、また人ごとのばらつきも多い要素だと思いますが、少なくとも自分の脳が認知できなかった、という事実が生じた時点で対象コードの複雑性に対して疑問を持つようにしています。
認知が難しい、ということはそれだけでバグの原因となり得ます。
そもそも実はコードを書いた人間でさえ、プログラムの挙動を正確には理解できていない可能性もあります。
未熟、あるいは粗悪
未熟、あるいは粗悪なコードというのは読めばわかるものです。
そういったコードは思いつきをずらずらと書き並べた文章に似ています。
全体的な設計に乏しく、整理されておらず、適切に分割されないので長大だったりします。
時間的または精神的余裕がない状況では、そういうコードが量産されやすいものです。
しかし、ことコードの品質において Quick&Dirty は幻想です。
品質と開発スピードのトレードオフについて良記事があるので、紹介させていただきます。
過去のプロジェクトでそういったコードが残され、後任に託されるのもよくあることだと思います。
その場合でもレビューアは当然の責務として、
- ちゃんとレビューする ※再利用や安易な踏襲を防ぐ
- 作業者にリファクタを促す
といった努力が必要になるでしょう。
自分のメンタルモデルと一致しない
コードレビューする際、まず最初にプルリクエストの説明書きやドキュメントからコードの目的を把握することから始まります。
その際、レビューアは自分なりの設計に対するメンタルモデルを作り上げるはずです。
※つまり、「自分ならこう作る」「こういうふうに動くはず」という漠然とした予断的なイメージ。
そして自身のメンタルモデルと照らし合わせるようにコードを読んでいくわけですが、
それと一致しない場面では当然、疑問が生じるのでその時点で立ち止まるようにしています。
あるべきものがない、存在理由がわからない(ブラックボックスに感じる)など、理由は様々ですが生じた疑問は必ず解消されなければなりません。
わからないことをわからないままにしないのは、レビューアが果たす最低限の義務だと考えます。
まとめ
今回は私自身の経験も踏まえて、コードレビューの生産性を高める Tips について紹介させていただきました
- 機械的なアプローチを取り入れて、レビューの負担を減らす
- レビューの範囲を小さく保ち、目的/観点を明確にする
- 信用に応じて、レビューの仕方を調整する
- 危険な兆候には立ち止まる
最初に書いたとおり、より良い製品や開発組織を生み出すためには効果的なレビューは欠かせないものと私は信じております。
しかし、レビューはヒューリスティックな手法である以上、非言語的な要素を多分に含み、習得にはそれなりに時間がかかる物でもあります。
日頃からコードレビューに取り組んでいるエンジニアの皆様に、本記事が少しでもお役に立つことができれば幸いです。
関連記事
副業転職の Offers 開発チームがお送りするテックブログです。【エンジニア積極採用中】カジュアル面談、副業からのトライアル etc 承っております💪 jobs.overflow.co.jp
Discussion