Pull Request のコメント数を減らすアホみたいなコツ
私は長年 Pull Request のコメント数が多くて何回もレビューを往復することが多くて大変つらかったが最近ものすごく単純なコツに最近きづいたのでそのことをシェアしようと思う。
Pull Requestレビューの悩み
これはならない人はならないので、共感してもらえる人は少ないかもしれないが自分の悩みは Pull Requestのコメント数でこれが本当に多い。何がつらいって、レビューのコメントが多いという事は、マージに時間が掛かるということだ。最初にコードを書いてテストして完成させるのは２時間もかかってないのに大抵レビューで何往復もして時間を取られるのが本当につらいし、進捗がでないもの嫌だし、時間かかるし、自分が最近解決したい問題の中でも筆頭の問題だった。
何が悪いのだろう？
すごく嫌なので物凄く考えたがうまくいかなかった。例えば、英語のスペルミスも良くしたし、ログやコメントの英文にレビューが入ることも多かった。マネージャに、このクラスはインナークラスで外出しにとか、ここはこういう書き方ではなく、こういう書き方がいいと言われたりしていた。
正直なところ、自分としては自分が考えて作ったクラス構造やテストの構造のほうが良い気がするし、何なら自分のバージョンのほうが計算量が少なくて済む。他の人はあまりユニットテスト書いてないのに私のほうががっつり書いているのに、なんで？と思っていた。リポジトリオーナーの好みの問題？他の人に相談しても「君のコードはくそコードではない。」と帰ってくる。コメントの内容を読んでみても、自分がオブジェクト指向の設計パターンや、C#のモダンなコーディングの書き方を知らないからとかではないし、DIとかもちゃんと使うし、リファクタリングはまめにしている。人一倍ユニットテストも書いてテストしてるので挙動も完璧。なので本当の本当の本音を言うと
俺のコードのままでええやん。何が問題なん？
と思っていた。ただ、私だけコメントがたくさんつくのは変わらないので、それを減らしたかった。何か知識が足りないならそれを補えばよいのだがそうでもない様子なのでまさに八方ふさがりで、理由がよくわからなかった。リポジトリオーナーの趣味に合わせるの？
クリスの一言
いつも自分を助けてくれるクリスに相談して、私がコメントをたくさん食らっているPRを前に解説してもらうことにした。PRのコメントは大抵「こうしろ」と書いてあるだけで、なぜそうなのかの理由までは書いてないことは多い。
最初にクリスが言った一言はとても心に残った。「みんなプルリクエストをApproveせずコメントを残すのは、わからないからだよ」と言っていた。私は周りの人は私より１００倍凄いプログラマなのだから、自分のコードなど当然理解できると思っていたがそうではないようだ。その「わからない」というのは、その人が全力を使って「わからない」のではたぶんない。
自分は何がわからないのかわかっていない
例えば、そのPull Request の Dictionaryのforeach で、リストを回すときに、私は .Values.ToArray() をしていた。クリスは「ここは理解できないので、コメントを書いたほうが良い」といった。自分としては？？？だった。そんな長くないこのループを下まで読めば、Dictionaryを変更する必要があるので、Dictionary自体でループをしたらそれが出来ないので、わざわざToArray()しているのは自明じゃないか？と私は思っていた。多分クリスもループを下まで読めばきっとわかる。クリスは私に聞いた。「ここは、なんでDictionaryをそのままKeyValueでループしないの？」「それは、Dictionary 自体でループしないのは何故？」「それはDictionaryを変更する必要があるから、もう一つの理由は、メモリにKeyとValueが両方乗るより、Valueだけのほうが省コストでしょう。」「なるほど。でもそれって、このループを最後まで読まないとわからないよね。だから下まで読まなくてもわかるようにここにコメントを書くとよい。みんなわからないことにコメントするんだ。で、このToArray() はなぜToList()じゃないの？」
正直自分はそんな細かいことどうでもよいと思っていた。ちゃんとロジックがユニットテストで完璧にテストされており、Dictionaryが変更可能になればよいと思っていた。しかし、見る人が見ると、ToArray() である「理由」がわからないのだ。ToArray() のほうが領域を確保してしまうので、冷静に考えたらToList()良いのは分かる。知識では知ってるはずのことだったけど自分は気にしていなかった。他の人が「何がわからないと思う」のかを。
コードは書くものではなく、読むもの
自分がわかってなかったのはこのことだ。コードは書くものではなく読まれるものだ。自分はコードが重複なく、良い設計で品質よくしっかり動作するのが重要だと思っていた。
しかし、コードは書くより、読まれるほうが圧倒的に多い。Pull Request でコードを書く時は、「読者が読みやすいもの」を書く必要がある。読者に「なんでここはこうなってるのだろう」と思わせたら終わりだ。自分は周りのエンジニアのレベルが高いので、彼らがわからないはずがないと思っていたが、きっと作者である自分よりコンテキストが少ない。だから、めっちゃくちゃわかりやすくしてちょうどよい位だ。
読み物と考えるといろいろ変わってくる。本と同じだ。自分にとってスペルミスとか、ネイティブにわかりやすい表現とかは「細かいこと」と考えがちだが、読むほうからすると混乱の原因だ。だから、それ以降は、スペルチェッカーを常にonにしたり、ログメッセージはChatGPTでリライトしたりしている。
読みやすさは単純な構文というわけでもない
読みやすいというのは、単純な構文を使うという話ではない。「わかりやすく、明確に」ということだ。例えば、Linqを使ったほうがめっちゃ簡単に書けるのにだらだらと、For文で書いていたら「なんでこの人はLinq使わずにめんどくさいことをしているのだろう？」と思うだろう。その場で使ったほうが便利な構文やライブラリがあるのに使われてなかったら「あれ？」と思うだろう。ただ、そうなると、何が他の人にとって「あれ？」と思うのだろうというのが疑問になるだろう。次に紹介するGood Code Bad Codeはその問いに１００％答えてくれた。
コードを書くという意識をすてて、「読みもの」と考えたら、コードを書いている最中でも「こういう書き方したら読む人がちゃんとわかりやすいかな？理解できるかな？」という事を常に考えるようになって、そういう観点でリファクタリングする癖がつくとコメントの数が減少していった。
お勧めの本
さて、コードの読みやすさに関する本は実はたくさん出ているのだが、私の今の一押しは下記の本だ。リーダブルコードや、リファクタリングなどの本は良い本だがいかんせん古い。だから最近のモダンなコーディングの「わかりにくさ」に関しては言及がない。ところがこの本はモダンな構文での「わかりやすい、わかりにくい」例が載っているし、基本的なロジックのわかりやすさから、設計の観点でのわかりやすさ、わかりにくさも解説しているので、If分の書き方から変わってきた。マジ最高。
レビューが付いた時に気をつけること
ただ、それだけやっていてもコメントがつくことがある。この時に注意したいことは、レビュワーのコメントがついて、その通りに修正してもうまくいかないことがあるということだ。
例えば、レビュワーがコメント１で「変数名を＊＊＊に変えて」と言っていたけど、コメント２で別の変更をしてといってきて、これにこたえると、最初の変数名がもはや適切でなくなるケースがある。
また、先日あったケースだと、レビュワーが、このLinqの構文の部分をログを吐くようにしてと依頼された。その構文でログを吐くためには、もともとワンライナーでシンプルだったものを、ラムダを使って、複数行で表現したうえで、bool を反転させる必要がある。.Where(p => p.some()) が .Where p => { if (p.some()) { Log… return false; } return true; }) とかやって、Can't read と言われた。これは考えると新しい関数を書いて.RemoveWhere(p => Some(p)); でブールの反転をさせることがなく書ける。
何が言いたいか？というと、レビューをそのまま実装しようと考えると、「コードは読み物」のマインドセットがあっても、コードが歪んでしまうことがある。だから、コメントはコメントと受け取って、ゼロから考えて「読み物」のコードを書いていこう。
まとめ
Pull Request のイテレーションを短くしたかったら、コードは「読み物」と思って書こう。他の人見てなんの疑問を持たずに楽に理解できるように、目指してみよう。時にはO(M*N) のソリューションが O(N)より良いとされるケースがあるかもしれないそれはきっと「読みやすさ」だろう。それぐらい人が理解しやすいコードが他の品質属性と同じ、それ以上に重要視される理由だろう。だって読めなければだれもメンテできないし、効率もダダ下がりだから。
最後に、コードの読みやすさは時代とともに進化するので、キャッチアップをわすれずに楽しもう！