通常、コードレビューはどのように行われますか？ プールリクエストを送信し、フィードバックを取得し、修正を行い、2回目のレビューのために修正を送信してから承認を取得すると、マージが発生します。 簡単に聞こえますが、実際にはレビュープロセスは非常に時間がかかる場合があります。
何百行もの変更があるプールリクエストがあるとします。 レビューアは、コードを完全に読み、提案された変更を理解するために多くの時間を費やす必要があります。 その結果、プールリクエストの作成から承認までのプロセス全体に数日かかる場合があります。これは、レビュー担当者と変更の作成者の両方にとってあまり快適ではありません。 そして、最終的にはレビュアーが何かを見逃す可能性があります。 または、チェックが表面的すぎる場合があり、最悪の場合、プール要求はすぐに拒否される可能性があります。
プールリクエストが大きいほど、それをチェックすることによるメリットは少なくなります。
そのような状況を避ける方法は？ プールリクエストをレビュー担当者がより簡単に理解しやすくし、プロセス全体を最適化する方法
Skyengモバイルアプリケーションチームのコードレビュープロセスの仕組みに関するバックエンド開発者のSergey Zhukによる記事を翻訳しています。
カテゴリを変更する
プロジェクトに新しい機能を実装するというタスクがあることを想像してみましょう。 作業中のプールリクエストには、異なるカテゴリの変更が含まれる場合があります。 もちろん、そこには新しいコードがいくつかあります。 しかし、作業の過程で、新しい機能の追加に貢献するために、一部のコードを事前にリファクタリングする必要があることに気付くかもしれません。 または、この新しい機能により、排除したいコードに重複が現れました。 または、突然間違いを見つけて修正したい場合。 最終的なプールリクエストはどのようになりますか？
最初に、コードでどのようなカテゴリの変更が発生するかを見てみましょう。
- 機能の変更。
- 構造的リファクタリング-クラス、インターフェース、メソッドの変更、クラス間の移動。
- 単純なリファクタリング-IDEを使用して実行できます（たとえば、変数/メソッド/定数の抽出、条件の簡素化）。
- クラスの名前変更と移動-ネームスペースの再編成。
- 未使用（デッド）コードの削除。
- 修正コードスタイル。
戦略レビュー
これらのカテゴリはそれぞれ異なる方法でレビューされることを理解することは非常に重要であり、それらを検討する場合、レビュー担当者は異なることに集中する必要があります。 変更の各カテゴリには、独自のレビュー戦略が関係していると言えます。
- 機能変更：ビジネス上の問題とシステム設計を解決します。
- 構造的リファクタリング：後方互換性と設計の改善。
- プリミティブリファクタリング：可読性の向上。 これらの変更は、主にIDEを使用して実行できます（たとえば、変数/メソッド/定数の抽出など）。
- クラスの名前変更/移動：名前空間の構造は改善されていますか？
- 未使用コードの削除：後方互換性。
- 修正コードスタイル：ほとんどの場合、プールのマージ要求はすぐに発生します。
異なるカテゴリの変更を確認するために異なるアプローチが使用されるため、レビューに費やされる時間と労力も異なります。
機能の変更。 ドメインロジックの変更を伴うため、これは最も長いプロセスです。 レビュー担当者は、問題が解決されたかどうかを確認し、提案されたソリューションが最適であるか、改善できるかどうかを確認します。
構造的リファクタリング。 このプロセスは、機能の変更よりもはるかに短い時間で済みます。 ただし、コードをどのように編成するかについては提案や意見の相違がある場合があります。
ケースの99％で残りのカテゴリをチェックすると、マージがすぐに発生します。
- シンプルなリファクタリング。 コードが読みやすくなりましたか？ -メルジム。
- クラスの名前変更/移動。 クラスはより良い名前空間に移動されましたか？-Merzh。
- 使用されていない（デッド）コードを削除するのはmerzhimです。
- 修正コードのスタイルまたはフォーマット-merzh。 あなたの同僚はコードレビュー中にこれをチェックすべきではありません。これはリンターの仕事です。
変更を分類する必要があるのはなぜですか？
さまざまなカテゴリの変更が異なって改訂されることについてはすでに説明しました。 たとえば、ビジネス要件に基づいて機能の変更を検証し、構造的なリファクタリングの場合、後方互換性を確認します。 また、複数のカテゴリを混在させる場合、レビュー担当者が複数のレビュー戦略を同時に念頭に置くことは困難です。 そして、ほとんどの場合、レビュアーはプールリクエストに必要以上に多くの時間を費やし、このために何かを見逃す可能性があります。 さらに、プールリクエストにさまざまな種類の変更が含まれている場合、修正を加えると、レビュアーはこれらすべてのカテゴリを再度修正する必要があります。 たとえば、構造的なリファクタリングと機能的な変更が混在しています。 リファクタリングはうまく実行されていても、機能の実装に問題がある場合でも、修正後、レビュー担当者はプールリクエスト全体を最初から確認する必要があります。 つまり、リファクタリングと機能の変更の両方を再確認します。 そのため、レビュアーはプールリクエストにより多くの時間を費やします。 すぐに別のプールリクエストをリファクタリングで検討する代わりに、レビュー担当者はコード全体をもう一度レビューする必要があります。
まったく混合する価値がないもの
クラスの名前変更/削除とそのリファクタリング。 ここでGitに出会いますが、Gitはそのような変更を常に正しく理解しているわけではありません。 多くの行が変更されると、大規模な変更を意味します。 クラスをリファクタリングしてからそれをどこかに移動すると、Gitはそれを移動しているとは認識しません。 代わりに、Gitはこれらの変更を1つのクラスを削除して別のクラスを作成すると解釈します。 これは、コードレビュー中に多くの質問につながります。 そして、コードの作者は、彼がこのlyいコードを書いた理由を尋ねられますが、実際には、このコードは、小さな変更を加えてある場所から別の場所に移動しただけです。
機能の変更+リファクタリング。 このケースについてはすでに上記で説明しました。 これにより、レビュアーは2つのレビュー戦略を同時に念頭に置くことができます。 リファクタリングがうまく行われたとしても、機能の変更が承認されるまでこれらの変更を遅らせることはできません。
機械的な変更+人による変更。 「機械的な変更」とは、IDEまたはコード生成を使用して行われるフォーマットを意味します。 たとえば、新しいコードスタイルを適用し、3000行の変更を取得します。 そして、これらの変更を人によって行われた機能的または他の変更と混合する場合、レビューアーにこれらの変更と理由を精神的に分類するように強制します：これはコンピューターによって行われた変更です-スキップすることができ、人によって行われたこの変更が必要ですチェックアウト。 そのため、レビュアーはチェックに多くの余分な時間を費やしています。
例
Memcachedへのクライアント接続を緩やかに閉じるメソッド関数を使用したプールリクエストを次に示します。
プールリクエストが小さく、100行のコードしか含まれていない場合でも、検証することは依然として困難です。 コミットからわかるように、さまざまなカテゴリの変更が含まれています。
- 機能的（新しいコード）、
- リファクタリング（クラスの作成/移動）、
- 修正コードスタイル（余分なドックブロックの削除）。
同時に、新しい機能自体はわずか10行です。
その結果、レビューアはコード全体をレビューし、
- リファクタリングが正常であることを確認します。
- 新しい機能が正しく実装されているかどうかを確認してください。
- この変更がIDEまたは人によって自動的に生成されたかどうかを判断します。
したがって、このようなプールリクエストを確認することは困難です。 状況を修正するには、これらのコミットを別々のブランチに分割し、それぞれのリクエストのプールを作成します。
1.リファクタリング：クラス抽出
ファイルは2つだけです。 レビューアは、新しいデザインのみをチェックする必要があります。 すべてが正常な場合-merzhim。
2.次のステップもリファクタリングです。2つのクラスを新しい名前空間に移動するだけです
このようなプールリクエストの検証は非常に簡単で、すぐにインスタンス化できます。
3.余分なドックブロックの削除
ここで面白いことは何もない。 メルジム。
4.機能そのもの
そして、機能の変更を伴うプールリクエストには、目的のコードのみが含まれます。 そのため、レビューアーはこのタスクのみに集中できます。 プールリクエストは小さく、簡単に確認できます。
おわりに
経験則：
変更のカテゴリが混在する巨大なプールリクエストを作成しないでください。プールリクエストが大きいほど、レビュー担当者が提案した変更を理解するのが難しくなります。 おそらく、数百行のコードを含む巨大なプールリクエストは拒否されます。 代わりに、小さな論理部分に分割します。 リファクタリングが正常であるが、機能の変更にエラーが含まれている場合、リファクタリングを簡単に抑えることができるため、あなたとあなたのレビューアは最初からすべてのコードを見なくても機能に集中できます。
そして、プールリクエストを送信する前に、常に以下の手順を実行します。
- 読み取り用にコードを最適化します。 コードは書かれているよりもずっと読みやすいです。
- 理解に必要なコンテキストを提供するために、提案された変更を説明します。
- プールリクエストを作成する前に、必ずコードを確認してください。 そして、それが他の誰かのコードであるかのようにしてください。 時々、見逃したものを見つけるのに役立ちます。 これにより、プールリクエストが拒否される確率と修正の回数が減ります。
同僚のオレグ・アントネヴィッチは、変更をカテゴリーに分割するというアイデアについて話してくれました。
編集者から：SergeyはプログラミングとPHPについて多くの興味深いことを書いており、時にはストリーミングビデオサーバー 、 HTMLファイルのレンダリングなど 、何かを翻訳しています 。 この記事へのコメントで彼に質問してください-彼は答えます！
まあ、私たちは開発者にとって常に多くの興味深い空席があることを思い出させてくれます！