プルリクエストのレビューってだるくないっすか？
下手なことを言って自分の無知をさらしたくない。マサカリを投げ返されて酷い目にあう可能性もある。そもそも。私めのような低級コピペエンジニアからしたら、あなた様のコードにケチなんてつけらんないですぅ。ヒギぃ！ みたいな。
冗談めいて書いたけど、レビューをためらうのはこう言う気持ちからだと本当に思う。
根底にあるのは恐怖だ。
過剰な自意識に縛られ、そこから動けないでいる。
僕もそうである。「そうだった」ではない。今もなお「そうである」。プルリクレビューほど嫌なものはない。
しかし、チームで開発している以上、プルリクレビューを行うのは義務である。
レビューこそチーム開発で品質にレバレッジを効かせる手段であり、ここをないがしろにして各々が1人でソースコードを書いてるのだとしたら、それは単なるフリーランスの集まりだ。徒党を組んでる意味がない。
そこで、今回。ヘボヘボDeveloperの僕が強強エンジニアの中でどうにかこうにかプルリクレビューを行うためにやっていることを紹介する。
これを見れば、どんなにヘボヘボであろうと1個ぐらいレビューコメントを残せるようになる。（かもしれない）
では、行く。
目次
自分のプルリクに対してレビューする
まず、レビューをするにしても場数を踏むことが重要である。そのため、まずは自分のリポジトリの自分のプルリクをレビューしてみることをオススメする。
下記は僕のリポジトリのプルリクエストだ。コミッターは実質僕しかいない。
https://github.com/yheihei/tampermonkey/pull/2
タイポからlet、constの指摘まで。なんでもいい。なんせ相手は自分なのだから、多少厳しいことを言っても傷ついたりしない。
できればこれが他者だと思って丁寧な言葉づかいをするほうが良い。そうするといざ他者にレビューするときに無礼な書き方で失礼をこくことがなくなる。
個人のリポジトリがない場合。例えば会社のプロジェクトなどのプルリクでもよい。
他者にレビューを依頼する前に、自分だけでレビューすれば良い。自分のコード品質も上がるし、レビューの場数もふむことができる。
自分のコードの妥協点を指摘する
でもレビューするってどう言う観点ですればいいのさ。ってなると思う。観点が僕は知りたいのさ！ と。
その人に向かって「いいからやってみろよアンポンタン」と返してしまうのは良くない。
その時に役立つのが、妥協した点を探すと言うことだ。
例えばこれは個人用の開発だし、適当なコードでもいいよなって妥協すること、よくある。そこであえて自分の妥協点をビシバシ自分で指摘して行くといよい。
例えばこんな観点だ。
よくある妥協点
タイポ
getColorがgetColerになっていた。こう言うのがあればチャンスだ。なんせタイポと書けばいいのである。簡単だ。
自分のコードならまだしも、人のコードである場合。すげえ忙しい時にタイポなんて指摘して怒られないだろうか。。。と気弱な人は思うかもしれないが、違う。タイポの指摘は最高である。
僕はタイポされたメソッドが数年間使われ続け、関連メソッドもそれに引きずられてタイポしている(せざるを得ない)プロジェクトを頻繁に見る。こういうのはよくある。
後から入ってきた中途がそのタイポを直そうとしたら、膨大な箇所のタイポがgrepに引っかかった。いや、理論的には全てgrepで置換すればいいはずだが。。。今動いているプロダクトに影響が出ないと言い切ることができるか。。。100%影響がないと。言い切れるだろうか。。。まだ入ってきたばっかりで何も知らないし。。。(そっとIDEを閉じる)
そう言う悲しくて間抜けなシーンが、タイポの指摘をすることで防げるのであれば、最高な指摘だと言える。
コメントの不備
自分のコードを見る。これ、人が見て伝わるかな？ って少しでも思ったらチャンスだ。「処理の流れがよくわからない。コメント追加せよ」と書こう。
いったん、よく分からない、と自己表明すると、いろんな部分が気になってくる。すると冗長処理が見つかったり、もっと関数名や変数名で語ろうと思うようになる。いろんな箇所をリファクタリングするチャンスになる。
なので「コメント追加せよ」という指摘はたいへん尊い。
本業でも有効だ。自分が分からないことは、数年後に入ってくる中途も高確率で分からない。コメントがある、それだけで未来の中途はものすごく喜ぶはずだ。
だいたいその中途にはドキュメントなんて高尚なものは残されていない。ソースコードとDBだけ用意された状態で丸投げでやらされるのだ。IT企業と呼ばれている企業の80%ぐらいはそうだ。(経験談)
そんなクライシスな状況下で、親切なコメントが書いてある。これだけで精神的な負担は大きく減る。あなたが勇気を持って「コメント追加して」と言ったことで、将来的に喜ぶ人はめちゃくちゃにいるのだ。積極的にわからないことを表明し、コメントを追加してもらおう。
変数やメソッドの命名
振る舞いが適切でない関数名(getと言っているのにsetしているメソッド。振る舞いに書いてないことをやっているメソッド)、英文的に意味が伝わらない変数名、頭文字で省略された謎の変数名(color_cd。color_codeって書けや)。
こういうのを見つけたら即コメントを書く。
この辺りは下記の書籍が参考になる。
Clean Code アジャイルソフトウェア達人の技 (アスキードワンゴ)
配列操作の冗長性
配列操作の全てをforeachやforでやる流儀の人がいる。
例えば配列の中の特定の要素を取り出すみたいなのを、foreachでやる。これは可読性が良くない。for文の中身を見ないと、振る舞いがわからないからだ。
PHPで言えばarray_filterとか、JavaScriptで言えばfilterとか、繰り返しの処理に対して振る舞いを語る関数が用意されている。そう言うものを使っていくべきだ。
JavaScriptの記事だが、下記の記事がわかりやすい。
for文を見たらそこがレビューのチャンスだったりする。
特定のテストケースでの不備
自分のプルリクを見て、テストをしていなかったら、ちょっと考えてテストを一つだけしてみる。このケースはどうかな？ と。やりすぎない範囲で。
すると特定のテストケースでエラーがでるとか、そういうのが発見できる。
他人のプルリクの場合、ブランチをチェックアウトして、実際にテストしてみるといい。
開発者が確認した以上の内容をモンキー的にやってみて、おかしな箇所が見つかれば報告する。
テストコードが書かれていれば、追加でやるべきテストケースがないかをチェックする。
仕様の理解誤り
僕は個人のプロジェクトでも必ず仕様書を書くようにしている。仕様があるのであれば、仕様に沿っているかを最終確認すると良い。
もし仕様書と違った動作をしたり、仕様書でカバーできていない動作があれば、コメントに残す。その後、仕様書を直すのか、実装を直すのか判断する。
他人のプルリクの場合、設計書や仕様書のリンクが貼ってあれば、その仕様をきちんと満たしているかを確認する。
ただ、担当外の人がこれをやるとめちゃくちゃな時間がかかるので注意が必要だ。個人的には余裕があるときや、仕様が明確なとき、よく知っている機能のときだけやればいいと思う。
ソースコードがよくなったことを実感する(これ大事)
最後に自分のプルリクをマージする前に、ソースコードをみかえす。
すると、あきらかにソースコードが良くなっている。。。と実感するはず。
この感覚が重要で、レビューを経てソースコードがよくなっていくフローを体験していけば、プルリクレビューは開発者をいじめるものではないことが肌感で分かるようになる。
すると、他人に対しても気軽にレビューができるようになっていく。
まとめ
プルリクレビューができない/苦手っていう人は
- 自分のプルリクを自分でレビューしよう
- 一見くだらないと思うことでもレビューコメントを書いてみよう。自分自身であれば何を言ってもバカにされないぞ
- 指摘に対応したあと、自分のコードがよくなっていることを実感しよう
これを繰り返していくと、他人のプルリクをレビューできるようになると思う。がんばっていこうぜ。(俺も)