Zenn
🦥

コードレビューにおける「可読性が悪いので書き直して」という指摘はレビュアーの怠慢である

2024/05/10に公開
コードレビュー
可読性
tech

はじめに

2011 年 4 月にエンジニアデビューしてから今まで「可読性が悪いので書き直して」という趣旨の指摘を受けたことがほとんどなかったのですが、この 1 年は可読性警察に逮捕される日々が続いています。

ただし、私自身は「書き直しを強制されるほど可読性が悪いかな？」という気持ちで誤認逮捕された感が拭えず、これを機に「可読性とは何か？」を整理することにしました。

取り締まり例

実際に指摘を受けた TypeScript コードを 2 例紹介するので、皆さんもレビュー指摘内容に共感できるか考えてみてください。

ケース 1

1 つ目のコードは、ユーザーを年齢の昇順で並び替えるコードです。

before.ts
type User = {
  name: string;
  age: number;
};

const users: User[] = [
  {
    name: "A",
    age: 20,
  },
  {
    name: "C",
    age: 40,
  },
  {
    name: "B",
    age: 30,
  },
];

users.sort((x, y) => x.age - y.age);

このコードは「最終行の sort で使用した変数名 x と y が何を指すのかわからない点が可読性が悪いので、ユーザーであることがわかる変数名にしてほしい」という趣旨の指摘を受けました。

次のようにコードを修正したのですが、読みやすさは改善されたのでしょうか？

after.ts
users.sort((user1, user2) => user1.age - user2.age);

ケース 2

2 つ目のコードは、商品の購入可否を判定するコードです。ここでは、次の 3 つの条件を満たしている場合に購入可能とします。

  • ユーザーはログイン中である
  • 商品の在庫がある
  • 利用可能なクレジットカードが登録されている
before.ts
const canPurchase = () => {
  const isLoggedIn = true;
  const existsStock = true;
  const isAvailableCreditCardRegistered = true;

  return isLoggedIn && existsStock && isAvailableCreditCardRegistered;
};

このコードは「3 つ目の条件を見るときに前の 2 つの条件が満たされていることを覚えておく必要があるという点が可読性が悪く、早期リターンを使って書き直して欲しい」という趣旨の指摘を受けました。

次のように修正しましたが、修正後のコードも最終行の return true; を見るときは 3 つの条件が全て満たされていることを覚えている必要があるという点は変わっていません。この修正で可読性は改善されたのでしょうか？

after.ts
const canPurchase = () => {
  const isLoggedIn = true;
  if (!isLoggedIn) {
    return false;
  }

  const existsStock = true;
  if (!existsStock) {
    return false;
  }

  const isAvailableCreditCardRegistered = true;
  if (!isAvailableCreditCardRegistered) {
    return false;
  }

  return true;
};

可読性の定義はあってないようなもの

Wikipedia では、プログラミングにおける可読性について次のように説明しています。

プログラムのソースコードを人間が読んだときの、その目的や処理の流れの理解しやすさを指している。ソースコードを保守するのは人間であるため、その可読性は重要である（ただし、未熟なプログラマが、よく使われるイディオムによる簡潔なコードを「可読性の低いコード」と見なしてしまうことがあり、読む人間のスキルもまた重要である）。コメントも可読性に影響し、適切なコメントは可読性を向上するし、誤ったコメントが放置されていれば誤読のおそれが高くなる。可読性の低いコードは、バグを生みやすく、重複コードによる非効率を生みやすい。

上記のとおり可読性とは「理解のしやすさ」を指す曖昧な言葉なので、コードレビューで「可読性が悪いので書き直して」と指摘してもレビュイーの共感を得られることはありません。私自身、前述した 2 つの事例はレビュアーのスキルや好みに依存するものだと思っていて、修正前後で可読性は 1% も改善していないと思っています。コードの書き方について指摘をする場合、レビュアーは 可読性という曖昧な言葉で濁さず、どこがどう悪いのかを具体化して伝える努力をしなければいけません。

では、どのように具体化したら良いのでしょうか？

曖昧な可読性を具体化する

ここでは具体化する方法を 3 つ紹介します。

定量的な指標を使う

サイクロマティック複雑度のような定量的な指標を導入する[1]ことで、曖昧さを排除することができます。

サイクロマティック複雑度は Wikipedia で次のように説明されています。

Thomas McCabe が開発したもので、プログラムの複雑度を測るのに使われる。プログラムのソースコードから、線形的に独立した経路の数を直接数える。

手法としてではなく、そのコンセプトは文章の可読性（複雑度）を測定する Flesch-Kincaid Readability Test に似ている。

曖昧さのない判断基準に従うのであれば、誤認逮捕されたと感じることもなくなるはずです。

定性的かもしれないが一般的な指標を使う

次はメールを送信する関数 sendEmail 呼び出しのコードですが、どちらの方が理解しやすいでしょうか？

sample1.ts
sendEmail("foo@example.com", ["bar@example.com"], [], [], "test", "test mail");
sample2.ts
sendEmail({
  from: "foo@example.com",
  to: ["bar@example.com"],
  cc: [],
  bcc: [],
  subject: "test",
  body: "test mail",
});

1 つ目のコードは sendEmail の利用者に引数の順序を把握することを要求する分、読み手に負荷を与えます。一方、2 つ目のコードは各引数の意味が引数名で明確になるので、読み手の負荷を減らすことができます。

1 つ目のコードをレビュー依頼された場合は「可読性が悪いので書き直して」と指摘するのではなく、「各引数の意味が呼び出し側のコードを見ただけだと分かりにくいので、分割代入引数を利用して各引数の意味が呼び出し側のコードを見ただけで明確になるように書き直して」と指摘する方が、レビュイーの共感を得られます。

ちなみに、この指標は コナーセンス と呼ばれるソフトウェアの品質指標の一つです。

ルール化する

指摘したい内容を計測するメトリクスも無いしうまく言語化することもできないけど、どうしても修正してほしい場合もあるでしょう。その場合は、コーディングルールとして明文化します[2]。ただし、ルールに特例を設けることは NG です。特例を設けると「このケースは特例に該当するのか？」の判断が人によってブレてしまい、曖昧さを排除することができないためです。ルール と ガイドライン は明確に区別して運用する必要があります。

余談

本記事を締め括る前に、前述した 2 つのコードの可読性は修正前後で 1% も改善しないと考える理由を説明しておきます。

ケース 1

問題のコードを再掲します。

users.sort((x, y) => x.age - y.age);

私自身、上記コードの x や y は for 文で利用するループカウンター i や j と何ら変わらないものの認識です。ループカウンターに i や j を利用されたコードを読んで「読みにくい」と感じることがないのと同様に、ラムダ式で x や y を利用されたコードを読んでも「読みにくい」と感じることはありません。

そもそもコードを読むとき、下図のオレンジの範囲 だけ を読んで内容を理解しようとすることはあるでしょうか？

コードを開いた時にオレンジの箇所に最初に目がいくことはあっても、コードを読んで内容を理解しようとするときは、下図の緑で囲んだ範囲 全体 を見るのではないでしょうか？

意味ある単位で見れば「x と y が何を指すのかわからない」という指摘は出ないはずです。それでも x と y がユーザーであることを読み替えるコストが発生するなどと主張するのであれば、for 文で利用するループカウンターの i や j は許容するが、ラムダ式内の x や y は許容しないことに対する論理的な説明が欲しいです。

ケース 2

意味的に似たコードは 1 つのブロックにまとめたいので、個人的には修正前のコードの方が読みやすいと思っています。

具体的にいうと canPurchase は次の 2 つの処理で構成されます。

  1. 購入可否の判定に必要な情報を取得・構築する処理
  2. 処理 1 で取得・構築した情報をもとに購入可否を判定する処理

修正前のコードは次のとおり処理 1 ( オレンジの部分 ) と処理 2 ( 緑の部分 ) がブロックで分かれており、整理されていて読みにくさを感じることはありません。具体的な原則名があるわけではないですが、クラスや関数などを意味のまとまり単位で分割するのと同様に、関数内の処理も意味のまとまり単位でまとめた方が読みやすく感じます。

一方、修正後のコードは次のとおり処理 1 と処理 2 のコードが混ざってしまい、修正前と比べて読み難く感じます。また、コード量も約 3 倍になって冗長だとも感じます。

ただし、この点に関しては好みが分かれると思っていて「修正前の方が絶対読みやすい」と強く主張する気はありません。私が誤認逮捕されたと感じる理由は、指摘で受けた次の内容が修正後のコードでも解消されたと感じられず、指摘に筋が通っていないと感じるためです。それにもかかわらず、修正を強制された点も大きいです。

3 つ目の条件を見るときに前の 2 つの条件が満たされていることを覚えておく必要があるという点が可読性が悪く、早期リターンを使って書き直して欲しい

まとめ

タイトルのとおり、コードレビューにおける「可読性が悪いので書き直して」という指摘はレビュアーの怠慢 です。どこがどう悪いからどう修正して欲しいのかをちゃんと言語化して伝えましょう。

言語化にあたっては一般的なソフトウェアメトリクスや原理・原則などが参考になるでしょう。普段から学習して、知識をアップデートしていきましょう。

うまく言語化できないけど修正を強制したいという場合、それは正当な指摘ではなくレビュアーの好みの問題です。チームメンバーに合意を得た上でルール化しましょう。

脚注

  1. もちろん、採用する指標と基準値については事前にチームで合意を取る必要があります ↩︎

  2. もちろん、ルール化する内容は事前にチームで合意を取る必要があります ↩︎

Discussion