コードレビューの方法 前編 コードレビューの機能
ピクセルグリッドでは、受託案件のコードは必ずプロジェクト内でのレビューを経て積み重ねられていきます。レビューを行う、レビューをしてもらう、それぞれの立場から、ピクセルグリッドのコードレビューのエッセンスをお伝えする座談会です。
はじめに
この座談会は、初めはコードレビューをするときの視点やポイントを聞き、どうやってきちんとした「売り物のコード」を生産しているのかを語ってもらおうと企画されていました。
ところが、実際話してもらうと、単にコードの質を上げるだけではなく、もっと大きな役割が見え始めました。前後編、2回でお届けします。
参加者は全員ピクセルグリッドのフロントエンド・エンジニアで、以下のメンバーです。
- 山田 順久：長期プロジェクトの2代目メインレビュアーを務める。ピクセルグリッドでもレビュアーといえば、真っ先に名前が挙がる。
- 坂巻 翔大郎：主にHTML、CSSを書くことが多いため、今回の座談会では、マークアップ・エンジニアの視点からも語ってもらう。
- 宇野 陽太：Angularをメインとした案件など、比較的稼働人数が多いプロジェクトのレビューを担当した経験を持つ。
- 今冨 悠一：2019年10月、ピクセルグリッド入社。ピクセルグリッドという新しい文化にこれまでの経験をなじませている最中。
レビューは啓蒙的に
――今日はコードレビューすることについて、お話したいと思います。レビューする内容についてはいろいろあると思いますが*、今回はレビューする意味や、その具体的な方法をお話いただければと思います。
*注：コードスタイル
「コードスタイルを統一すること」が目的のレビューについては、次の座談会で以前取り上げています。
――ピクセルグリッドではコードの管理はGitHubを利用しているので、レビューをやり取りする場所はGitHubですよね。
宇野：そうです。レビューのよくある一連の流れというのは、コード書く、Pull Requestを出す、メンションをレビュアーに飛ばす、レビュアーがレビューする、というものですよね。自分はあんまりメインレビュアーでがっつりやってはいなかったんです。ゆっきー（山田）は、引き継いでというかたちだけど、案件でメインレビュアーやっていましたよね。
山田：案件のフェーズが、中期から後期に入ったところで、メインレビュアーを引き継いだってかんじですね。前任者がいたときは、その人がリードエンジニアでメインレビュアーもやっていたんですよね。それで、その人とみんなとで設計を議論してまとめて、これでいこうって固めて。そのあと、レビューでもそれに沿って、リードする人の目線で教育的なレビューをされていたかな。
僕はそれを引き継いだから、同じように教わって維持していくというレビュアーの立場でしたね。
宇野：教育というか、啓蒙？
山田：うん、そうですね、啓蒙。
――啓蒙的レビュー？ どんなことですか？
宇野：そうですね……たとえば、ある人のコードのなかであんまり書いてほしくないコードがあったとします。他の人たちは基本的には、前例を見ながら実装していくので、あんまり書いてほしくないコードだったとしても「そういう前例があるならこれは使っていいんだろう」って思ってしまいますよね。レビュー通っているのだから。
そこで、あとから見て、「この処理ダメじゃない？」「書き方よくないよね」というようなレビューをして、「じゃあ一斉にコードを直していきましょう」となっていく。
プロジェクト前期から中期ににかけてそういうことはわりと発生して、そこで安定させていくのですよね、コードを。「じゃあこの書き方は非推奨にします、次からはこう書いてください」っていうのを啓蒙していくってかんじですね。
山田：構造的なダメ出しをいとわず、モジュールとかレイヤーをまるっと書き換えになる指摘もいとわないかんじですね。
――それって、指摘するほうもけっこう覚悟がいりませんか？
山田：それは覚悟を持ってやるんじゃないですか。
宇野：ただ、実装しているほうも、いろんな人のコードを見て、いろんな人が苦労しているのがわかるから。多分この設計はダメなんだろうなということが実感としてわかってくる。だから、全体を俯瞰する人としてメインレビュアーを立てておくわけです。
その人がリファクタリングとかを中心に作業をしていくというのが理想だけど、でもだいたいリソースは足りないから（笑）、そういう人に機能開発の作業も振られたりして負荷が高くなる、という欠点はありますね。
レビューやりつつ、リファクタリングしつつ、自分の担当範囲もやらなきゃいけないという感じになってくる。
山田：さっき言っていた僕がメインレビュアーになっている案件は、最近はそういった前例とかグッドプラクティスが出尽くしたし、メンバーの練度も高まったので、レビュアーとしてあんまり指摘することはないです。
設計方針の共有と維持がポイント
宇野：プロジェクトの前期と中期と後期で、レビューの内容って変わりますかね？ 前期は、アーキテクチャもしっかり固まっていない場合が多いから、移り変わりが激しいとは思うんですけど。後期はそれがだんだん落着いていくけど、「このアーキテクチャでこの先スケールできるのだろうか」とかいう視点があるのかな？
山田：うーん、それはもちろんそうだけど。ただ、レビューの内容自体は「前期だからこう、後期だからこう」はないかも。
宇野：判断量はプロジェクト前期のほうが多いと思うけど、基準としてはそこまで変わることはないということかな。
山田：そうですね。プロジェクトの段階や、レビュアーとレビュイーの関係によってレビューの目的はさまざまだとは思うんですが、一番大きな理由としてはプロジェクトにおける設計方針を共有して、お互いに維持するためになると僕は考えてます。
その意図でのレビューは、このモジュールがあのモジュールに依存するのはおかしいとか、この処理はこのモジュールに書いてはいけないとか、構造上の問題の指摘になるんじゃないですかね。
今冨：私の場合は、まだピクセルグリッドの入社して日が浅いので、和を乱していないか、足並みがそろっていないところはないか、というところでレビューを出すかんじもありますね。
――とみーさん（今冨）は途中からプロジェクトに入りましたよね。
山田：さっき「啓蒙」という言葉が出たけど、そういう意図では、レビューは途中参加したメンバーの「学習」のためという側面もあると思います。
僕自身はその案件に途中でヘルプで入って、コードリーディングをしたり、僕がレビューする側になるということを通して、このプロジェクトのコードがどうなっているかを学びました。このときのレビューは僕から問題を指摘するというよりは、コードリーディングの一貫であったり、「これなにしてるんですか？」っていう途中参加者の立場からの疑問を出すためのレビューだったように思いますね。
レビューを通じて設計方針が各人に共有されて、コードにもそれが反映されて統一性のある状態が保たれることで、読みやすく直しやすく拡張しやすいコードになったんじゃないかな。
メインレビュアーを固定する
――先ほど、メインレビュアーのお話が出ていましたけど、案件においてレビュアーは決まっているものなのでしょうか。
宇野：これまで自分が関わっていたプロジェクトだと、たいていメインレビュアーを決めて、その人にお願いするパターンですね。メインレビュアー自身のレビューはサブを決めておいて、その人にお願いしていました。
それとは違ってランダムにレビューする人を決めるというやり方もあるけど、それは、自分のやっていたプロジェクトにはあんまり合わなくて。というのは、人によって担当している箇所は違うので、いきなりレビュー担当になっても、そのコードを書くに至った前提を共有できていなくて、レビューするのが難しい。
だったら、全体を把握する人を一人立てて、その人がレビューする。つまり、プロジェクト全体のコードの設計方針を、その人に一任するというほうが進めやすかったですね。
――レビュアーは、全体を把握しているということですか。
宇野：そうなりますね。仕様の詳細を把握しているというよりは、俯瞰してみているという感じですかね。
山田：うん、詳細は知らなくても、構造を知っているという感じかな。
宇野：受託のプロジェクトって、その機能を担当しているエンジニアとディレクターと、その先のお客さんがいるんですけど、そこで細かい仕様はやりとりしているわけで。でもそのことはレビュアーは熟知していなくてよいんです。「その機能のためのコードをプロジェクトに組み込んだときに、全体の整合性が取れているかどうか」ということを見ていますね。
たとえば、整合性が取れないコードが入っていたという場合、コードを書いた人にレビュアーが理由を聞きます。で、たいていの場合は「仕様がこうなっているからです」という返答だと思います。だったら「その仕様のことをコメントに残しておいてください」とレビューでお願いしておく。
コードレビューのポイントとなる観点
- 全体の整合性が取れているかどうか（アーキテクチャ的観点）
- コードから仕様が読み取れるかどうか（コードリーディング的観点）
コードから仕様が読み取れれば、コードを熟知していなくても大丈夫。コードから読み取れないのであれば、修正、あるいは、案件都合をコメントに残しておくように依頼する。
山田：そうですね。コードから推測不能な「案件の都合」は、書いておかないとわからないです。
――レビュアーが仕様を知りすぎないほうがいいのでしょうか？ 仕様の都合なども「書いておいてください」と指摘できますし。
宇野：そういう部分はありますけど、でもまったく知らなくてよいわけではないですね。レビューのために「あえて仕様を調べない」となってしまったら、それは違うと思うので。ただ、重点の置き方として、仕様を知るよりコード全体がどうなっているか把握することが重要かなと。
レビュアーをランダムに立てる
――なるほど。レビュアーをランダムに立てるという方法もあると言っていましたよね。その場合、全員が全体的な仕様を把握している可能性が高いのでしょうか。
宇野：そうですね。理想的なのは、だれもが仕様を把握していて、設計方針もすべて把握しているという状況ですかね。ただ、チームメンバーのスキルとか能力的なことの足並みが揃っていないとむずかしいかな、というかんじがあります。
今冨：僕はピクセルグリッドに入社する前は、レビュアーはけっこうランダムに担当するのが多かったです。実は、自分もそれが好きで。
――どんな利点があるんでしょうか？
今冨：開発が始まった場合は、一人が一機能作ったほうが効率的で速いと思います。でも、そうするとその機能のことがその人しかわからない。なので、ある程度、開発が落ち着いてくると、人に依存する部分を減らしていきましょう、担当以外のコードのこともよく知りましょうという意味合いで、レビューをランダムにだれかに当てて出していました。だれが抜けても困らないように、こんなの書いていますよというのを共有することも兼ねて、レビューを出すことが多かったです。
そうすると、毎回見なくても、なんとなく点だけでもそこを押さえておくことができる。ここはこういう意図で書いているんだということがわかりますよね。
宇野：そういう利点はありますよね。逆に、メインレビュアーを立てることの一番の弱点は、その属人化だと思います。属人化してしまうその分、意思決定は早いんですけどね。
――どちらの方法にも、メリット、デメリットがありますね。
宇野：その解決方法になるかはわからないですけど、以前、参加したプロジェクトで、「コード共有会」っていうミーティングを毎週30分ぐらいやっていたことがありました。プロジェクトに関わっているエンジニアが、各人が持ち回りで「自分はこういうコードを書きました」というのを共有する会を、レビューとは別に設けたんですね。みんながレビューしなくても、みんなが、ほかのコードがどう書かれているかを知っている、という状態を作るようにしたんです。
わりとそれはうまく回っていました。その場で、ここのコードはもっとこういう書き方してもいいんじゃない？ という意見も出したりして、みんなでレビューする会みたいなものに近かったですね。
今冨：そういうのあるといいですよね。人がコード見るときの目の付け所を知ることもできる。
宇野：やっぱり一人で書いていると不安なんですよね。「このコードの路線は正しいのだろうか？」とか「もっといい書き方ありそうなんだけど思いつかないな」とか。そういう不安感はあります。レビューとか、このコード共有会みたいなのがないと、なかなか相談できないので。
HTML/CSSのセルフレビューはコード意図を共有するために
――今回は普段HTMLやCSSを担当することが多いエンジニアにも参加してもらっているんですが、HTML/CSSをレビューするときは、JSと目的が違うんでしょうか？
坂巻：案件では、HTML/CSSを担当する人って、たいてい自分ひとりだから、レビューすること／されることがない（笑）。ただ、僕が自分のコードのレビュー、つまりセルフレビューすることはあります。レビューコメントをいろいろ書きます。
そんなレビューをどうしてやるかというと、それは他の人に「ここはこうなんだよ」と伝えるために書いていますね。コードのコメントは書かない代わりに、レビューのコメントとして自分の意図を書きます。「ここはこういう意図で書いているので、あとはよろしくお願いします」というような意味合いかな。
CSSって、JSと違ってコードだけ読んでもわからないんですよね。JSはコード読めばわかると思うんですけど。
宇野：CSSは宣言しているだけだからね。
坂巻：そうそう。宣言の塊なので、それがなんでそうなっているかは、コード中のコメントに残すか、さっき言ったようにレビューのコメントとして申し送りをしなくちゃいけない。
「このdivに、このclassをつけてください」という、1つだけならいいです。でも、「このdivにこのclassをつけるときは、こっちにもあっちにもつけておいてください」というお願いをしなくちゃいけないときは、書かないとわからないんです。そういうのって、相手がどれだけ空気を読むかにかかってくるので。
宇野：CSSの場合で言えば、クラス名である程度の用途は把握できるんですよ。でも、なんとなくどこにつけたらいいかはわかるんですけど、それとマークアップの構造ってまた別で。それをマークアップのどこにつけなきゃいけないかというのは、ほんとに空気を読んでいくしかない。ぽちぽち試しにやってみて、UIのどこが変わるんだろう？ みたいな。
坂巻：そう。だから、そういうのは、申し送りにするようにしています。「ここは要素がなくなってほしくないんで、ngIf*を使わないでください」とか、そういうのは書かないとわからない。要素が消えるのか、要素ごとまるごとなくなるのか、言わないとわからない。
行コメントで書けることでもあるんですけど、あんまりたくさん書くことがあったり、議論したほうがいいことだったりしたら、GitHub側でやったほうがいいかもしれない。
*注：ngIf
ngIfとは、Angularにおいて構造ディレクティブと呼ばれるものの一つで、条件に応じてDOMの追加または削除を行います。
HTML/CSSのレビューチェックポイントは？
――具体的に、どういうところに気をつける、ということはありますか。
坂巻：アクセシブルかどうかは見るよね。<div>にonclickって書いてあったら怒るぞーって（笑）。
宇野：ごめん、この前やったね……（笑）。
小山田：hrefにjavascript:void(0)って書いてあるとかね。
坂巻：ああ、あるねぇ。
小山田：あとは名前の付け方とかかな。「ここgazoってなっているけど、画像以外も入るよね？」みたいな。それだったら「もうちょっと広くmediaって名前にしたらいいんじゃないかな」とか。（編集部より：座談会収録中にちょっと参加してくれました）
坂巻：あと、動いているものを見ないとわからないということはありますよね。コードだけ見てもわからないので、実際にCSSが適用されているページを見て、ということもあります。たとえば、ブラウザウィンドウを狭めたときにどうなるかとか。
宇野：コードだけを見て、「これはなんのためにつけているんだろう？」というところがあったら、実際に動いているところを見に行って、「なるほど、こういうことね」っていう理解が必要になる。
坂巻：そういうのをコメントに残しておいたりとかも、必要かもしれない。
小山田：Pull Requestに、ブラウザ幅を狭めたらどうなるかのアニメーションGIFや動画を貼り付けてくれると助かる。Pull Requestを作る人の思いやりとして。
坂巻：そういうのはいいよね。僕は、HTMLだと、適切な要素が使われているか見るかな。CSSだと、クラス名が適切かどうかとかは見てるけど、それ以外は機械的にLintで弾けるものが多いですね。!important使うなー！とか。詳細度を低めにしろー、とか。
――あらかじめバッドなパターンとかあるんですか。
宇野：全称セレクタにposition: relative入っているとか。
坂巻：ありうるよね、そういうのもね。まあ、そんなことしないけども。全称セレクタが使われると僕は指摘しがち。「なんでここ全称セレクタなの？ 本当に必要なの？」みたいな。
宇野：何度も書くのがめんどうくさいからという理由だけで、雑に全称セレクタに入れているとか。
坂巻：それはありえますね。「アウトライン、消す……の？」みたいな。アウトラインつかない要素があった場合に、「なんで？ 本当に用意しているの？ 全部用意しているの？」って思っちゃう（笑）。全部用意していて、フォーカスしたことがわかるならなにも言わないけど。
――見る人の力量によるところも大きそうですね。
坂巻：よるんじゃないんですか。アクセシブルにするかどうかも、仕様になければそこに興味がない人は見ないし。
宇野：レビューする場合も、コストをどこまでかけるかということもあって。やりすぎてもよくないですよね。理想を追い求めていくのはいいけど、そこでかけたコストを、保守効率という面で回収できるのか。
まあ、気にはなるけど、今のところ大きな影響がないからスルーという場面もあるんじゃないかな。あとは、さっきも話に出たけど、プロジェクトの最初のほうですね。最初のほうは、今後どうなるかわからないからまだ手が出せない。なので、なかなかこれって決めて進めていくのはしんどい部分がある。
（後編に続く） （構成：編集、丸山陽子）