コードレビューでズタボロだったので『リーダブルコード』を読み返してみた

前提：動くものは作れた、ただしそれは動くだけだった

こんにちは！mballです。
先日転職をしたのですが、一つ大きな変化がありました。それはレビュー文化です。以前の会社はレビューを行わず、機能が完成したらとにかくマージ！といった具合だったのです。動けばOKでした。今思えば可読性も保守性もない汚いソースコードです。

現在はレビューを受けてからマージをしています。エンジニアになって約1年、レビューをほとんど受けずに書いてきた結果、たった100行の変更に10個以上コメントがつきました。こんなに自分はできなかったのか。。。

流石に自分のレベルの低さに引いたので、『リーダブルコード』 を流し読みしました。いくつかすぐにできそうなアクションを項目にして、その項目を忠実に守ったらレビューでの指摘がぐっと減りました。

そこでコードレビュー前に最低限確認すべき観点を『リーダブルコード』から抜粋して（自分に対しての戒めの意味を込めて）ご紹介させてください。

1.命名に気をつけましょう

関数名と変数名は短いか

3~5単語 がベストです。
英単語がわからなかったら、日本語を翻訳アプリで英語にしましょう。
個人的にオススメなのはDeepl翻訳が良さげです。 https://www.deepl.com/ja/translator
※ 注意： DeepL翻訳をはじめとしたテキストコピペ系Webサービスは機密情報の扱いに注意しよう

fooやvalのような汎用的な名前を使用していないか

fooやvalは考えなくても頭に浮かぶので楽ですよね。僕もたまにやってしまっていました。
ただし、こんな命名をしたら、その変数には何が格納されているか、処理を追わなければわかりません。
具体的な名前を使いましょう。

予約語を使用していないか

予約語とは、プログラミング言語などの人工言語の仕様に定められている、開発者が付ける識別名として利用できない文字列のこと。予約語に挙げられた単語やフレーズは変数名や関数名などに使用することはできない。
参照：https://e-words.jp/w/%E4%BA%88%E7%B4%84%E8%AA%9E.html

予約語を使用していると、システムでたまにバグが起こります。
回避するために「(言語名orフレームワーク名) 予約語」でググりましょう（例：「JavaScript 予約語」）。

参考：https://riptutorial.com/ruby-on-rails/example/32446/reserved-word-list
参考：https://developer.mozilla.org/ja/docs/Web/JavaScript/Reference/Reserved_Words

スタイルガイドに沿った命名になっているか

これは必須ではないですが、チームで取り決めがない限りはスタイルガイドに沿うのが一般的でしょう。
「(言語名orフレームワーク名) スタイルガイド」などで調べて命名を見直してみましょう。（例：「Rails スタイルガイド」）

参考：【保存版】Rubyスタイルガイド（日本語・解説付き）総もくじ

2.見た目の美しさに気を配りましょう

インデントの数は合っているか

Node.jsにおいてのprettierなど、formatterを入れましょう。
参考：https://prettier.io/

スペースの入り方は統一されているか

VSCodeのショートカット「option + shift + f」ですぐに実現できます。
参考：Visual Studio Code キーボード ショートカット

並びに一貫性はあるか

以下のコードを見比べてみてください。

ONE = 1
TWO = 2
THREE = 3
FOUR = 4
FIVE = 5
SIX = 6
SEVEN = 7
EIGHT = 8
NINE = 9
TEN = 10
ELEVEN = 11
TWELVE = 12
THIERTEEN = 13

と

ONE       = 1
TWO       = 2
THREE     = 3
FOUR      = 4
FIVE      = 5
SIX       = 6
SEVEN     = 7
EIGHT     = 8
NINE      = 9
TEN       = 10
ELEVEN    = 11
TWELVE    = 12
THIERTEEN = 13

ただ数字を変数に入れて格納しただけですので、違いはそうわからないかもしれないですが、見た目が整っているだけで、これをレビューする側のストレスは減らせるはずです。

3.コメントを書きましょう

処理内容をただ日本語にしていないか

短い処理を日本語で説明するくらいならコメントしないほうが良いです。
長い処理であれば、端的に何をしているかだけコメントするのが良しとされています。

意図をコメントしているか

何(What)をしているのか、は頑張ってコードから読み取れるとしても、なぜ(Why)このコードにしているのか、はコードから読み取れません。
意図を汲み取れるのはコメントだけ、ということを覚えておきましょう。

指示語は使っていないか

「その」「この」などの指示語を使うと、せっかくのコメントが無駄です。
コメントを書く理由の一つとして、コードを読むために考えさせないことが挙げられます。
しかし、「その」や「この」などの指示語を使うと、指示語が指している言葉は何かを考える必要が出てきてしまいます。
コメント書いているのに相手に頭を使わせてしまうなんて、勿体無いですよね？

4.ロジックを理解するために考えないコードにしましょう

比較の際は左側に変数を持ってきているか

if (n > 10) console.log('hogehoge');

比較したい対象(変数など)は左側に持ってきてあげましょう。

if (10 < n) console.log('hogehoge');

はこれくらいならすぐ分かりますが、複雑な式であれば理解に時間を要してしまいそうです。

原則として

• 左側：調査対象の値(変化する)
• 右側：比較対象の値(変化しない)

と覚えましょう！

条件を描くときは肯定系か

admin != false

admin == true

上記の二つは共に同じ意味合いです。否定の否定 = 肯定 ですが、理解するのに時間がかかってしまうため肯定形にしてあげましょう！

ネストを浅くしているか

ネストが深いと何しているのか途中で頭がオーバーヒートしてしまいます。こちらの記事にはいくつかの対応方法が載っているのでご参考ください。
参考：ネストの深さは闇の深さ

5.式を分割しましょう

説明変数にしているか

例えば params.user.values.type がユーザーのタイプ(管理者or無料会員or有料会員)を表していたとしましょう。
管理者だけが行える処理があったらおそらくこうなるでしょう。

if (params.user.values.type == 'admin') {
  // 管理者だけが行える処理
};

しかし、このコードを書いた人以外は params.user.values.type がユーザーのタイプを表しているかなど、すぐにわからないと思います。
そこで以下のように変更してあげましょう。

const userType = params.user.values.type;
if (userType == 'admin') {
  // 管理者だけが行える処理
};

どうでしょうか、最初より伝わりやすくなったかと思います。
説明するために変数へ格納すれば、このコードを読む人もすぐに理解できますのでお試しください！

要約変数にしているか

上記のコードはまだ直しがいがあります。

const userType = params.user.values.type;
const isAdmin = (userType == 'admin');
if (isAdmin) {
  // 管理者だけが行える処理
}

どうでしょうか、これならほとんどの方が一瞬で理解できるかと思います！
このように、ロジックの意味を変数にしてあげることで、より可読性の高いコードに近づきます。

横長の式を減らしたか

以下の二つの式はどちらが見やすいですか？

const Config = { DB_HOST: process.env.HOST, DB_USERNAME: process.env.USERNAME, DB_PASSWORD: process.env.PASSWORD, DB_DATABASE: process.env.DATABASE, DB_PORT: process.env.DEV_PORT};

const Config = {
  DB_HOST: process.env.HOST,
  DB_USERNAME: process.env.USERNAME,
  DB_PASSWORD: process.env.PASSWORD,
  DB_DATABASE: process.env.DATABASE,
  DB_PORT: process.env.PORT,
};

横長か縦長か、が違うだけでこんなにも違うのです。

変数の再代入をなくしているか

変数の再代入は悪、とまでは言いませんが、なるべく避けるといいでしょう。
理由に関しては参考になる記事を見つけましたので、そちらをご参照ください。
参考： 変数への再代入を避けたいのは何故か

一つの関数に処理は一つだけか

一つの関数に複数の処理をまとめるのは非常に保守性が低いです。
もし複雑だと少しでも感じたら

• まず処理の流れを書き出す
• 書き出した処理を一つ一つ関数に切り出す
• 元の処理では他の関数を呼び出すだけにする

の3STEPで修正するのをオススメします。

ここに挙げた項目ができていなかったらぜひ原著を

プログラミングを始めたての時期に『リーダブルコード』を初めて読みました。その頃は正直何言ってんだ、と思ってましたが、今読み返してみたら当たり前にこなしておきたいことばかり書かれていました。
この記事では行動ベースでしかお話しできていないので、その行動が必要な理由だったり背景だったりを知るために、ぜひ原著を読んでいただきたいです！
リンクはこちら→『リーダブルコード』

長文失礼しました！この記事がどなたかの助けになれば幸いです。