「昔々、コードのレビューを行い、メールに行番号を示すコメントを書き込んでいました。とても面白かった。プロから：誰もdiffで何も見ず、IDEで見ました。しかし、マイナスもありました。いくつかのマージの後、行番号が変更されました。」
アレクサンダーマカロフ、イー
「私たちの会社には興味深いコンセプトがあります。それは椅子のリクエストです。これは、同じオフィス内で、開発者が椅子に座ってあなたに近づき、「ほら、これはプルリクエストを作成するよりも速い」と言うときです。
アントンモレフ、WormSoft
最近、YouTubeでコードレビューに関するSDCastポッドキャストの公開録音がありました。この号から最も興味深いものを選択して解読しました。
Spotify、Ya Musicのフルオーディオバージョン、およびダウンロード用のファイルとしてYoutubeの
フルビデオバージョン
コードレビューを何かをチェックしたりバグを探したりするような扱いをしないでください
Sergey Zhuk、Skyeng：コードレビューの基本的な目標は、チーム内で知識を共有し、最良の解決策を見つけることだと思います。チームは提案された変更を確認し、ドメインに関する知識がメンバー間で均等に分散されます。ソリューションの作成者は、完璧だと思ったコードを実際に改善する方法を理解しています。
したがって、コードレビューは回避したり、より迅速に実行したりする必要はありません。これはビジネスとチームへの投資です。コードが改善され、チームが改善されます。はい、最初はリクエストがラップされたときに気に入らなかった（そして誰がそれを望んでいるのか）。
しかし、それから私はそれを本を読んだり会議に参加したりするのと同じように、学習プロセスとして見始めました。
私はこれに加えて自分自身を感じました。開発者として、私はこのアプローチで育ちました。
しかし、ニュアンスがあります。確かにあなたの多くは、リファクタリングと機能、そしていくつかの小さな変更が混在している千行の要求に出くわしました。 1つのリクエストにさまざまなものを混在させることで、知識の共有とレビュー担当者の生活の両方が複雑になります。システムの動作が変更されたかどうか、ビジネス要件が満たされているかどうか、問題全体が解決されたかどうか、そしてソリューションは成功したかどうかを評価するのが難しくなります。
私たちはチームでこの瞬間に気づき、ルールを導入しました。1回のプルリクエストで、異なる性質の変更に干渉しないでください。リファクタリングは、個別に、個別に機能を、個別にマイナーな変更を送信します。
彼のチームの実践に関するセルゲイのレポート。テキストバージョンはこちらです。
これらのリクエストは通常、迅速かつ簡単にレビューされ、質の高いフィードバックを受け取る可能性がはるかに高くなります。また、メンテナの側には利点があります。リファクタリングが好きで、機能にバグがある場合は、リファクタリングを個別にフリーズできます。
アレクサンダー・マカロフ：コードのレビューにできるだけ時間をかけるべきではないことに同意します。開いて、括弧が大丈夫であるように、このコードは何かをします-それはそのようには機能しません。レビューアがコードを最後まで保証できない場合、コードレビューは実行されていません。
そのため、私たちは今、かなりの数のガイドラインを自分たちのためにまとめ始めており、そのうちの1つがコードレビューについて話します。
Yiiチームのガイドラインの一部。
しかし、個別の小さなプルリクエストに関する論文には、リードが来てそのようなものを紹介する状況がありました。チームは敵対的でした。どうやって手に入れたの？
Sergey Zhuk：私たちが相互作用してAPIを使用したのと並行してチームがありました。彼らは一ヶ月でたくさんの機能を作りました、私たちはほんの少しだけしました。つまり、当初は機能の提供ではなく、このアプローチによる品質に重点を置いていました。そして、リリースを遅くすることでビジネスに同意しましたが、何も壊さないようにしています。 1か月後、隣人の1人が故障し、次に別の隣人が故障しました。しかし、私たちはしません。そしてこの例では、誰もがすべてを理解していました。
Konstantin Burkalev、SDCast：私は一般的にコードレビューの実装プロセスを持っていました-そしてそれは簡単ではありませんでしたが、誰もがそれが良いことを理解しているようでした、はい。あなたは、「みんな、プルリクエストでマージしましょう」と言います。彼らはあなたにこう言います：「はい、小さな特徴があります。」
ここでの原則は本当にうまく機能します。何かが壊れたとき、あなたはこう言います。アイデアは、人々が自分の経験から間違いを理解することです。課そうとすることは必ずしもうまくいくとは限りません。
レビューの速さ
放送中、聴衆の間で投票が行われました。これがその1つです。
Konstantin Burkalev：6月は特に次のようになります。「まあ、あなたは私のリクエストを見ました、そこで大丈夫ですか？私はそれを書き、拳を握り締めて待ちました。」
そして、私には自分の仕事があります。夕方にしかそこに行けないか、まったくわかりません...
Sergey Zhuk：私たちのチームでは、レビューが常に優先されてきました。したがって、規制があります。プルリクエストが到着すると、コンテキストを失わないように、現在行っていることを終了し、レビューに進みます。あなたがやろうとしていることはまだ進行中であるからです。そして、そのタスクはすでに実行されています。
コードはすでに作成されており、それを調べてマージし、製品にアップロードするだけです。
つまり、私は自分で何かを終え、切り替え、すばやく見て、仕事を続けました。おそらく、1日に3回、レビューのために自分のタスクに気を取られてしまいます。しかし、繰り返しになりますが、私のチームでは、すべてが小さなプルリクエストに分割されており、それぞれ200〜300行のコードが含まれていることを理解する必要があります。したがって、最小限の時間が費やされます。
「誰のレビューは誰よりも重要ではない」
Konstantin Burkalev：これは疑問を投げかけます-誰がレビューすべきか。リードのみ？上院議員だけ？それとも、その人が成長しようとするために、能力の低い人に与えることができ、また与えるべきですか？
そして、何をレビューするか尋ねられたとき、人々はこのように答えました。
アレクサンダー・マカロフ：チームに多くの人がいて、リードが自分自身のボトルネックを作った場合、チーム全体の速度が低下し、その結果、チームがさらに悪化します。リードとして、Skyengで働いていたとき、ピーク時に1日に15回のプルリクエストがありましたが、最小のものではありませんでした。私は朝と夕方にレビューのための時間を取っておきました。
全員をレビューする必要があります。おそらく、「火事、すべてが火事になっている」という話を除いて、それはそこで悪化することはありません。
私はめちゃくちゃです、それは大丈夫です-私は何年もの間同じプロジェクトを使用していますが。そのため、今はできるだけ多くの人にリクエストを見てもらいたいと思っています。これは良いことであり、そうあるべきです。
誰もがレビューを信頼すべきかどうかは別の問題です。私には素晴らしいことを理解できる人がいましたが、彼らは焦点に関して大きな問題を抱えていました。たとえば、1人はレビューに6時間を費やしました。私は人々に彼らの時間を管理する方法を教えなければなりませんでした。
私たちが営利企業について話している場合、私は誰がこの義務を負い、誰が自由にレビューできるか、そしてこのステータスに応じて、1日あたりどのくらいの時間をレビューに費やすことができるかを特定することに賛成です。
アントンモレフ：私が見るように、さまざまなプロセスがあります。短期間でリリースする必要のある機能を実行している場合、6月にコードを表示させることはできません。しかし、ある種の社内製品を作成している場合、または期限が長くない場合は、はい、6月にリードまたはシニア開発者が行ったことを確認させることは素晴らしい習慣です。したがって、Junsは、プロジェクト全体で何が起こっているのか、そして意思決定メカニズムがどのように機能するのかをよりよく理解するでしょう。
「これはすぐに拒否されます」
Sergey Zhuk： Skyengは、コミットメッセージのチェックを実装しました。JIRAにタスク番号が必要です。そうでない場合、プルリクエストをマージできません。コードを開くと、そこに何があるのかわからないことがあります。タスクを開くと、何かを理解できます。
残りの部分については、最初は大変でしたが、タスクが完全に間違っているか、チームがアーキテクチャ的に同意しない場合にのみ、拒否することにしました。そして、承認を入れ、マージし、コメントを書き込みます。プルリクエストの作成者が必要な場合は、戻って修正します。そこで、彼は小さな粗さを修正するか、ある種のパターンを使用します。あなたの拒否慣行は何ですか？
アレクサンダーマカロフ：基準はあなたの基準と完全に一致しています-もちろん、タスクがうまく行われていない場合は、それをまとめる必要があります。コードが正常に見え、アーキテクチャ的にすべてがクールであっても。
, : , .
たとえば、「皆さん、テストを受けましょう」と言います。もちろん例外もありますが、テストは非常に重要です。その人がタスクを正しく理解したかどうかは彼らから明らかです。繰り返しになりますが、ユースケースではなくクラスとメソッドをテストする場合、これはすでに疑わしいものです。私たちは今、感染を台無しにしました、これは突然変異テストです。 Tulzaは同じテストを残しますが、コードを変更して実行します。また、変更されたコードが同じテストで合格した場合、コードの一部は必要ありません。コードを取得して削除するだけで、何も変更されません。
少し舞台裏。
また、もちろん、セキュリティとパフォーマンスの問題-ここでは拒否されます。些細なことは拒否しません。修正を依頼するか、自分で修正します。作成者のプルリクエストに直接プッシュし、完成したコードに、何を、どのように、なぜ実行したかを示します。
アントンモレフ：あなたが言ったことに関して-問題は解決されましたか？開発者は、ある問題を解決しているときに、別の問題を解決したことがあります。そのような状況を拒否する必要はありません。または、少なくともタスクがどのようにモデレートされたかを把握します。
Konstantin Burkalev：しかし、コミットメッセージをタスクトラッカーにリンクする瞬間は私にとって重要なようです。それが大いに役立つ日常の仕事があります。いつ知っていますか：「聞いてください、ボタンに関する問題の中で同様のことをしました。」ボタンに関するタスクを見つけ、番号を理解し、リポジトリログに移動し、それらのコミットの差を見つけて確認します。実際、これを台無しにしました。ここに移動できます。
しかし、逆も起こります。ここで1つのプロジェクトのソースコードを見ていて、バックエンドでaction684関数に出くわしました。
私は友人に手紙を書きます、私は尋ねます、コードのこのクールな名前は何ですか。彼は答えます-トラッカーのタスクへの参照。「なぜ関数の名前を思いついたのか、それをタスクの一部として書いているのです」...もちろん、通常の世界には存在しないはずのスレッシュ。