この記事は、社内向けに書いた資料を公開の許可を得て加筆修正したものです。
記事中の具体例やサンプルコードはJavaScript/TypeScriptで書かれていますが、内容自体は言語にかかわらず使えます。

同僚の @shisama にも手伝っていただきました。

2020/11/02 ついに完結！

はじめに

読みやすいコードは、コメントがなくても文章を読むように理解できます。
逆に、読みにくいコードはコメントがあってもさっぱり意味がわかりません。

文章を読むように理解できるコードを書くために普段気をつけていることや、コードレビューの際に重点的に見ているところをまとめました。

普段から読みやすいコードを心がけている方にとっては何も目新しい物はなく、当たり前に意識しているであろうことばかりです。

特にプログラミングを始めたばかりの方は参考にしてみてください。

命名について

読みやすいコードを書く第一歩は、わかりやすい名前をつけることです。
わかりやすい名前とは、具体的には以下のようなものです。

役割がわかる名前
誤解を招かない名前

変数名・定数名・プロパティ名

以下のことがわかるような名前をつけましょう。

どんな情報が入っているか？
- ユーザーIDか？ユーザー名か？それらを含むユーザー情報オブジェクトか？
- 「画像サイズ」は画像の縦横サイズ？画像ファイルのサイズ？
- 入っている情報がわかれば、型も容易に推測できます。
単位は何か？
- 例えば時間の場合、日か時間か秒かミリ秒か（それともYYYYMMDDの文字列かDateクラスのインスタンスかMoment.jsのインスタンスか）曖昧になりがちなので、はっきりわかる名前にしてください。
- データサイズはバイトかキロバイト[KB]かメガバイト[MB]か、あるいはキビバイト[KiB]かメビバイト[MiB]か曖昧になりがちです。特にKB/KiB, MB/MiB。

悪い例

data
- あらゆる情報はデータなので、これだけでは何が入っているのかわかりません。
- 一時的に使う、スコープが数行程度の変数（ソート用の比較関数の引数等）であればあまり問題にはなりませんが、処理の根幹に関わる変数がこんな名前だと混乱の元です。
user
- どんなユーザー情報が入っているのかわかりません（ユーザーID？ユーザー名？{id: 1, name: "John Doe"}といったオブジェクト？）
- 唯一許されるのは、Userクラスのインスタンスの場合のみです。
file
- どんなファイル情報が入っているのかわかりません（ファイル名？拡張子を除いたファイルタイトル？ファイルサイズ？）
- 唯一許されるのは、Fileクラスのインスタンスの場合のみです。
startTime
- 時間をどんな形で持っているのかわかりません（Dateオブジェクト？YYYYMMDD形式の文字列？Unix時間？）
- Unix時間だとしても、単位が秒なのかミリ秒なのかわかりません。

いい例

userId
- ユーザーIDだとわかる
- 重複のない一意の値だとわかる
- 多分数値型じゃないかな
startTimeUnixMilliSec
- Unix時間（数値型）だとわかる
- 単位はミリ秒だとわかる

ただし、あまり細かくやりすぎると長くなるのも事実なので（上のstartTimeUnixMilliSecも長いですね）、文脈から明らかにわかる情報は削っても構いません。
（明らかにわかる情報という条件つきですが）

否定形はほどほどに

名前に否定形はあまり入れないほうがいいです。
例えばこんなコード。

const doesNotHide: boolean = ...;
if (!doesNotHide) {
    // 何か処理
}

**何か処理するのは表示するとき？しないとき？**と混乱しますね。

特に"hide"自体が「隠す（表示しない）」という否定の意味を含んでいるので、**「表示しなくなくない」**でさらにややこしくなります。

目的を名前にする

うっかりすると、「手段」を名前につけてしまうことがあります。
後から手段が変わったときに実態と異なる名前になってしまい、バグのもとです。

名前には「目的」をつけましょう。

// NG: これ意味ある？
const ONE = 1;

// 「赤色のカラーコード」にわかりやすい別名をつけているだけならOK: 赤色を保持すること自体が目的
// 「何かのテーマカラー」として使う場合はNG: 目的はテーマカラーを格納することで、今はそれが赤色なだけ
// （後から「やっぱりテーマカラーは青のほうがいいな」と思ったら `RED = "#0000ff"` と書く羽目になる）
const RED = "#ff0000";

// テーマカラーを使うならこのような定数（例：強調時の文字色）に入れてから使う
// 後からテーマカラーを変更する場合でも COLOR_FOREGROUND_STRONG という定数の役割は変わらないので値だけ変えればいい
const COLOR_FOREGROUND_STRONG = RED;

「自分は『テーマカラーに RED 』みたいなアホな書き方しないよ！」という人、 CSSのクラス名やID名に left-contents とか sidemenu みたいな名前を付けていませんか？

これがメンタリズムです。

関数名

以下のことがわかるような名前をつけましょう。

何をしているか？
引数はどう使うか？
どんな情報を返すか？
将来の拡張や関数の追加に対応できるか？

悪い例

user()
- ユーザーをどうにかする関数だろうと推測できますが、単一ユーザーを取得するのか、複数ユーザーを取得するのか、ユーザー数を数えるのか、ユーザー情報を更新するのか、ユーザーを削除するのか、有効なユーザーかどうかを調べるのか全くわかりません。
findUsers(name)
- ユーザーを絞り込む関数だろうと想像できますが、関数名だけでは「名前で」絞り込むことを推測できません。
- 将来、「ユーザーを所属グループで絞り込む関数」を追加するかもしれません。名前で絞り込む頻度が他の絞り込み方法に比べて圧倒的に高いとかでない限り、findUsers()という「特等席」を与えるべきではありません。
findGroupCalledByUser(id, user)
- ユーザーの所属グループを取得するのだろうと想像できますが、ユーザーオブジェクト（？）から呼ばれるかなんて情報は不要です。後で他の場所からも呼びたくなったら名前変えますか？
- 関数名は何をやっているかが重要で、どこから呼ばれるかという情報は不要です。

いい例

countUsers()
- ユーザー数を取得する関数だとわかる
- 戻り値はユーザー数（数値型）だとわかる
findUsersByName()
- ユーザーを名前で絞り込む関数だとわかる
- 引数は絞り込む名前だとわかる
- 戻り値はUserクラスのインスタンスの配列だとわかる
user.isAvailable()
- 有効なユーザーかどうかを返すメソッドだとわかる
- 戻り値はboolean型で、有効であればtrueだとわかる
group.hasUser()
- グループにユーザーがいるかどうかを返すメソッドだとわかる
- 戻り値はboolean型で、ユーザーがいればtrueだとわかる
user.removed()
- ユーザーが削除されているかどうかを返すメソッドだとわかる
- 戻り値はboolean型で、削除されていればtrueだとわかる
- isRemoved()でも問題ありませんが、removed()だけでも通じます。
- ただし、同時にremove()メソッドもあったりすると紛らわしいので取り扱い注意。

このように、関数は「何かをするもの」なので、原則として動詞を頭につけてください。
「●●かどうか」を返す関数（boolean型）は、上記のように三人称単数現在形や過去分詞を使いましょう。

便利な動詞たち

関数名によく使われる動詞です。
「その関数が何をやっているか、何を返すか」がわかりやすいものを選んでください。

取得系

get
- 取得する
- 万能感があります。安易に飛びつかず、以下のfetch / find / searchといった、より限定的な意味の動詞を使うことを検討してください。
fetch
- （別の場所から）取ってくる
- DBやネットワーク越しに何かを取得する場合に使います。
find
- 探す
- 複数の候補の中から絞り込む場合に使います。
- 例: findUserById() - 複数のユーザーの中から、指定のIDに一致するユーザーを取得
search
- 探す
- findは候補の中から絞り込むのに対して、searchは対象の中を探します（特定の名前を持つファイルを「複数のファイル」から探す場合はfind、「ディレクトリの中で」探す場合は search ）
list
- 複数（0以上）取得する
- listを名詞として扱い、getListOfUsers()のような名前（ユーザー一覧を取得）でも間違いではないと思いますが、listUsers()（ユーザーを列挙する）のほうが簡潔に書けます。
- これも万能感があるので、findUser()であれば単一のユーザー情報を、findUsers()であれば複数の情報を返すという方法でもいいと思います。
count
- 条件に合うものの数を数える
- countを名詞として扱い、getCountOfUsers() のような名前（ユーザーの数を取得）でも間違いではないと思いますが、countUsers()（ユーザーを数える）のほうが簡潔に書けます。

（2019/01/18追記）
エゴサしてたらこんなはてブコメントを見つけました。

fetch,find,searchの説明は違和感。何より、howよりwhatと言うのであれば、これら全てfindでいい事になる。

fetchについては、確かにHowの意味を含むので「どこかから取ってくる」と明示したい場合にのみ使うべきです。
例えばFetch APIやDB操作のようなライブラリが該当します。

find/searchについては、関数の役割次第です。
特定のディレクトリからパターンにマッチするファイルを探す関数であればsearchFilesByPatternInDirectory(glob, dirName)、システム全体（あるいはアプリケーションが関知している領域全体）から探すならfindFilesByPattern(glob)のような名前がよさそうです。

「どこから」「どうやって」という情報が必要ないなら指摘の通りfindで問題ありません。

作成系

create
- 無から有を作る場合に使います。
- 例: createUser() - 新規ユーザーを作成
build
- 材料を組み立てて「構築する」場合に使います。
- 例: buildQuery() - 引数として渡した条件等からSQL文字列を構築（queryという名前はSQLなのかURLのクエリストリングなのか曖昧な場合があるので、文脈上明らかな場合にのみ使ってください）
generate
- 変換や計算等で「生成する」場合に使います。
- （正しい使い方かわかりませんが、個人的には）長方形の面積のように一回こっきりしか計算しないものではなく、擬似乱数やフィボナッチ数列のような次々に生成するものに対して使うイメージです（そもそも「面積を生成」って変ですね）
- 例: generateId() - IDを生成する（コールするたびに違うIDが生成される）

更新系

update
- 特定のデータを更新する場合に使います。
- 例: updateUserName() - ユーザー名を更新する
- ↑ただの例なので、「関数じゃなくてメソッドにしろよ」というツッコミは勘弁してください…
save
- データを永続化する場合に使います。
- 例: saveUser() - ユーザー情報を（DB等に）永続化する

削除系

delete
- データやリソースの削除に使います。
- 例: deleteUser() - ユーザー情報を削除する
- 一部の言語（JavaScriptやC++等）ではdeleteが予約語として存在しているので、delete()という関数名は作れません。
remove
- データやリソースの削除に使います。
- 例: removeUser() - ユーザー情報を削除する
- deleteとの違いは以下のとおりです。
  - delete - 削除する。削除されたものはもはや存在しない。
  - remove - （一覧から）取り除く。取り除かれたものはまだどこかに存在しているかもしれない。
  - 例えば、jQuery.remove()やJavaのList.remove()はDOMツリーやリスト内から要素を削除しますが、その際に削除された要素を返します。つまり、要素は完全に消滅したわけではなく、単に「取り除かれた」だけで、まだメモリ上のどこかに存在しています。これらのメソッド名がdelete()だったらネイティブからみて違和感を覚えるかもしれません。

動詞を使わない関数名

上で、このように書きました。

関数は「何かをするもの」なので、原則として動詞を頭につけてください。

原則には例外がつきもので、時には動詞を使わない関数名やメソッド名をつけることがあります。
JavaやJavaScriptの文字列にあるcharAt()やindexOf()などがその例です。

これらは関数名と引数とつなげてフレーズにしているという特殊な命名方法です。

charAt(0) は "char(acter) at 0" 、つまり「0番めの文字」
indexOf('a') は "index of 'a'" 、つまり「'a'のインデックス」

こういう名前を自然に作れるようになると玄人っぽいですが、一歩間違えると意味が伝わらなかったり誤解されたりするのでご利用は計画的に。

また、単なるプロパティや属性のgetter/setterでもわざわざ動詞をつけることはないかもしれません。

class Rectangle {
    private width_: number;
    private height_: number;

    constructor(width: number, height: number) {
        this.width_ = width;
        this.height_ = height;
    }
    get width(): number {
        return this.width_;
    }
    get height(): number {
        return this.height_;
    }
}

HowではなくWhatを

関数を使う側にとって必要なのは、その関数が「何をするか（What）」であって、「どうやってするか（How）」ではありません。言い換えると、内部実装の情報まで関数名に含める必要はありません。

むしろ内部実装まで関数名に含めてしまうと抽象度が下がり、将来実装が変わった時に誤解されてバグの元になります。

// ユーザー情報
class User {
    // ...
}

/**
 * 悪い例: ユーザー情報をMySQLから取得する
 * 使う側にとっては内部実装（どこから取得するか）はどうでもいい
 * 将来取得先を変えた場合（PostgreSQLとかHBaseとか、あるいはマイクロサービス化して外部APIから取得とか）に整合性が取れなくなる
 */
export function findUserFromMySQL(id: number): User | null {
    // ...
}

/**
 * いい例: ユーザー情報を取得
 * 使う側からは「取得する」ことだけわかればいいので、内部実装の情報は関数名には含めない
 */
export function findUser(id: number): User | null {
    // たとえばMySQLから取得
}

もちろん、呼び出し元が内部実装を知る必要がある場合は別です。
その場合でも、ほとんどが内部的に使われる関数に限られるので、exportする必要はまずないはずです。

内部実装を関数名に含める必要がある例

// ユーザー情報
class User {
    // ...
}

/**
 * ユーザー情報を取得
 * キャッシュにあればそれを取得し、なければRDBから取得
 */
export function findUser(id: number): User | null {
    const user = findUserFromCache(id);
    if (user !== null) {
        return user; // キャッシュにあった
    }

    // キャッシュにないのでRDBから取得
    return findUserFromRdb(id);
}

/**
 * キャッシュからユーザー情報を取得
 * 内部実装の情報（どこから取得しているか）が呼び出し側で必要なので関数名に含めている
 */
function findUserFromCache(id: number): User | null {
    // たとえばmemcachedから取得
}

/**
 * RDBからユーザー情報を取得
 * これも内部実装の情報が呼び出し側で必要なので関数名に含めている
 */
function findUserFromRdb(id: number): User | null {
    // たとえばMySQLから取得
}

条件式について

読みにくいコードにはまず間違いなく複雑な条件式があります。
プログラムの規模が大きくなったり高度なアルゴリズムを使ったりすると複雑な条件が出てくるのは避けられませんが、条件が複雑ということと条件式が複雑ということはイコールではありません。

条件が複雑でもわかりやすい条件式を書くことはできます。

条件式は関数化して「目的」がわかるようにする

例えば、以下のような条件式は何をしたいのか即断できません。

if (year % 4 === 0 && year % 100 !== 0 || year % 400 === 0) { // 何を判定したい？？
    // 何か処理
}

これはyearが閏年かどうかを判定しているのですが、=== !== && ||が入り乱れているので非常にわかりづらい条件式です。

ここで重要なのは、本来の目的は**yearが閏年かを判定したい**のであって、「yearが4で割り切れるが100で割り切れない、又は、400で割り切れるか」を判定したいのではない、ということです。

「4で割り切れるが…」は手段です。手段を条件式に書くと読みづらいコードになります。

ifの中には手段（どうやって判定しているか）ではなく、目的（何を判定したいのか）を書きましょう。

if (isLeapYear(year)) { // 閏年かどうかを判定したいとわかる
    // 何か処理
}

function isLeapYear(year: number): boolean {
    return year % 4 === 0 && year % 100 !== 0 || year % 400 === 0;
}

単に関数化しただけですが、単体テストもやりやすくなりますし、格段に読みやすくなったとおもいませんか？

複雑な条件は分解する

上のisLeapYear()は関数化して全体の見通しはよくなったものの、関数の中身はまだ読みづらいままです。
しかし、条件部分が関数化してあれば読みやすくするのも簡単です。

読みやすくするために、条件式を分解しましょう。
分解のコツは、真偽が確定したらその時点でreturnすることです。

function isLeapYear(year: number): boolean {
    // 元の条件式: year % 4 === 0 && year % 100 !== 0 || year % 400 === 0

    if (year % 400 === 0) {
        // 400で割り切れる年は閏年
        return true;
    }
    if (year % 100 === 0) {
        // 400で割り切れず、100で割り切れる年は平年
        return false;
    }
    if (year % 4 === 0) {
        // 400でも100でも割り切れず、4で割り切れる年は閏年
        return true;
    }

    // それ以外は平年
    return false;
}

これでだいぶわかりやすく、ロジックにバグがあってもすぐに気付けるレベルになりました。

関数について

読みやすい関数は流れが追いやすいものです。
そして、使いやすい関数は役割がわかりやすいものです。

どうすれば読みやすく使いやすい関数を作れるのかを見ていきましょう。

神＝悪

いろいろな引数を取り、いろいろな役割をする関数。それが神関数です。
一見便利ですが、1つの名前で複数の役割があると使う側はいろいろな引数のパターンや役割を覚えなくてはいけないうえに、何よりややこしいので作るのはやめましょう。

神は悪です。

代表的な神がjQuery()で、これ1つで

文字列でセレクタを渡すと、該当するDOMをjQueryでラップして返す
文字列でHTMLを渡すと、新たに作った要素をjQueryでラップして返す
DOM要素を渡すと、jQueryでラップして返す
jQueryオブジェクトを渡すと、クローンしたものを返す
プレーンオブジェクトを渡すと、指定のプロパティを含むjQueryオブジェクトを返す
引数を渡さないと、空のjQueryオブジェクトを返す
関数を渡すと、DOM構築が完了したタイミングで呼び出す（readyハンドラ）

「検索」「生成」「ラップ」「コピー」さらに「イベント処理」までこなしてくれる全能の神。
でも使う側は混乱してしまいます。神の領域は人間には早すぎる。

特に文字列の内容によって検索したり生成したり挙動が変わるので、思わぬバグを埋め込んでしまうことがあります。

jQueryはとても便利なライブラリで、お世話になったことのある人も少なくはないと思いますが、こういう使いにくさもあったりします。

誤解しないでいただきたいのですが、関数のオーバーロードをやめましょうと言っているわけではありません。
例えば引数に時間を受け取る関数で、DateオブジェクトでもUnix時間でもISO8601形式の文字列でも受け付けるようにするなら個人的には許容範囲だと思っています。
形は違えど時間情報という本質は変わらないので、混乱もあまりないでしょうから。

jQueryの例でも、「セレクタ文字列またはDOMを受け取る関数」程度であればまだわかりやすかったのではないでしょうか。

ネストは浅く。ただし本質は別にあり。

ネストが深いコードは読みにくいとよく言われます。
こんなコードを考えてみましょう。

ネストが深いコード

/**
 * ユーザーをグループに紐付ける
 * 戻り値: OK/NG
 */
function associateUserWithGroup(userId: number, groupId: number): boolean {
    const user = User.findById(userId);
    if (user !== null) {
        const group = Group.findById(groupId);
        if (group !== null) {
            const result = user.associateWith(group);
            if (result) {
                // OK!
                return true;
            } else {
                return false;
            }
        } else {
            return false;
        }
    } else {
        return false;
    }
}

はい。たしかに読みにくいですね。ただし、問題の本質――つまり読みにくい理由――はネストの深さではありません。
正常終了時の流れ、つまり処理の本流がわかりづらいから読みにくいのです。

上の例でいうと、本流はユーザーをグループに紐付けるところですが、こんなに大事な処理が分岐の分岐の中に書いてあります。
分岐は枝分かれです。枝の先に本来の処理を書くべきではありません。

本流は**「枝」ではなく「幹」に書きましょう**。

本流が明確なコード

/**
 * ユーザーをグループに紐付ける
 * 戻り値: OK/NG
 */
function associateUserWithGroup(userId: number, groupId: number): boolean {
    const user = User.findById(userId);
    if (user === null) {
        return false;
    }
    const group = Group.findById(groupId);
    if (group === null) {
        return false;
    }
    const associated = user.associateWith(group);
    if (!associated) {
        return false;
    }
    // 今後さらに判定条件を追加する可能性を考えてこうしているが、そうでないなら return user.associateWith(group); だけでもOK
    return true;
}

本流がどっしりと構えてあり、例外的な処理を枝分かれで処理しているので、流れが追いやすいですね。

`else`はあまり使わない

上の2つのコードを見比べるとわかると思いますが、elseは処理の流れを真っ二つに分けるために読みづらくなります。そのため、読みやすいコードにelseはあまり出てきません。
個人的にもifはしょっちゅう書きますが、最近elseを書いた記憶は数えるほどしかありません。

だからといって「elseは不要！」などと言うつもりは毛頭ありません。逆にelseがないと読みづらい場合もあります。
詳しくは次節で説明します。

本流が2つ以上ある場合は`else`を使う

上でこんなことを書きました。

elseは処理の流れを真っ二つに分ける

逆に言えば、処理の流れを真っ二つに分ける必要があるとき、つまり本流が2つあるときはelseが必要ということです。

例えば何かの値が奇数なら処理A、偶数なら処理Bと分ける場合や、A/Bテストを実装する場合などです。

そんなときに以下のようなコードを書くとどうなるでしょうか。

奇数・偶数で処理を分ける

function doSomething(value: number): void {
    if (isOdd(value)) {
        // 処理A
        return;
    }
    // 処理B
}

これだと、処理Aが傍流で処理Bが本流のように見えてしまいます。

2つの処理の重み（長さではない）が同等でどちらも本流の場合は、elseを使って真っ二つに分けましょう。

奇数・偶数で処理を分ける（if-elseバージョン）

function doSomething(value: number): void {
    if (isOdd(value)) {
        // 処理A
    } else {
        // 処理B
    }
}

これで本流が2つあることが伝わりやすくなりました。

付け加えるなら、処理A,Bも関数化すれば条件式部分がコンパクトになり、より流れを追いやすくなります。
さらに、処理の中でさらに分岐が発生してもネストが深くならない等、いいことづくめです。

目的ごとに関数化する

関数の機能はできるだけシンプルにしましょう。一言で説明できるような関数がベストです。
機能がシンプルであれば、組み合わせて使うのも簡単です。

「指定のユーザーが所属しているグループの中で1ヶ月以上活動していないグループから脱退（紐付け削除）し、さらに別のグループにイベントを作成」なんて関数を作ったら読みにくくて仕方ない上に、名前をつけるのにも一苦労です。

個人的には、しっくりくる名前をつけられない関数は設計が間違っているとすら考えています。

変数・定数について

プログラムの大部分は変数の操作です。
変数の使い方を間違えるとバグに直結するので、 間違えにくい変数の使い方 をぜひ覚えてください。

手品をしない

何のことかというと マジックナンバー の話です。

プログラム内でいきなり出てくる複雑なリテラル がマジックナンバーです。

手品をしている例

let priceBase = 123;
let priceWithTax = Math.floor(priceBase * 1.08); // なんだこの1.08は…？

いきなり 1.08 というマジックナンバーが出てきました。
このレベルなら 1.08 という数値と priceBase / priceWithTax という定数名から「消費税のことだろうな」と推測できますが、世の中には推測が難しいマジックナンバーはたくさんあります。
「目的を名前にする」で出てきたカラーコードなんかも直接書かれるとわけがわかりませんよね。

そして、将来消費税率が変更されたときに 1.08 が出てくる場所をすべて変更しなければいけません。
単純な文字列置換では消費税と関係ない 1.08 も変更してしまうかもしれませんし、もし乗算以外の場所で 0.08 という値や 8 / 100 を使っていたら置換漏れによるバグを埋め込んでしまいます。

マジックナンバーはやめて、定数シンボルを使いましょう。

手品をしない例

const TAX_RATE_CONSUMPTION = 0.08; // 消費税率

let priceBase = 123;
let priceWithTax = Math.floor(priceBase * (1 + TAX_RATE_CONSUMPTION));

さらに言うなら、こういった定数シンボルは 設定ファイルとして一箇所にまとめましょう 。
仕様変更のときに「この値はどこに入っていたっけ？」とあちこち探し回らずに設定ファイルを見るだけで済みます。

※そもそも税込価格はこんなところでチマチマ計算式を書くのではなくて「税込み価格を計算する関数」を作るべきです。
今回はマジックナンバーの例として出しているだけなので、あまりそのへんには突っ込まないでください。

グローバル変数は極力避ける

どの関数からでもアクセスできる変数が グローバル変数 です。
それに対し、特定の関数内からしかアクセスできない変数は ローカル変数 です。

グローバル変数とローカル変数

// グローバル変数: どこからでもアクセスできる
let globalVariable: number = 0;

function foo(): void {
    // ローカル変数: この関数からしかアクセスできない
    let localVariable: number = 0;

    // グローバル変数にはアクセスできる
    globalVariable = 1;
}

function bar(): void {
    // ローカル変数: foo()内の同名の変数とは別物
    let localVariable: number = 1;

    // グローバル変数にはアクセスできる
    globalVariable = 2;
}

// エラー: 外からローカル変数にはアクセスできない
localVariable = 2;

変数は 極力ローカル変数を使いましょう 。

グローバル変数は どこからでもアクセスできるので便利 な反面、 どこからでもアクセスできるので危険 です。

どこからでもアクセスできる ということは、 どこからでもバグを入れられる ということです。
ローカル変数は 限られた場所からしかアクセスできない ので、 限られた場所からしかバグを入れられません 。
そのため、断然ローカル変数のほうが安全です。

そして、より重要なことは、言語やアーキテクチャによっては 安易なグローバル変数が別の事故を生む可能性があります 。
例えばマルチスレッドプログラミングでグローバル変数を安易に使うとややこしいバグを入れてしまう可能性がありますし、PHPでグローバル変数をホイホイ使っていると、Node.jsに移行したときに同じ感覚で使ってしまい、 確実にハマります 。

変数のスコープは短く

「グローバル変数は極力避ける」に関連しますが、変数のスコープ（生存期間、アクセス範囲）は短くするのが鉄則です。
スコープを短くするとそれだけバグが入り込む可能性が減るのは上で書いたとおりですが、それ以上に 一度に覚える必要のある変数が減るため、理解しやすくなります。

また、関数全体で使っている変数が多い場合でも、局所的に見ればごく一部しか使っていないことはよくあります。

覚える変数が多い例

function foo(): void {
    let a = 0, b = 1, c = 2, d = 3, e = 4;

    // ①ここではa, bしか使わない
    ...

    // ②ここではc, dしか使わない
    ...

    // ③ここではeしか使わない
    ...    
}

この場合、①で c, d, e へのアクセスがあってはならないため、その確認が必要です。
そして c におかしな値が入っていた場合、②の中身はもちろん、②以外で c へのアクセスがないかも確認しなければいけません。

このような場合は ブロックスコープ を使うとか、あるいはそこだけ 別関数に分離 してしまいましょう。

覚える変数が少ない例

function foo(): void {
    {
        // ①ここではa, bしか使わない
        let a = 0, b = 1;
        ...
    }
    {
        // ②ここではc, dしか使わない
        let c = 2, d = 3;
        ...
    }
    {
        // ③ここではeしか使わない
        let e = 4;
        ...
    }
}

このようにすれば、①では そもそも c, d, e にアクセスしようがない ため、確認自体が必要ありません。
そして c におかしな値が入っていた場合も、②のブロックのみを確認すればOKです。

何より、関数内のどの場所でも 一度に覚える変数は2つ以下 なので脳にもやさしく、関数分割によるリファクタリングもやりやすくなります。

JavaScriptでvarで変数を定義している場合はブロックスコープが一見使えそうだけど実際は使えないので要注意です。letを使いましょう。

言語によってはブロック単位の変数定義ができないものもあるので、その場合は関数分割しましょう。

「関数は意味のあるまとまりごとに区切るべきであって、変数のスコープを目的とするべきではない」という意見もあると思います。
それ自体に異論はありません。
しかし、実際のコードでは変数のスコープが意味のあるまとまりであることが非常に多いので、変数のスコープと意味のあるまとまりは密接に関係があります。

コメントについて

いよいよ最終章です。
コメントはただ書けばいいというものではありません。コメントの書き方をほんのわずか覚えておくだけで、あなたのコードは格段にわかりやすくなります。

やっていることではなく、やりたいことを書く

しばしば、こんなコメントがプログラマーの間で「意味のないコメント」としてネタに上がることがあります。

意味のないコメントの例

function foo() {
  let a = 1; // aに1を代入する
  a++;       // aをインクリメントする
}

なぜ意味がないかというと、コードを読めばわかることしか書いていないからです。
言い換えるとコードをコメント化しただけで、何のためにその処理が必要なのかが全くわかりません。

1にはどんな意味があるのか？
インクリメントの意図は何か？
この関数は何をするものなのか？

このように、コメントには**「やっていること」ではなく「やりたいこと」を書きましょう**。
そうすれば、やりたいこと（コメント）とやっていること（コード）が違っていればすぐにバグだとわかります。

文章のようなコードを書く

この記事の冒頭の伏線をようやくここで回収します。

以下の2つの関数は、これまでにサンプルコードとして書いてきたものです。果たしてこれらのコメントは意味があるでしょうか？

/**
 * 閏年の判定
 * @param year 判定する年
 * @returns Yes/No
 */
function isLeapYear(year: number): boolean {
    if (year % 400 === 0) {
        // 400で割り切れる年は閏年
        return true;
    }
    if (year % 100 === 0) {
        // 400で割り切れず、100で割り切れる年は平年
        return false;
    }
    if (year % 4 === 0) {
        // 400でも100でも割り切れず、4で割り切れる年は閏年
        return true;
    }

    // それ以外は平年
    return false;
}

/**
 * ユーザーをグループに紐付ける
 * @param userId ユーザーID
 * @param groupId グループID
 * @returns OK/NG
 */
function associateUserWithGroup(userId: number, groupId: number): boolean {
    // ユーザーIDからユーザーインスタンスを取得
    const user = User.findById(userId);
    if (user === null) {
        // 取得できなければ失敗
        return false;
    }
    // グループIDからグループインスタンスを取得
    const group = Group.findById(groupId);
    if (group === null) {
        // 取得できなければ失敗
        return false;
    }
    // ユーザーをグループに関連付ける
    const associated = user.associateWith(group);
    if (!associated) {
        // 関連付けできなければ失敗
        return false;
    }
    // 関連付けできたら成功
    return true;
}

これらコメントも、先程と同じようにコードを読めばわかることしか書いていないので意味がありませんね。
（associateUserWithGroup()の戻り値の情報は関数名だけではわからないので、その部分だけはコメントの価値があります）

しかし、前項の「意味のないコメントの例」に出てきたコード・コメントとは質が全く違います。

前項の例はコメントがコードをなぞっているだけで何をしたいのかわからないからコメントの意味がなかったのですが、今回はコードだけで十分に意図が伝わるからコメントの意味がないのです。

変数名や関数名がわかりやすく、処理の流れが追いやすければ、コメントがなくても十分にわかりやすいコードを書けるということがおわかりいただけたと思います。

これが、冒頭で書いた文章を読むように理解できるコードです。

コードをコメント化するのではなく、コメントをコード化することでコメントの意味がないほど自明なコードになります。これがわかりやすいコードの究極形です。

前提知識を書く

「やりたいこと」すらコードで表現できた今、コメントには何を書くべきでしょうか。
逆に言えば、コードで表現できない情報、例えば前提知識のような情報はコメントで書くべきです。

例えば、閏年判定における以下のような情報は前提知識やアルゴリズムとしてコメントに書いてもよいでしょう。

一年の長さは365.24218944日
これを365.2425日（365 + 1/4 - 1/100 + 1/400）と近似する
この近似により、普段の年は1年を365日、以下を満たす年は366日とする
- 4で割り切れるが100で割り切れない年
- 400で割り切れる年
上記の条件を満たす年を閏年、それ以外の年を平年と定義する

繰り返しますが、コメントがなくても伝わるコードがベストです。
しかし、前提知識やアルゴリズムの詳細などはどうしてもコードだけでは表しにくいので、そういう情報こそコメントで適切に伝えましょう。

参考

リーダブルコード - 読みやすいコードを書くための必読書です。