エンジニアとして現場に出ると、
ほぼ100%、コードレビューというものに遭遇します。
もう当たり前すぎて、
あまりコードレビューについて考えることは少ないかもしれません。
本記事では、そんなコードレビューについて、
今一度初心に帰って、本質や目的、やり方などについて、
考察を深めてみようと思います。
コードレビューのやり方などは、
正解があるわけではないので、
あくまで私が考えるベストなやり方について、
本記事で紹介します。
またそのやり方についても、
技術的なやり方というよりは、
こちらの記事にあるようなメンタル・マインド面で重要なことに
フォーカスしています。
ちなみに、コードレビューに関連して、
強強エンジニアとは何か？と考察した記事もあるので、
興味がある方は、こちらもご覧ください。
コードレビューとは？
コードレビューとは、
自分が書いたソースコードをプルリクエストとして、
出した際に、ベースブランチにmergeする前に、
他のエンジニアによる第三者チェックを挟むことで、
まずいコードや冗長なコードなどを議論するようなものです。
まあ、平たく言うと、
第三者チェックとして、
他のエンジニアに自分の書いたコードを見てもらうことですね。
こういったレビュー自体は、
ソースコードに限らず、例えばドキュメントレビューとか、
文面レビューとか、色々あると思うので、
コードレビューは、それのソースコード版というだけなので、
概念自体はシンプルですかね。
コンサルとかだとパワポレビューというものもあるらしいです。
基本的にソフトウェア開発はチーム開発なので、
このようなコードレビューは、一般的には行われるのが普通ですね。
個人開発とかだと、コードレビューはあまりないかもしれませんね。
コードレビューは怖いもの？
さて、そんなコードレビューですが、
しばしば怖いものとして取り上げられることもあります。
というのも、このコードレビューの際に、
「こんなコード書いて恥ずかしくないの？」というような
攻撃的なコメントをレビューワーがしてしまったり、
あまりに細かいコメントとかをしすぎたりして、
実装者が萎縮してしまう現象があるのが、
背景にあるのかと思います。
まず、大前提としてコードレビュー自体は、
怖いものではありません。
言ってしまえば、コードレビューが怖いと思われるのは、
レビューワーのレビューの仕方が良くないのかなと個人的には思います。
そのため、コードレビューが怖いものとして
現場で認識されるようになってしまった場合、
もうその現場でのコードレビュー文化は、
実質的には失敗していると個人的には考えます。
ただ、ともすると、
スキルが高い人がもてはやされるのが、
良くも悪くもエンジニアですので、
スキルが高い人のコメントは、
攻撃的だろうとなんだろうと、
通ってしまいやすいという構造もあるかもしれません。
コードレビューの様々な形態
さて、一口にコードレビューといっても、
様々なシチュエーションが考えられます。
また、現場によっても、
どういう方針でコードレビューを採用するかは異なることが
多いでしょう。
ここでは、コードレビューのいくつかのパターンを
あげていこうと思います。
自分より上のエンジニアにレビューしてもらう(一般的なレビュー)
まず、よくあるのが、
自分よりベテランないし、スキルが高い、または役職が上の人などに、
レビューしてもらうパターンです。
おそらくこれが最もよくあるパターンかと思います。
ただ、このパターンの場合、
力関係がはっきりしているため、
しばしば、前述の怖いコードレビューに陥りやすいという
デメリットがあります。
自分より上のエンジニアに攻撃的なコメントをされてしまうと、
なかなか反論できないものかと思うので、
コードレビューとしては、かなりギスギスしたものになってしまう恐れがあります。
一方で、コードレビューの質自体は、
自分より上の人の目を通してもらうことで、
ソースコードの品質担保という面ではメリットに働くでしょう。
また、スキルの観点では、自分では気づけない観点とかを指摘してもらえるので、
レビューのたびに強くなれるみたいなメリットはあるでしょう。
総じていうと、ストレスは溜まるが、
スキルは上がるといったところでしょうか。
自分と同等のエンジニアにレビューしてもらう(相互レビュー)
次に、自分と同階級のエンジニアにレビューしてもらう、
所謂相互レビューについてです。
これは、私が勤めていた会社が採用している方針でした。
メリットは、互いにフラットなので、
コードレビューを通して、議論をしやすく、
風通しも良いという特徴があります。
そのため、この場合はあまり怖いコードレビューには
なりにくいかと思います。
デメリットとしては、
自分より上のエンジニアの場合の裏返しですが、
質の担保に若干の不安があるということですね。
この場合は、ある程度自分でも、
そのままmergeしても大丈夫だなっていう状態の品質にしておかないと、
レビュー漏れが起きやすくなるので、品質担保の面では若干の不安が残りそうです。
自分より下のエンジニアにレビューしてもらう(お手本を見せる)
これは正直そこまで一般的ではないのですが、
先述の相互レビューの文脈で、
「こういう感じで書いたから、参考にしてみてください」というように、
下のエンジニアにお手本を見せるという場合で使うものかと思います。
まあ、これに関しては、そもそもコードレビューじゃねえよってなるかもしれませんが、
下のエンジニアが、コードレビューのフェーズを通して、
議論をすることができるという点ではコードレビューになるかと思います。
メリットは、相互レビューのときとほぼ同様ですね。
デメリットは、品質担保の面では、実質的に
コードレビューなしというところでしょうか。というのも、
下のエンジニアには、上のエンジニアが書いたコードをレビューするのは、
現実的には難しいことが多いからです。
コードレビューがない現場
そもそも、コードレビューがない現場もあります。
これは、正直文化としては、よほどの事情がない限り、
成熟していないと言わざるを得ません。
コードレビューをしないことによるデメリットは、
枚挙にいとまがありません。
- コードの品質を担保できない
- コードがどういう意図で追加したのかわからない
- 簡単にベースブランチにmergeできてしまうのは保守性の面で厳しい
- エンジニアのスキルも上がりにくい
- チーム開発のメリットを活かせていない
- 誰でも好き勝手にコードを書き換えられてしまう状況に近い
といったところでしょうか。(まだまだたくさんありそうですが)
わかりやすい例としては、
今日入ったばかりの新人エンジニアが書いたソースコードを、
コードレビューなしで、そのままmasterにmergeしてしまい、
障害に繋がってしまったりなどでしょうか。
もちろん、コードレビューをしたから、
障害を防げるというわけではないのですが、
そもそも最初からコードレビューをしないで障害が起きたのと、
コードレビューをして、障害が起きたのとでは意味が違います。
コードレビューをしていたのであれば、
コードレビューの漏れを学習できるので、
後に活かすことができます。
コードレビューをしないというのは、
よほどの特殊ケースを除いて、
チーム開発の破綻と言っても過言ではないかもしれません。
コードレビューについて思うこと
さて、コードレビューの概要について、
大まかに述べてきました。
ここで私なりに、コードレビューについて
思うことを縷縷述べていきたいと思います。
コードレビューの目的
まず目的ですが、
これは正直チームによって変わるような気がします。
ただ、コードレビューの目的として
よく取り上げられるものとしては、
- ソースコードの品質担保
- 第三者チェック
- チーム開発の文化醸成
- コードレビューを通して現状のコードのissueを見つけたりと議論のプラットフォームとして機能させる
といったところでしょうか。
なので、そのチームによって、
上記のどれを重視するかによって、
目的も変わるかと思います。
一般的には第三者チェックが、
コードレビューの目的になることが多い気がします。
コードレビューのコスト
コードレビューは、基本的には行うべきものですが、
少なからぬ工数がかかるのも事実です。
というのも、基本的に、
自分のタスクを中断して、
他のエンジニアが書いたコードを見て、
検証するというのは、コード量にもよりますが、
それなりに工数がかかるものでしょう。
その点を勘案して、
コードレビューの比重を落とすなどは、
チームによっては有り得る選択でしょう。
ただ、個人的には、
コードレビューをしやすいようにするというのも、
大事かと思っていて、
実装者が事前に「こういう観点で見てほしい」などと伝えておけば、
レビューワーも負担が減る思うので、
そういう気遣いで、コードレビューの工数は小さくできるような気もします。
コードレビューでやってはいけないこと
コードレビューでやってはいけないことの最たるものは、
攻撃的なコメントです。
例えば、「なんでここのコードこんな汚いの？」みたいな、
直接的かつ、実装者を貶めるようなコメントは絶対にやってはいけません。
とはいっても、スキルが高いエンジニアの方に限って、
このようなコメントをしてしまっているケースは少なく有りません。
スキルが高い人からすると、なぜこんなコードになるの？という
シンプルな疑問の発露なのかもしれませんが、
それを受け取る側からしたら、かなり威圧的に感じるはずです。
それが結局のところ、コードレビューが怖いの原因になったりします。
また、このような攻撃的なコメント以外にも、
決めつけのようなコメントも好ましくないなと個人的には思います。
例えば、「ここはちょっとまずいですね。修正してください」のような、
まずいことが前提で話を進めてしまうのは、一方的なコミュニュケーションになりがちです。
ただ、コードレビューの難しいところは、
まずいコードは確かに指摘する必要があるので、
まずいコードを見つけて、黙認するのもよくありません。
その場合は、「ここのコードはこういう風にしたらどうでしょうか？」と
あくまで提案スタイルでコメントするのが好ましいと思います。
その際、例示のコードも一緒に添えておくと親切かと思います。
コードレビューは品質担保の側面ももちろんあるのですが、
それ以前に、コミュニュケーションの一種です。
コードレビューしていると、目の前のコードばかりを見てしまって、
忘れがちですが、向こうにはそのコードを実装してくれた人がいるということを
忘れてはなりません。
基本的に、コードレビューは、実装者とのコミュニュケーションです。
一方的だったり、高圧的だと、円滑なコミュニュケーションになり得ないのは
言うまでもないかと思います。
コードレビューのコミュニュケーションの側面を軽視しすぎた結果が、
コードレビューが怖いというイメージの醸成にもつながっているのかと思います。
それに、レビューワーと実装者のスキルレベルがよほど離れている場合を除いて、
基本的にそのコードについては、実装者が一番詳しいものです。
なので、そもそも実装者はコードレビューに対して、
異議があれば、「ここはそういう意図ではありません」という風に、
反論するのも全然ありです。
基本的にコードレビューは、
先輩エンジニアが後輩のコードをレビューして
修正させるという一方通行のものではなく、
対話の場所ということは軽視すべきではないと思います。
コードレビューのやり方
上記を踏まえて、
コードレビューのやり方ですが、
基本的には下記の点が重要だと思います。
- コメントは必ず敬語
- 実装者へのリスペクトを忘れない
- 良くないと思ったコードは提案形式でコメントする
- 最終的に修正するかどうかは、よほどまずいコードとかじゃない限り実装者に任せる(修正を強制したり、指揮命令関係になるべきではない)
- まずいと思ったコードも、即座にまずいと決めつけるのではなく、そういうコードにした意図を聞く
- レビューワーは、質問や提案をしていくことで、実装者にまずい部分を気付かせるのが理想(直接ここがだめとは指摘しない)
- レビューワーは、実装者が考慮していなさそうな部分(実装者の苦手部分)とかでコードを見て、まずい部分を探す
- 動作確認して問題ないか確認する
- 良いと思ったコードは、褒める(ただ、これはレビューワーが実装者よりも先輩の場合。同階級だと下手に優劣の文脈が生じる可能性があるのでケースバイケース)
要するに、コードレビューは、
実装者とのコミュニュケーションで、
実装者へのリスペクトを忘れてはならないということが、
最も大切なことだと思います。
世間で強強と言われている人に限って、
自分がスキルが高い反面、
こういった基本的なことを軽視している印象があります。
変数の命名、リーダブルコードとかでもわかりますが、
コードを書くことは、チーム開発で、
他のエンジニアがどう受け取るかということを汲み取る力は、
エンジニアの基本スキルです。
世間で強強と言われていても、
こういったことを軽視するような人(所謂ブリリアントジャーク)は、
二流、三流と言わざるを得ません。
