はじめに
エンジニアとしての経験がある程度増えてくると、コードレビューを担当することも増えてくると思います。
コードレビュー時にするべきことはたくさんあると思うので、あえて今回はコードレビュー時に "しないこと" をシンプルに３つまとめます。
これを知ることで、コードレビューを効率よく的確に行えると思います。
コードレビュー時に "しないこと" ３選
- ローカルで動作確認しない
- 「動いているからいいか」をしない
- 代わりに実装しない
慣れている人にとっては当然意識していることかもしれませんが、
詳細がとても大切ですので、できているかぜひチェックしてみてください。
1. ローカルで動作確認しない
これをやってしまうと起こる問題は...
- 非効率
とにかくこれです。
動作確認はあくまでプルリク作成者に行ってもらいましょう。
プルリク作成時のルールとして「デザインの変更はスクショ必須」「ユーザーの動作を伴う変更はスクリーンレコーディング必須」と取り決めておくと良いでしょう。
2. 「動いているからいいか」をしない
これをやってしまうと起こる問題は...
- パフォーマンス低下の原因になり得る
- 今後の開発負債となり得る
- 相手の学習機会を奪ってしまう
といったことです。
後でしようと思っていることは、することはないと思っておくと良いでしょう。
ほんの少しパフォーマンスに影響があれば、それが繰り返されボトルネックになっていく可能性もあります。
関数の命名が適切でなかったり、もっとシンプルにかけるロジックを放置しておけば、その後の開発に無駄な工数が増える危険性も含んでいます。
今取り組むことが最善だと思いましょう。
また、プルリク作成者は「今がベスト」だと思っている可能性もあるため、学習や知るきっかけを奪うことにつながります。
もっと良い書き方があるのにレビューしないことは、レビュワーの最大の罪です。
どうしても急がないといけない場合は「必ずその場で」期限を明確にしてissue化・タスク化しましょう！
3. 代わりに実装しない
これをやってしまうと起こる問題は...
- 今後のレビュー負荷が減らない
- 相手の学習機会を奪ってしまう
といったことです。
レビューされることによって、プルリク作成者たちはちょっとずつ成長していきます。
それに伴って、レビュワーのレビュー負荷は減っていくものです。
しかし、
「自分でやった方が早いな...」
「伝えるのが難しいな...」
などによりレビュワーが代わりにやってしまう場合も多いと思います。
これはレビュワー・プルリク作成者たち両方にとって大きなデメリットです。
レビュワーの負荷は減りませんし、プルリク作成者たちの学習機会を奪います。
何より、２でも記載した通り、もっと良い書き方があるのにレビューしないことはレビュワーの最大の罪です。
こちらもどうしても急がないといけない場合は、代わりに行った上で
「どうしてこの変更をしたか」
などの意図を必ず丁寧に伝え、別途プルリク作成者なりに修正してもらいましょう。
終わりに
レビューをすることは、レビュワー・プルリク作成者たちの双方にとってたくさんのメリットがあります。
今回の３つを意識して "しない" ことでそのメリットを最大限に活かしましょう！
Comments
＞ローカルで動作確認しない
えーっ？😱 どうやってテストすんの？
怖すぎる😨
＞ローカルで動作確認しない
わかる気はしますが…CI/CDなどでテストが通っていることが前提ですよね？
ローカルで動作確認しないのはあくまでレビュワーでレビュイーはしていることが前提と書いていますね。
まぁ私は動かしながら確認しないと理解できないことが多いのでやってしまいますが。
個人的には1-3まで全部真逆ですね。
教育の観点で考えるとこうした方がいいよねというのは分かるのですが、タイトルが釣りっぽくなってるし勘違いする人も多そう。
プルリク…github経験者からするとPull requestsの略かもと思えるんですが、そうでない人にとってこれは一般的で通用する言葉なのか疑問
GitHubもSourcetreeもBitbucketもGitLabもBitBucketもgitを基盤としたサービスです。
そのサービス内で使われる用語、それがプルリクエスト、略してプルリク。
これでシェア8割くらい行ってるんじゃないかな。ここまで復旧しているシステムに対して知識がない=コードのバージョン管理ができていない、コード品質は黄色信号（普通は赤）です。
もしかしてapp_{yyyymmdd}みたいなことしてます笑？
git以外のバージョン管理システムもそこそこあるので、プルリクを知らないイコールバージョン管理していないと見るのはちょっと乱暴かと
はGitHubのようなプルリクエストが存在するシステムしか知らないということでしょうか。
1をしないことで、追試の効果があると思いますがいかがでしょうか？
2と3は、このような対応をする方がレビュワーとして適しているのかと疑問に思うのですがいかがでしょうか？
2で"パフォーマンス低下"を挙げられていますが、そのプルリクエストの範囲だけでパフォーマンスを測れるものなのでしょうか。
"どうしても急がないといけない場合"と注釈されていますが、そうならば、仕事を割り当てるべきではないのでは？思いますがいかがでしょうか？
@fugusuki2202
ツッコミ待ちだったらアレだけどGitlabはMerge Requestではなかろうか
SourcetreeはただのGUI GitクライアントだしGitホスティングサービスと同列に並んでるのはよくわからんな・・・
まあマイナー処だとAzure ReposとかCodeCommitは実際PRを採用しているし
シェア率の高い用語であることに異論はないが職域上、言葉は丁寧に扱いたいものだ
オレ個人としてはPull RequestよりMerge Requestの方がProductに入れてくださいって懇願してる感があって好きだけどね(笑)
本題に対するツッコミとしてはコードレビューは文字通りコードのレビューしかしないというスタンスでしかないのでまあチーム内合意と要求されるクオリティ水準が達せられているならまあいいのではないかと思うが,なんというかあまり大声で言うようなことでもない気がするんだよなぁ...ほら、いざ障害とかクリティカルバグが起きた時に面目立たないっしょ
Let's comment your feelings that are more than good