レビューの認識をすり合わせて効率アップ
エンジニア転職してチームに配属されたらレビューを受ける機会が多くなります。気持ちよくレビュー活動ができていないとレビュワーとレビューされる側の相性によっては精神的な疲労になります。私も副業をしてたりちょくちょくいろんなところでお手伝いさせてもらってて、レビューがゆるいところと厳しいところをともに経験してきたのでその上で今の自分の立ち位置を共有していこうと思います。
レビューってなにって人もいると思いますのでそちらから解説していきます。
興味ない人は飛ばしてもろて。
よくある問題点
レビューの受け取り方って案外難しいのですよ。レビューされる側は素直に受け取るだけでもだめですし、自分の成果物にフィードバックを受けるわけなのですくなからず精神的な抵抗も生じますし。。。ちなみに、レビューは何をするためのものかについてはほとんどの会社やチームで何も説明されません。そんな中生じる摩擦によって開発サイクルが遅くなっていくことがあります。
レビューとはなにか？
まずはレビューの全体像を掴むためにこちらの図を参照ください。
登場人物は実装者とレビュー者です。
流れとしては、実装者が作ったものをレビュー者（一般的にはそのシステムに詳しい人）が受け取って、
- 誰が見てもわかりやすいコードになっているのか
- バグを起こしそうな書き方になっていないか
などをチェックします。
-
修正してほしい箇所があれば、修正されたコードにコメントを付けて実装者に戻します。レビュー者が修正することはありません。
-
コメントを受け取った実装者は、内容を確認して修正する必要があればソースコードを更新して再度レビュー者に確認依頼を出します。
-
レビュー者は修正後の内容を確認してOKであれば、ソースコードレビューがOKのログを残して一連の流れは完了です。
コードの質を高めて次の改修をしやすくするためにほとんどの自社開発の企業はレビューフローが存在します。
ここで問題になりがちなのがレビューがOKっていうのが人によることが多いということです。
レビューがOKになっている状態はどういう状態？
レビューの流れはわかったけど、
- レビューって何を見てるの？
- どうしたらレビューOKになるの？
ってなる人が多いのではないでしょうか？その開発チームに入ったらベテランであれ新卒であれ最初はそのチームのレビューがどんなものなのかの把握を始めると思います。
ちなみに私が考えるレビューとは、実装者が上げてきたソースコードを他の人の視点から見て良さそうかどうかを見るものだと思っています。動作を保証することでもなければ、バグがないことを担保することでもないと思っています。
え、よさそうかどうか、、、ってすごく曖昧じゃない？そんなものでいいの？と思うかもしれません。
そうなんです。すごく曖昧なんです！本質的にはレビュワーの目からみて良さそうに見えたらそれでいいのです。
ここで人にも組織にも色んなタイプがあるので問題が浮き彫りになります。
× バグがないかをレビューで担保する人
レビューを丁寧にやってもらえるのは、非常にありがたいのですがこのパターンでは、次の観点でもったいないが生じます。
まず本質的にはバグがないのかどうかは、テストで確認するでしょう。とはいうものの、レビューもテスト活動（品質保証）の一環ではあると思っていますし、私自身も本当に些細な変更で目視の確認をすることが十分なてすとになりそうだなというときは、それですすめることもあります。
本題に戻るとテストで確認するのに、レビューでも確認するというのはまったくもってもったいないです。それがユニットテストであれ、結合テストであれ、自動化されているされてないに関わらずテストフェーズがあるのであれば、
テストでやることをわざわざレビューで見てるというのは本当にもったいないです。今すぐやめましょう、もしくはレビューをお願いしている立場であれば
*「ここは、テストで書くので軽く見てください」*というようにコメントを書くのがいいと思います。
注意点
- 一つだけ制約が生まれるのは、実装のレビューとテストのレビューを同じ人がしないと、そのコメントで書いた約束が果たされるかどうか保証できないのでそこだけ
× レビューを通して新人を成長させようという人
レビューは教育の場でもあるという人がいるのですが、個人的には同時に2つのことをするとだいたいどこかで歪が出たりトレードオフになりがちななので反対です。
具体的には、レビューを教育の場にすることは次の観点で反対です。
- 機能開発のスケジュールにコミットしない
- レビューの品質のゴールの基準がブレる
この機能は来週まで出さないといけないという状況にあったとします。実装側からするとタスクは山積みなのにという場合です。そういった文脈を無視して、成長のために自分で考えてみてというやり方が非常に嫌いです。どういうときにその状況が起きやすいかというと、同じプロジェクトの内部で完結しないので他のプロジェクトの人にレビューをお願いしてこういう状況が起きます。特にそのチームへのjoinしてからの期間が短いときに。
なので、すでにレビュー者がある程度の答えを持っているのなら、そのやり方をsuggestしてあげるのがいいでしょう。もし教育の場にするのであればどの程度までかを決めておく必要があると思います。
私の場合は、より良いやり方をsuggestした上で次にコード品質がギリギリOKになるくらいまでの根幹にある考え方が理解できると言うまでがレビューでもやって良い教育だと思います。というのも、何回も同じところを指摘するのはそれは効率が悪いからです。
絶対だめ
- 知っている情報をあえて伏せてまで成長させようとする
× コードのスタイル(インデントはスペースいくつ分なのかなども目視でレビューする人もいたりします。。。)
コレ系のレビューは最近はgithub actionやそれ以外にもcodebuild, circleciなどがあるのでだいぶなくなってきたと思うのですが、スタイルの指摘をするくらいならガイドラインをホント決めたほうがトータルでコスパ良いです。
あとは、if文であろうと三項演算子であろうと、3秒以内に理解できるのであれば正直どっちであってもいいと思っています。
「ここは三項演算子のほうが見やすいです」ってそれgolangで実装してて言えるの？
ただそのチームでいろんなプログラミング言語を使っていてその言語独自すぎる実装はちょっとやめてほしいと思います。ただ、そんな場合もユニットテストがかかれてたら特に気にしません！
レビューとテストについて
ちらっとレビューとテストに関してどちらも品質を保証するものだというふうにお話しました。その違いがなにかについても少しお話したいです。
実際に作ってものが売られる（サービスを提供する）までにはクリアしなければいけない品質基準というものがあります。あると思っています。ここではあえて勝手に言葉を作らせてください。
- 当たり前品質
- 継続価値提供のための品質
当たり前品質
品質を満たしていないものを世の中に出すことで、その製品に欠陥があった場合に謝罪やお詫びをしなければいけません。それはWEBの世界でも同じです。
例えば、ユーザーはサイト上でAという価格でBと、Cという機能を享受できる前提でお金を払ったとします。それが、不具合を起こしてBという機能が提供できていませんでした。
こんな状態では、ユーザーからはせっかくお金を払ったのにという気持ちになります。サービス提供側からもユーザーに何かしら謝罪を行わなければいけません。
テストはどちらかというと上で記載したような事が起きるのを防ぐために実施します。言い換えたら挙動に対して保証をするようなものだと思います。ただ、テストをしたからと言って保証されているのは、あくまでテストした挙動だけですので。そのへんに関しては詳しくはJSTQBの資料に書かれていたと思います。
継続的価値提供のための品質
実際に車や商品を売るのと異なるのは、継続的にユーザーにサービスを提供し続けることです。その観点で保証するべき品質というのは、継続的に改善しやすいコードを書かなければいけないということです。
サービスを提供する上では、ユーザーが本当に欲しい物をはじめから提供できるわけではありません。いくらユーザーにインタビューしようとも、それを形にする段階で間違えることもあります。ときに小規模な修正で改善できることもありますが、根本的に方針を変更するときもあります。
サービスを提供するためのシステムのコードはどんなに少なくとも数千~数万行に及びます。そんなときに該当箇所を探すのに、どこに書いてあるのかや色んな所に不具合を起こしうるコードがおおいと、調査だけで数日から数週間の調査が必要になります。これはホントです。
その品質を担保するのは、ヒトしかできないので継続的な価値提供ができるコードを書いているのかその部分をヒトが担保する役割を担います。この部分はレビューで担保する側面も結構あると思います。もちろんテストコードを書くことで安心してリファクタリングや機能改善が実践できる側面もあるので、テストとレビューで重複する内容を保証しているのは間違いないです。
未経験エンジニア/新卒エンジニアには、レビュワーは神にみえる？
未経験エンジニアや他の職種から転職してきたエンジニアの人は特に、自分のソースコードやテストに対してレビューを受けるときに、レビュワーの言うことが絶対だと勘違いする方もいると思いますが、そんな事はありません。
レビュワーが間違っているケースというのも大いにあります。そして間違っているケースというのも2種類に分類されます。
- レビュワーが実装者の意図を汲み取れない場合（実装者のほうがその領域について詳しい）
- レビュワーがレビューの責任の範囲を逸脱してレビューをしている場合
レビュワーが実装者の意図を汲み取れてないというのは、実装者が事前の説明が何もなされてないために生じるものなのでこちらは実装者の怠慢であることが多いです。レビュワーがレビューして無駄に指摘されることを避けるために、Zoomやコメントをかいてあげることでレビュワーにも快適にレビューをさせてあげましょう。
そういったやり取りを気軽に作るためにもレビューに渾身の一撃を出すのではなくて、テストで担保するところはテストで担保すると割り切ってサクッとレビューしましょう！ というのも、レビューに対して苦手意識を持たれて業務が滞ったりチームにマイナスのイメージを持たれる方がよっぽど避けるべきだからです。ちなみにfindyで平均のプルリクエストマージ時間は2時間位とのことなので私も本業でそれくらいの時間でマージを目指してレビューをしています。そうするために、この観点だけは絶対に見るというのを決まってきて、逆に品質レベルが変に上ったり下がったりすることがなくなってきた気がします。
問題となる２つ目の点については散々行ってきてますが
レビュワーがレビューの責任の範囲を逸脱している場合
レビューは、基本目視で行います。そこで、コードのスタイル（インデントや、スペースの入れ方）や実際にちゃんと動くのかという動作保証なども行っている場合があります。
繰り返しになりますが、どこで何を担保するのかについてもう一度復習しておきます。
- なんとなく動いているかなという動作保証はレビュー前に実装者がしておくことです(テストコード書いてもいいですむしろ推奨。復習と書きながら初めてここで出した気がする。)
- バグがないかに関してはテストで担保するところです。
- コードのスタイルはリントによって担保されるべきです。
ここですべてを目視でレビューを行うデメリットについて言語化しておきます。
- レビューコメントの数（工数）が多くなる
- レビューが返ってくるまでの時間がながくなる
- 本来レビューで指摘するべき箇所の指摘が抜ける
- レビュワーの精神/体力の披露がたまる
- 実装者の精神/体力の疲労がたまる
- コミュニケーションコストが増加する
なので、ほうっておくとシステムの改善が全然進みません。そしてサービスの改善も進みません。チームが披露していきます。なので機能開発のサイクルにおいて、レビューのフローが足を引っ張っていると思うのであれば、もう一度チームでレビューのあり方について認識を合わせておく必要があります。
古い体質を変えられないのであれば、レビューの内容に悩んで精神を披露させることなく、無心で修正してください。そういうレビュワーなんだなと思わないとやってられない場合も多くあります。
効率よいレビューをチームで行っていくために
- レビュー者がどんな観点でレビューしているのか確認しましょう
- レビューで動作確認や、細かいスタイルの指摘まではしない。レビュー者が2人いたら、それぞれでブレるものは好みなのでその指摘が無意味なことを伝えるか、最終手段は無心で修正する。
まずはレビューを何回か受けてみて、そのチームやその先輩のレビューの文化について理解しましょう。そのうえで、まずはなんのためのレビューなのか確認してみましょう。
それで、改善提案ができるのであれば改善提案し、改善できない諸々の事情があるのであれば、細かいどっちでもよい修正やスタイルに関する修正で一喜一憂せずに無心で修正しましょう！スタイルの指摘に関しては自動化されてないのが悪いです。自動化に関してはまた別の記事のまとめます。ちなみに自分の環境だけ自動化させることもできるのでそちらに関してはまた別の機会に。
Discussion