序

「コードのレビューのしやすさ」が話題になっているのを見た。 そして、そういえばこれまで私は「読みやすいコード」については触れてきたが、「レビューしやすいコード」については触れてこなかった。

「読みやすいコードはレビューしやすいコードではないのか」?

答えはノーだ。 簡単に言えば、参考書に求められるものと辞書に求められるものは違う、ということだ。

そして、それはかなり難しい要素を多数はらんでいる。

今回はこの「レビューしやすいコード」について語っていこう。

そもそもレビューとは

ここでいうレビューはコードレビューである。 つまり、設計などのメタレベルのレビューではない、という前提で話している。 また、コードレビューがそうしたメタレベルのレビューを必要としていないということも前提としている。設計や要件は、事前のレビューを経て承認されているか、もしくは別の場でフィックスされている想定である。

そのコードレビューに関しても様々なものがある。 無数のレビュー観点があるのだ。

基本的にはコードレビューは何をどうレビューするかということにおいて、何らかの信頼を前提としている。 例えば、動作テストはした前提とかだ。 AIの書いたコードのレビューがしんどいのは、どこにも信頼をおける場所がないからだ。 (AIはどこにでもトラップを仕込んでくる。難しいロジックはちゃんと書けているのに値の代入に考慮漏れがあったりする場合だってある。盲信するのはとても危険だ。)

例えば信頼のおけないジュニアエンジニアが書いたものに関しては、基本的に全面的なコードレビューを実施することになる。 これは非常にしんどい作業だ。ジュニアエンジニアが書いたコードはコードクオリティが十分でない可能性が高く、一般的に読みにくい。読みにくいコードを全部読むくらいなら自分が書いたほうが早いので、これは教育コストとして支払う感じになる。

同様の問題として、「レビューに書く以上の時間を要するなら、他の人が書く意味がない」が発生する。だから、なるべくレビューする観点は絞り込みたい。 CIによるテストはある程度コードが正しいことを担保するが、十分ではない。テスト自体を書く必要がある場合はなおさらだ。テストフレームワークによるテストはそこに書いたテストしかしないのだから。 だから、まずは「動作テストはしたもの」という前提を置き、「動くか動かないかのチェック」はしなくていいようにしたい、というのが第一歩である。

その上で、どこを見るかだが、コードオーナーであればコードスタイルに合致しているかということは見たい。 つまり、「動作としてはそれで良いが、それはこのように書くべきだ」というものだ。 これもLinterでインデントなどのスタイルは強制できるが、大抵の場合見たいのはそこではない。

例えばユーティリティ関数を再発明していないか、命名規則はあるべき形になっているか、フロー制御が既存と一貫性のあるものであるか、ネストの書き方が流儀に合っているか、などなど。

他にもコード設計が意図したものであるか、考慮漏れ等はないか、データ設計は適切か、などなど観点は多岐に渡る。 チーム開発においてはレビューコストは避けて通れない問題で、「どのような問題を防ぎたいか」から「何をみるべきか」が発生し、なおかつ見るべきものは少なくしたほうがレビューコストを下げられる、ということに向き合う必要がある。

そして、この見るべきものは、書いた人の立場や見る人の立場にもよる。 「レビューがどのようなものか」は個別ケースで千差万別で、よって「レビューしやすいコード」の条件も場合によって千差万別なのである。

基本的なレビュー観点

OSSのケースで考えてみよう。 私は多数のリポジトリの作者だ。そこにプルリクエストが来て、レビューすることになったとする。 その手順もものすごく大切なのだが、一旦それは置いといて、私はそのコードを前向きにレビューしたとしよう。

まず見るのは、流儀に従っているかどうかだ。 書き方が私と全く違うコードを書いてきたら、内容を見る前にrejectする。 それはつまり、私のコードを読んでいない、もしくは尊重していないということなのだから、私のリポジトリのコードに混ぜることはできない。

次に見るのは、それはソフトウェアとしてあるべき機能かどうかだ。 基本的にはどのような機能を追加しようとしているのかは事前に合意されているべきで、合意せずにPRを送ってきた時点でrejectされて然るべきなのだが、まぁ、そこはTODOになっていたりしたらいきなり送られてきても読むかもしれない。 そして、その場合は機能が適切かどうかを評価することになる。

そして、インターフェイスが適切かどうか。 さらにはコードクオリティ。 などなど見ていくことになる。

では企業のチーム開発だと考えてみる。 この場合はその人の立場(職務上の立場もあるし、そのタスク上の立場もある)によって基本的に見ようとするものが変わる。

コードオーナーがいて、リポジトリのコードを誰かが責任をもって管理しているとしたら、コードオーナーとしてはやはり「自分が書いてもこうなる」を求めたいところだ。

相手のコードクオリティに全面的な信頼を寄せられるのなら、単純にコードを見て「自分ならこうする」を言うだけでいい。

また、ビジネスドメインに詳しい人のレビューだとしたら、つけられた名前の適切さを確認したり、値を算出するロジックの正しさを確認したりするだろう。

もしコードクオリティに不安がある人が出したものであれば、ビジネスドメインという「別観点」ではなくてもロジックの正しさを確認したくなる。 「根本的には正しくないが、テストは通る」みたいなロジックも存在するからだ。

際どい挙動が生じやすい問題であれば考慮漏れがないかは、例え技術的に信頼できる人物から出てきてもダブルチェックの意味で見たいものだ。

「コード以前」の問題でもある

本稿は「レビューしやすいコード」の話をしているわけだが、実は「レビューしやすい」はかなりの部分、コード以前のものが握っている。

正直に言おう。私のOSSプロダクトにいきなりDescriptionのないPRを投げつけてきたら、私はチラ見しただけでrejectする。

根本的に「誰に、何を見て欲しいか」を明示しないPRは、レビュワーの負担が大きいのだ。それが一瞬見ただけで理解できるようなものであればいいが、そうでないのなら「読んだ上で何をみるべきかを察しなくてはいけない」が発生する。 これはかなりレビューコストが高い。

だから、「誰に何を見てほしいかを簡潔にわかりやすく説明する」はものすごく大事であり、レビューしやすいコード以前の問題なのである。

そしてもうひとつ。 そもそものチケットで何をしようとしているのかを知らない人に、「こういう変更をしました。こういう問題がないか確認してください」と言っても、それは意図が分からないのでやはりコードを読んで察する作業が発生する。

だからレビューを依頼するであろう相手には事前に軽くでもチケットのコンテキストを共有しておく…… といいのだが、こっちは相手のバッファを埋める行為でもあるので、やりすぎると鬱陶しい。これをどのくらいやるか、というのはそもそもレビュー密度をどの程度に設定するかにもよるので、運用である程度決めておきたいところになる。

レビューしやすいコードとは

「レビュー以前の問題」をクリアしたとして、じゃあレビューしやすいコードとはどんなものか、というと、これは万能の解はない。

のだが、コード以前の問題同様、「見るべきものが多いコード」はレビューしづらい。

スコープが多いとコードをかなりしっかり読み解かなくてはいけなくなる。 「これは〜するためのもので、〜のために〜を〜しました」くらいの文章で説明しきれるくらいのコードにしておきたい。

チケットに対応しようとしたらスコープが広くなって広範囲をいじらなくてはいけなかった、というのであれば、それをサブタスクに切り直すとかして小さなスコープで独立するようにすべきだ。

典型的なので言うと、「ついでにばっさりリファクタリングしました」はその変更が独立した機構でない限りやめたほうがいい。 誰も使っていない旧来のコードを廃止して、今回のタスクで追加する機能が唯一のその機構のユーザーになる、というのであればまぁ……という感じだ。

多くの場合、差分はそのものが小さいことが望ましい。 が、新規の機構で、機構が独立しており、その機構を自分がメンテナンスするという前提であれば、大きな差分でも問題のない場合もある。だいたいのプラットフォームにはPRで特定のファイルを無視する機能がある。その機構そのものの責任はPRを出した人が負う前提であれば、そこについて詳しくは見ない、というオプションもあるのだ。

が、これは「レビュー以前の問題」がものすごく大事だ。 新しい機構を追加して、その面倒は自分が見るものであるということがちゃんと事前に共有されていないと、受け取った側は当然レビュー対象に入るものだと受け取る。 もちろん、「新しい機構を作る」ということ自体にも合意を得ておく必要がある。スタンドプレーが許される環境で、かつ自分がコードオーナーであれば独断で新機構を導入するみたいなことをする場合もあるが、そうでないなら「新機構作ろうと思ったので入れました」はレビュー以前の問題のひとつである。 場合によっては設計の合意だって必要だ。

そしてもちろん、新機構をレビューを受けずに投入したなら、ドキュメントを書く責任もある。

その他全般に言えることとしては、機能的なコードにおいては意味のない変更は悪だ、というのがある。 典型的には「フォーマッター拡張を入れているのであちこち勝手に変わりました」みたいなのだ。

基本的にレビュワーは「差分は須く意味のあるもの」として見る。 だから、意味のない差分はレビューを著しくやりづらくする。ギルティ。

コードの整形がもし必要なものだとしても、それは別のタスク、リファクタリングPRとして分けるべきものだ。

あとは大抵の場合、コードは意味明瞭な書き方をしておくほうがいい。 これは「局所的に読みやすいコードを書け」という意味だ。 計算量などの問題で難解なロジックを書く必要があるのでなければ、説明をそのままにコードにしたような愚直なコード¹にしたほうが読みやすい。

このあたりはレビューする人が誰か、というところにも依存する。 例えば数学的に高度なロジックを書いたとしても、それを見て一瞬で理解できるような数学力のある人が読むなら問題はない。

忌避感を感じる人もいるかもしれないが、こうしたコードは「読む人が読みやすいように」と意識して書いたほうが良い。

どうしてもそのように書きづらい場合は、コメントで説明するか、Descriptionで説明するか、長くなるならドキュメントに起こすべきだ。 ロジックを解説したほうがいい場合もあるが、より説明すべきは「なぜこの複雑なコードを必要とするのか」だ。 これは通じるなら暗黙の了解としてもいいのだが、できれば後で入ってくる人のためにも説明があったほうがいい。だから、Descriptionよりはコメントやドキュメントにしたほうがベターだ。

レビュワーにあまりに知識がなくて愚直なコードでさえ読み解けない場合? それは不幸を呪うしかない。 私もそういうケースは経験した。

これはチームビルディングとかそういう話になるのだが…… もし常にそういう人のレビューを通らなければマージできず、そのために滞るようであれば、そんな人を上位に据える会社は転職を考えたほうがいいかもしれない。

おまけ: OSSの手順

会社であれば基本的に合意のない開発は認められないと思うが、OSSではそれを省略してくる人がいる。なんなら、割といる。

が、まずissueを立てろ、である。 そこで、何が必要で、どのようにすべきであると考えているかをきちんと説明するのが最初。 この際、自分が書くつもりであればその旨を説明する。

それは会社でチケットを切る場合と似ているが、新しいコントリビューターはどんな人物かというのは分からないので、より丁寧に説明する必要がある。

つまり、その変更がソフトウェアとして合理的で受け入れられるものであるという合意を得る必要があるのだ。

そして、実装は誰がやるのか、というのは別に合意である。

その上で実装して、その実装について説明する必要もある。

OSSの場合、必要なものの8割はコードよりもきちんとした説明だ。