元記事
The Code Review Pyramid
https://www.morling.dev/blog/the-code-review-pyramid/
上記のブログ記事で紹介されていた内容がとても素晴らしかったので、和訳してみました。
ソースコードレビューに関する観点が適切にまとまっており、レビューに参加する方にぜひ読んでいただきたい内容です。
誤訳等あればご指摘ください。
本文
コードレビューでよくある現象として、コードのフォーマットやスタイルなどのありふれた側面に対して特に注目され、時間をかけて議論されることが挙げられます。一方で重要な側面(変更は目的を果たしているか、パフォーマンスは高いか、既存のクライアントに対して下位互換性を保てているか、など)は、あまり注目されない傾向があります。
この問題への意識を高め、焦点を当てるべき側面についてのガイダンスを提供するために、私は先日Twitterでとある図を共有しました。私はこれを「コードレビューピラミッド(Code Review Pyramid)」と呼んでいます。 この図の目的は、コードレビュー中に最も重要な部分(と私が思っていること)に焦点を当てることを助け、また自動化できる/すべき部分を示すことにあります。
何人かの方から、「コードレビューピラミッド」の永続的で参照可能な場所を求められたり、印刷用の高解像度画像を希望されたりしたので、ここに再掲することにします。
The Code Review Pyramid
(画像引用元:https://www.morling.dev/blog/the-code-review-pyramid/)
左側の説明
上部に行くほど「後から修正するのに、労力を要さない」
下部に行くほど「後から修正するのに、労力を要する」
上部2層は自動化しましょう!
下部3層にレビューの労力を費やしましょう!
Code Style(コードスタイル)
・プロジェクトのフォーマットスタイルに準拠していますか?
・命名規則に準拠していますか?
・DRYですか?(同様の実装を繰り返していませんか?)
・十分に"リーダブル"なコードになっていますか?(例:メソッドの長さなど)
Tests(テスト)
・全てのテストをパスしていますか?
・新機能は合理的にテストされていますか?
・コーナーケース (通常はあり得ないようなケース)はテストされていますか?
・可能な箇所でユニットテスト(単体テスト)が用いられていますか?必要な箇所でインテグレーションテスト(結合テスト)が用いられていますか?
・非機能要件に対するテストがありますか?(例:パフォーマンス)
Documentation(ドキュメント)
・新機能は合理的にドキュメント化されていますか?
・関連ドキュメントは用意されていますか?(例:README、APIドキュメント、ユーザーガイド、リファレンスドキュメント、など)
・ドキュメントは理解しやすいですか?重大なtypoや文法ミスはありませんか?
Implementation Semantics(実装セマンティクス)
・本来の要求を満たしていますか?
・論理的に正しく実装されていますか?
・不必要に複雑になっていませんか?
・堅牢な実装ですか?(例:並行性に問題がないか?適切にエラーハンドリングできているか?など)
・パフォーマンスは高いですか?
・セキュアな実装ですか?(例:SQLインジェクションされないか?など)
・計測可能ですか?(例:メトリクス、ロギング、トレーシング、など)
・新たに追加した依存関係(ここではライブラリやプラグイン等を指している)は、十分な機能を果たしていますか?ライセンスは許容できるものですか?
API Semantics(APIセマンティクス)
・APIは可能な限り小さく、必要なだけ大きくなっていますか?
・ある機能を実現する実装は1つに限られていますか?複数存在しないですか?
・一貫性を保っていますか?驚き最小の原則に従っていますか?
・APIと内部実装がクリーンに分割されていますか?内部実装がAPIに漏れていませんか?
・ユーザーが目にする部分に破壊的な変更が入っていませんか?(例:APIクラス、構成、メトリクス、ログフォーマット、など)
・新しいAPIは一般的に有用であり、過度に具体的ではありませんか?
FAQ
Q: なぜピラミッドの形なのですか?
A: ピラミッドの下部は、コードレビューの基盤となり、その大部分を占めている必要があるからです。
Q: おい、三角形じゃないか!
A: そう思うかもしれませんが、ピラミッドを横から見た図形です。
Q: 図面の作成にどのツールを使用しましたか?
A: Excalidrawです。