ルール無用の無法地帯と化すソースコードのコメントとは

## 前置き

私が所属しているチームの輪読会で『良いコード/悪いコードで学ぶ設計入門』が選ばれました。
メンバーがそれぞれ章を分担して毎週発表をするスタイルになります。

書籍に書いてあることではなく、自身の言葉で説明要約するために、個人のアウトプットとして本記事を作成しました。

:::note info
本記事で利用しているコードは、原則的には書籍の内容とは別物 + TypeScriptを利用しています。
:::

## 11章 コメント

コメントにはプログラム言語のような決まったパターンもないため、書く人の思うがままフリースタイルで書かれることが多いでしょう。
コンパイル, 実行時にコメントは無視されて動作には影響を与えないのだから、どんな内容を書くのも自由。後で自分ないし他の人が読む時に、コードの理解を促せるので書かない理由はないです。

ただ何でもコメントを書けば良い訳ではなくて、それらの注意点についていくつか説明します。

### コメントも更新の対象

ユーザー種別はnew, comeback, exist の3種以上の定義があるとして、特定のユーザー種別である場合の条件分岐を行いたいとします。
※例として3種のみを表示しています。

実装当時の要件ではnew, comeback のみが対象であり以下のような実装でリリースされました。メソッド名も具体的な名前でコメントも残してあります。

```typescript:type.ts
export const allUserTypes = ['new', 'comeback', 'exist', '...'] as const;
export type UserType = typeof allUserTypes[number];
```

```typescript:main.ts
/**
 * ユーザータイプがnewかcomebackであるか判定する
 * @param userType 
 * @returns 判定結果
 */
function isNewOrCombackUser(userType: UserType): boolean {
  return ['new', 'comeback'].includes(userType);
}
```

しばらく経ち対象のexistのユーザーを条件に加えることになりました。次の実装者は以下のようにメソッド名と実装部分のみを変更しましたが、コメントについてはスルーされています。

```diff_typescript:main.ts
/**
 * ユーザータイプがnewかcomebackであるか判定する
 * @param userType 
 * @returns 判定結果
 */
- function isNewOrCombackUser(userType: UserType): boolean {
+ function isNewOrCombackOrExistUser(userType: UserType): boolean {
- return ['new', 'comeback'].includes(userType);
+ return ['new', 'comeback', 'exist'].includes(userType);
}
```

:::note warn
更に悪い状況としては、メソッド名が変更されずに処理との差異が発生するパターンです。メソッド名が嘘を付くことで被害がより大きくなることでしょう。
実際には`密結合`になってたり`テスト実装`がなくてメソッド名を簡単に変えられない、といった手遅れな状況もあります。抜本的な改修は一朝一夕にできないため、コメントを残すこともありました（経験談）
:::

コメントが変更されないとどうなるのか？　コンパイルもテストも全部通って機能上は問題ありませんが、後に保守を行う読み手に悪影響を与える可能性があります。
`VSCode`などで該当メソッドを利用しようとした際に、コードの候補として表示されたり、実際のコードを確認せずともメソッドの概要, 引数, 戻り値を表示してくれます。

![image.png](https://qiita-image-store.s3.ap-northeast-1.amazonaws.com/0/3206583/682865f9-337d-0b75-877a-a5e28b39de68.png)

読み手がメソッドの概要と名前に違和感を持って実装まで見てくれればいいのですが、必ずしもそうではありません。
仮に気づいたとしても読み手がわざわざ個別のメソッドの実装を読み解きに行く必要があります。気づかずそのまま利用された場合は思わぬ不具合や要件漏れなどが起こる可能性もあり得ます。

そもそもの話ではありますが、仕様・実装が変わることによってメソッド名やコメントの変更をしなくて良い状態が望ましいです。
実装上は特定のユーザータイプを判定するだけですが、ビジネスや施策の意図に適した名前がないか確認してみましょう。

```diff_typescript:main.ts
/**
-* ユーザータイプがnewかcomebackであるか判定する
+* ユーザータイプがキャンペーンの対象であるか判定する
 * @param userType 
 * @returns 判定結果
 */
- function isNewOrCombackOrExistUser(userType: UserType): boolean {
+ function isCampaignUser(userType: UserType): boolean {
  return ['new', 'comeback', 'exist'].includes(userType);
}
```

作業者もレビュワーも実装に着目しがちですが、例えば`GitHub`で実装の差分だけ見てテスト通せばレビューOKとした場合、実装とコメントに差異がある`退化コメント`が発生するかもしれません。これについては100%防げるものではないので、社内やチームで共通の認識を持つ必要がありそうです。

### ロジックを和訳しただけのコメントはしない

複雑な条件分岐についてコメントが書かれているケースはないでしょうか。実際に見たことも書いたこともあります。
コメントがないとamount(金額)という曖昧な変数と、ユーザーに関する処理の意図を読み取ることは難しいでしょう。

実装者は自分で実装しながら、後で見たらわからないなぁと思いつつ親切心で以下のコメントを残してくれています。
…が今回のコメントは省略できるため、次の項目で説明します。

```typescript
// 月額課金が10000以上で、新規か復帰ユーザーもしくはiosユーザーである場合特別な処理をする
if (
  amount >= 10000 &&
  (userTypes.includes(userType) ||
  users.find((user) => user.id === userId))
) {
  /* 特別処理 */
}

/* 通常実行 */
```

### コメントで補足するよりロジックの可読性を上げる

先程の和訳コメントを削除したのとロジックの可読性を上げてみました。
- 条件式内のand ,or は複雑化しやすいためis変数 + ビジネス上の名称として定義
- 変数名をより具体的な名称に変更
  - userTypes => campaignUserTypes
  - users => iosUsers
  - amount => monthlyAmount

ちなみに`iosUsers`とは`iphone`や`ipad`などの`ios`を利用したユーザーのことです。

```diff_typescript
- // 月額課金が10000以上で、新規か復帰ユーザーもしくはiosユーザーである場合特別な処理をする
+ const isCampaignUser = campaignUserTypes.includes(userType) ||
+   iosUsers.find((user) => user.id === userId);

- if (
-   amount >= 10000 &&
-   (userTypes.includes(userType) ||
-   users.find((user) => user.id === userId))
- ) {
+ if (monthlyAmount >= 10000 && isCampaignUser) {
  /* 特別処理 */
}

/* 通常実行 */
```

このように変数名やロジックを見直して変更することで、コメント自体が不要になるケースが多いです。
コメントも更新対象ではありますが、先述の通り漏れる可能性もあります。極端な話、コメントがなくても読み手に通じる実装を心がけたいところです。

### ロジックの意図や注意点をコメントとして残す

コメントがなくても全部読み手に伝わるといいのですが、命名やロジックからでは把握できないことが数多くあります。これらのコメントについては積極的に残していきましょう。保守や仕様変更の際に大きな助けとなります。

- 仕様の問題
- 実装の意図
- 過去の経緯
- 修正の注意点

```typescript
// ファイルの取得結果は配列で取得されるが、原則1ファイルしか配置されていない仕様である。
// そのため1ファイル以外をエラー処理とし、配列1番目のデータを返す。
const searchWord = 'sales_20230701';
const files = await storage.getFiles(searchWord);

if (files.length === 0) {
  throw new Error(`${searchWord}のファイルは見つかりませんでした`);
}
if (files.length > 1) {
  throw new Error(`${searchWord}のファイルが複数見つかりました`);
}

return files[0];
```

コメントがないと`for`から`forEach`に変える人もいるかもしれません。
理由があってその実装をしているのに崩されたらたまったものではないです。
```typescript
// 並列処理を行う場合、apiのアクセス制限に引っかかる場合があるため直列処理で実行する
for(let i = 0; i < records.length; i ++) {
  /* 中略 */
}
```

## まとめ

書籍内としては僅か10ページ程だったのですが、かなり重要なことが書かれています。普段何気なくコメントを書いてはいたものの、今回触れた内容をほとんど意識していませんでした。

今回輪読会で取り扱うテーマとしてチーム内で共有できるので、全体の意識として取り掛かっていこうと思います。