RAKSUL TechBlog

ラクスルグループのエンジニアが技術トピックを発信するブログです

レビューに追われているエンジニアのためのレビュー観点と円滑なレビューの仕組みについて

ラクスル事業本部のサーバーサイドエンジニア兼エンジニアリングマネージャーの荒井です。

ラクスルに入社して4年半になりますが、チームのリードエンジニアを経て、今はエンジニアリングマネージャーを兼務しています。普段は開発とマネジメントが半々くらいで、具体の実装は他のエンジニアにお願いし、自分は設計やレビューにかける時間が多いです。

今回は、そんな私の視点から、普段どういった観点でレビューをしているかについてご紹介します。

まず前提条件を確認する

レビューはトレードオフです、時間をかけすぎずにいかに効率的に行うかが重要です。我々のリソースは有限で、レビューを素早く返すことによってエンジニアのフロー効率を最大化できます。したがって、以下のようなレビューの前提条件を確認することは、具体的なレビューの内容と同じくらい重要だと考えています。

どれくらいレビューを急ぐか

例：現在発生している障害の対応

例：他の実装のブロッカーになっているタスク

特に急ぐ内容であればレビューの依頼時に声を掛けてもらったり、GitHubのPull RequestであればDescriptionに記載するようにしていることが多いです。 極力1営業日中に返すのが望ましいですが、難しい場合は他の人にお願いをするケースもあります。

どれくらい丁寧に見るか

例：書き捨てのスクリプト・近々廃止予定のコード

例：決済、認証周りなどセキュリティリスクの高い機能

不具合が出たときのリスクが高い機能が含まれるか、例えば料金や決済周り、やり直しの効かない処理などの部分については念入りに確認します。逆に、ワンショットのスクリプトや、近々廃止予定のコードなどであれば最低限の確認で済ませることがあります。 レビュイー視点でも特に見て欲しい観点、不安な箇所があればレビュー依頼時に伝えてもらうようにしています。

何を目的にレビューをするか

品質保証

これについては詳述します

知識の継承

設計レビューやコードレビューはメンタリングの重要な機会でもあります。 したがって、レビュー時には、以下のようなレビュイーの属性も見ています。

  • 使用言語/フレームワークについて詳しくないメンバー
  • チームに新しくジョインしたメンバー

メンターがレビュアーに入っている場合は任せることも多いですが、その言語やフレームワークに慣れていないメンバーであれば、その言語やフレームワーク「らしい」書き方かどうかを見たり、便利なメソッドや他の書き方についてもコメントすることが多いです。言語に慣れている人であっても、例えばPRをどういう単位で出すか、フレームワークの機能やクラス分けをどのように扱っているか、テストはどういった観点で行っているか、などの暗黙知的な開発ルールについては、新規にジョインしたメンバーにはまだ浸透していないことがあるため、これらのレビューコメントがオンボーディングの機会になっている側面があります。

品質のために何をレビューするか

機能要件

正常系についてはあまり詳細に見ないことが多いです。基本的には正常系はテストされていることが多く、テストが書かれていてそれが通っていれば、機能的には満たされているはずだからです。したがって、下記のような内容は確認しますが、観点としてはあまり重要視していません。

  • 仕様はどのようなものか、仕様を満たしているか
  • テストが過不足ないか

一方で以下のような異常系については時間を取って見ます。

  • エラーケースが十分か
    • 例：transactionが正しく貼られているか
    • 例：外部システム連携があるか、ロールバックは適切にできるか
    • 例：DB、SaaSなどの障害時の挙動が適切か

非機能要件

機能要件はどのようなメンバーもレビューしやすいのに対して、非機能要件については特に経験のあるエンジニアほど見るべき部分だと思っています。以下に例をあげます。

  • 保守性
    • 命名規則がルール通りか、既存のコードに照らし合わせて違和感がないか
    • 役割が適切に分かれて適切に配置されているか
  • 同期・非同期処理が適切に選択されているか
    • 同期処理で時間のかかる処理が含まれていないか
      • 例：ファイルのコピー
    • 非同期処理のタイムラグで不具合が出ることがないか
      • 例：transaction内部でのenqueue
  • レスポンスタイム悪化の懸念がないか
    • 例：データ量の増加
    • 例：連携先システムの影響
  • リリース・リバート時の懸念がないか
    • 例：リリース順序やデプロイのタイムラグによって不具合発生の可能性がないか
    • 例：DBのカラム追加や変更が含まれているか、オンラインDDLが効くか
    • 例：DBのカラム削除があるか、リバートは容易にできるか

円滑なレビューのために

上にあげた同期・非同期処理の選択や、保守性の観点については、設計の話に近く、レビュー依頼があがってきた時点で指摘をすると変更コストが高く手戻りを生むケースが多いです。したがって、タスクの詳細化時点であらかじめ認識を合わせておいたり、ペアプロをするのも、開発のリードタイムを短くする手法としてよく用います。レビュー段階でいえば、指摘や相談事項が多い場合はペアレビューの時間を取って通話しながら行うことも多いです。

また、レビューの前提条件については、レビュアー・レビュイー両者が把握しやすいよう、GitHubであればPR Templateを活用してひと目でわかるようにしています。

## Summary

Write background of changes, summary, or point of view for reviewers etc.
Attach a screenshot if this PR changes the view.

For more information about QA testing, release, etc.,
please refer to[Basic Development Flow](https://www.notion.so/...).

## Links

<!-- e.g. notion, clickup -->

## Review Deadline

## TODO

## How to release

## QA

おわりに

レビューという工程は、開発者同士が対話し、より良い設計や実装について考えたり、チームのカルチャーを作っていくとても楽しい工程だと自分は思っています。この記事が、レビュアーにとってもレビュイーにとっても円滑なレビューの参考になれば幸いです。