はじめに
この半年くらいで初めて本格的にチーム開発を行い、今では日常的にプルリクエストというものを使っています。
チームの方々には、基本的なことから応用的な部分まで様々な観点からレビューをしてもらって、大いに勉強になりました。
ただ、時には「新人にとっては厳しいレビュー」をいただき、致命傷で済んだものもありました。
もちろんそれは悪意のあるものではなくて、新人とレビュワーのスキルのギャップによって意図せず生み出されてしまうものです。
そのような不幸なレビューによって苦しむ新人が減ることを願って、新人を殺してしまう恐れのあるレビューをまとめていきたいと思います。
新人教育の場に少しでも役に立てていただけると嬉しいです。
前提条件
今回の対象とする「新人」は、本格的な開発経験が1年未満の方を想定しています。
個人で少しプログラミングはしてきたけれど、チーム開発は未経験の新卒や、インターン生、プログラミングスクールでスキルを身につけてきた第二新卒の方がイメージです。
※ レビューの受け取り方には個人差があります。
※ 今回のレビューの例は全てフィクションです。
※ 所属組織に一切関係はありません。
※ 悪用は絶対にしないでください。
致命傷のレビュー
以下のレビューを積極的に使うと、簡単に殺せると思います。
公式ドキュメントで殴る
個人的に、一番辛いレビューがこれです。致命傷です。
公式ドキュメントに「コントローラ」について詳しく載っているので、それを参考に修正してみてください。
https://readouble.com/laravel/5.7/ja/controllers.html
レビューの内容は正しいし、レビュワーにとっては当たり前ですぐに理解できることなのだと思いますが、新人にとっては激しく辛いです。
あくまでも公式ドキュメントは基本的な使い方が記載されている場所であって、それを応用して書くということがかなり難しかったりします。
また、フレームワークの全体像がつかめていないのに、ドキュメントの1点を読んでも何がなんだかわかりません。
しかし、真面目な新人の場合は、「自分がこれを読み込んで頑張れば良いだけ！」と抱え込んでしまい、それで上手くいけば良いのですが、多くの場合は何も解決できなくて途方もない時間を溶かしてしまいます。
可能な限り公式ドキュメントで殴るのはやめてください。
改善例
公式ドキュメントに「コントローラ」について詳しく載っているので、あとで一緒に読みながら修正していきましょう。
https://readouble.com/laravel/5.7/ja/controllers.html
公式ドキュメントを読む習慣を作ることは非常に良いので、最初のうちはペアプログラミング（ペアリーディング）をしながら、「公式ドキュメントの読み方」を育てていけると良いと思います。
無意識に煽る
新人をけっこう落ち込ませるのに効果的です。事故の場合もあるので注意です。
こういう書き方があるのは知りませんでした。どこかに参考になるものはありますか？
<a link="example.co.jp" target="_blank">
まあこの例は明らかに <a href="example.co.jp" target="_blank"> が正しいのですが、レビュワーがミスに気づかせるためにこういう書き方をしたのだと思います。
が、新人としてはなんとなく煽られている感じがして気持ちが良いものではないので、素直にミスを指摘してくれたほうが良いのでは？
こういうレビューが続くと、新人が失敗を極度に恐れるようになることもあるので、「新人に自分で気づかせる」という意識もほどほどに。
改善例
aタグはlink属性を持たないので、
<a href="example.co.jp" target="_blank">が正しいです。
aタグの他の属性についてはここにまとまっているので、書くときは参考にしてみてください。
https://developer.mozilla.org/ja/docs/Web/HTML/Element/a
ミスを的確に指摘してもらいつつ、再発を防止するための方法を教えてもらえると、新人の励みになります。
「新人にミスを気づかせる」よりも、「新人に同じミスをさせないようにする」というレビューのほうが良いと思っています。
ちゃんと確認した？って確認する
確認していても、「すみません確認不足でした」としか言えなくなる奥義です。
この実装については、ちゃんと確認しましたか？もう少し考える余地がありそうです。
const xyz = Object.assign(xy, {z: 3})
新人としても、プルリクエストを出すからには可能な限りの確認をしているはずです。
ノーヒントで「確認した？」とだけ聞くと、調べる時間だけが経過して、なかなか答えにたどり着けないこともあります。
同様に、直接聞きに行った時に「ちゃんとググった？」って聞かれるのもキツイですね。
そもそも何を確認すれば良いかわからない、何をググれば良いのかわからない状態なときもあるので。
ペアプロなどを通じて、レビューには出てこないような思考の欠陥を知ることも大切だと思います。
改善例
Object.assignは破壊的関数なので、この後に元々のxyの値を使うことができなくなります。
参考：https://qiita.com/yujilow1220/items/978b811a9910a9516bb4
ES2018で追加されたスプレッド演算子で結合すると良いと思います。
const xyz = {...xy, z: 3}
実装は動いているけれど、コードの品質に問題がある場合は特に確認が難しいです。
まずは、新人の今の実力を認めて、その成長を促進させるようなレビューができると良いでしょう。
かすり傷のレビュー
そこまで大きな傷にはなりませんが、ジャブのようにじわじわとダメージを与えることができます。
プルリクエストの範囲を広げる
プルリクエストをテンポよくマージしていけると嬉しいものですが、レビューで修正内容を広げられるとけっこう困ったりします。
商品一覧ページのこの部分と共通化できると思うので、一緒に修正してもらえますか？
一気に修正したい気持ちもわかりますが、品質への影響がない場合には、issueなどにまとめておいて、別のプルリクエストで修正しても良いと思います。
1つのプルリクエストに対して何日もレビュー＆修正を繰り返していると、精神的に辛くなってきませんか？
プルリクエストを小さくし、その分マージを多くして成功体験を積み重ねられると、新人のモチベーションも上がっていくでしょう。
改善例
商品一覧ページのこの部分と共通化できると思うのでissueにまとめておきました。これがマージされたらそちらもお願いできますか？
プルリクエストの回転数を上げて達成感を得られるような状況も作っていきましょう。
場合によっては、プラスアルファの部分はメンターのほうで担当して、コードの書き方のお手本を見せたり、その部分のレビューを新人にお願いするなどして、多角的に作業を進めるのも良さそうです。
わからない単語を並べる
プログラマ独特の言い回しが理解できない時もあります。なるべく平易な言葉を使うことをおすすめします。
コードの品質を担保するために、マジックナンバーは定数に切ってください。また、このファイルは抽象度が高いので、もう一段階層を掘ってください。
まあ、何を言っているのかわかりそうでわからないです。
こういうレビューは、なんとなくわかった気になって進めることもできますが、修正して出したら間違っていた、ということもよくあります。
シャレオツでナウいレビューも良いのですが、わかりやすい表現を使ったほうが結果的にお互いが助かります。
改善例
変更があった場合に対応しやすいように、数字を直接入力するマジックナンバーは避けて、定数を設定して使うようにしてください。また、このファイルは特定の用途で使われるので、"profile"ディレクトリのようなものを作って、そのなかに置いてください。
まあ、ちょっとレビューは長くなりますが、レビューと修正が何往復もするよりも健全でしょう。
マジックナンバーなどの言葉は、初回は意味の説明と同時に記載すると伝わりやすいです。
事前に会話可能であれば、一度言葉の説明をしてから以後使うようにしても良いかもしれません。
レビュー以前の問題
こんなにレビューに気を遣う必要があるのか？と思った方も多いかと。
もちろん、レビュー以前にエンジニアどうしのコミュニケーションが取れていて、新人の心理的安全性が保たれていればなんの問題もありません。
レビューで多少は苦しんだとしても、直接コミュニケーションを取ることで精神的に追い込まれることは無いでしょう。
ただ、新人との挨拶もそこそこに、文字でのコミュニケーションが主体となる職場もあるはず。
そうなった場合、新人は職場の人々の雰囲気もつかめず、慣れない環境の中で、新しい技術に向き合っていかなければならないので、なかなか苦しい戦いが待っています。
「新人には大変な思いをして学んでもらって、そこから成長してほしい。」
そう思う方もいるかもしれませんが、そんなことをして新人に辞められてしまっては一大事です。
結局は、ベテランエンジニアが人手不足で苦しむことになってしまいます。
新人をレビューで殺さないでください。
新人教育における不幸が少しでも減ることを願っています。
追記
予想以上に多くの方に記事を読んでいただけて、大変嬉しく思います。
ただ、誤解を招いて欲しくないので追記をさせてください。
私自身は、現在弊社で丁寧な指導のもと、充実した日々を送っています。
十分な心理的安全性が保たれていて、わからないレビューに対しても気軽に口頭で聞きに行ける環境を作っていただいています。
もし、こういった環境が無いままに上記のようなレビューを受けたら大変だろうな、という想定のもとで記事を書きました。
ですので弊社に誤解のないよう、よろしくお願いいたします。