フロントエンド初心者が１年間で受けた指摘をまとめてみた【js編】

１年間にコードレビューで頂いたjsの指摘の中で、汎用性（？）の高そうなものをまとめてみました。

読みやすく保守しやすいコードの書き方は、（たぶん一番大事な部分なのに）独学では習得が難しいなと思いました。
受けた指摘も、読みやすいコードにするためにどう書くか？という指摘がメインです。

主観で★1（学習コスト０）〜★5（哲学）に分類してみてます。

もらった指摘

★1　学習コスト0

コミット前にconsole.logを消しておく

とはいえ、動作確認で必要になることも多いので、最悪レビュー前には消しておきたいくらいの気持ち。
複雑なコードを書いているときについつい消し忘れがち。
コミット前のgrepを癖付けると吉。

事情があってconsole.logを残したいとき（デモ環境などで確認が必要な場合）は、コミットメッセージに記載＆レビュー用のプルリクエストにコメントとして残すと、レビュアーや未来の自分に優しいです。

不要なコードはコミットに含めない

超当たり前ではあるのですが、やりがち
①単純に作業の消し忘れ
②既存のコード準拠で新しくコードを書いていくとき
のパターンがありました。

①に関しては一旦作ってみたけど使わなくなった変数や関数が残ることが多かったです。
eslintなどで検出するか、prettierで一気に修正してしまうといいかもしれません。

②は、既存コードをコメントアウトして残しておくと一見参考にできそうでいいように思えるのですが、
後々「どこを使ってて、どこを使ってないんだっけ…？」というのが分からなくなってしまうので、
利用しない・するかもしれない関数は最初からは入れず、段階に応じて持ってくるようにします。

コミット時点で使用していないものが混入してしまうと、読解コストが上がってしまうので気をつけたいですね…！（自戒）

改行・インデントが統一されていない

①改行に不要なスペースが入っている
②インデントがタブではなくスペースになっている
③改行が2行になっている

普通にド迷惑で恥ずかしい。けど初心者のうちはやらかしがち（だと思いたい。）
①と②は、VSCode拡張機能のindent-rainbowを入れると検知しやすさが爆上がりします。
③はeslintやprettierなどでゴリッと削ってしまいたいところですが、既存コードに追加など気軽に修正できないこともあるので、コミットを細切れにして、コミット前に確認すると良いかも。

★2　思いやり

スタイルに関するクラス名をjsからの参照に使わない

横着すると悲しみを背負います。

// 微妙なやつ

// html
<div class="header_title">ほげほげ</div>

// css
.header_title {
  font-size: 12px;
}

// js(jQuery)
$('.header_title').on('click', function() {
    ... //処理
  });

というコードにしてしまうと、

// 微妙なやつの成れの果て

// html
<div class="header_title_new">ほげほげ</div> // < スタイル変えるわ！

// css
.header_title_new { // < スタイル変えたわ！
  font-size: 12px;
}

// js(jQuery)
$('.header_title').on('click', function() { // < は？動かないんだが
    ... //処理
  });

という悲劇が起こるので、

// 良いやつ

// html
<div class="header_title js-header_title">ほげほげ</div>

// css
.header_title {
  font-size: 12px;
}

// js(jQuery)
$('.js-header_title').on('click', function() {
    ... //処理
});

分けると後々幸せになれます。

関数の引数を分かりやすく

// 微妙なやつ

const openTarget = (t, boolean) => {
  ... // 処理

↑よりは

// 良いやつ

const openTarget = ($target, isClosed) => {
  ... // 処理

こちらのほうが、引数の意味しているところが分かってちょっと読みやすくなります。

JSDocをちゃんと書く

TypeScriptを使えばそこそこ解決するのですが…。
引数としてどういう型を想定しているか？引数の内容はどんなもの（役割を持っている）か？などを書くと、未来の読解コストが下がります。

/**
 * [filterErrorCassette エラー状態のカセットのリストを返す]
 * @param {Object} $cassette 対象のカセット
 * @return {array} エラーカセットの配列
 */

varの巻き上げ（hoisting）に注意する

参考：やっとわかったjsの「巻き上げ」
なぜvarを使っているのかって？IEお前のせいだよ！

巻き上げ考慮して先に変数宣言するとき、ミスでglobalになってしまう可能性がある（varをつけずに宣言するとglobalになる）

const/letが使える人は使いましょう（涙）

テンプレートリテラルはIEでは動きません

一応constはIEでも使える場合がある（IE11では可）のですが、ES6の記法は基本的に動かないので注意ですね…。
ES6が使える環境からIE対応する環境に移ってくるとつい忘れがちです。

【jQuery】 $('.hoge')はコストが高いので複数回使う場合は変数宣言しておく

jQueryを高速に動作させるためのポイント５つ
クラス指定でDOMを取得する場合、jQuery内部では結構面倒なことをやっていたりします。
何度も同じDOMを使う場合は変数に入れておきましょう。
入社したての頃に一番言われたポイントかもしれないです。レビュアーさんありがとう…

★3　読みやすいコード

早期returnを使って関数をシンプルにする（ガード節）

// 微妙なやつ

if (hogehoge === true) {
  // 処理①
} else {
  // return;
}
  // 処理②

この書き方だと、こういう読み方になります

• hogehogeがtrueのときに処理①を行って…
• あ、それ以外は処理②やらないんだな

早期returnをすると…

// 良いやつ

if (hogehoge === false) {
  return;
}
  // 処理①
  // 処理②

• なるほど、hogehogeがfalseのときは後の処理はしないんだな
• falseでなければ処理①と処理②をするんだな

と、分岐を削れるため読むエネルギーが減ります！

!hogehoge.lengthよりhogehoge.length === 0のほうがよい

!を使うと（何の否定だろう…？）と「否定」であることを頭に置きながら読む必要があるので、単純に0やfalseなどと等価であることを示すほうが読みやすいです。

関数の起点となる（イベントをバインドする）クラスを付与する場合は、役割に応じた命名をする

// 微妙なやつ

// html
<button type="button" class="js-button">ポップアップ</button>

// js
$('.js-button').on('click', function() {
  ...

jsでclickなどイベントをバインドするクラス名は、上のように「もの」的なクラス名をつけるよりも…

// 良いやつ

// html
<button type="button" class="js-popup_open_trigger">ポップアップ</button>

// js
$('.js-popup_open_trigger').on('click', function() {
  ...

このように、「何をするのか」を元にクラス名をつけてあげると、クラスを見るだけで処理のイメージが付きやすくなってお得です。

可読性を高めるために変数を使用する

例えば、ウィンドウ幅が750px以下のときのみ実行したい関数があったときは
if (windowWidth < 750) { ...と書くよりも

const hasSmallerWindow = windowWidth < 750px;
if (hasSmallerWindow) {
  // 処理
}

とすると、750は狭いウィンドウ幅を判定するための数字なんだな…というのが分かりやすくなります。
とはいえ…

変数化しなくても可読性を損なわない場合もある

上で示した例は750の意味が単体では分からないので変数化したほうがよいのですが、
読む量が少なかったり、読んでちゃんと意味が分かるものについては変数化しなくても読みやすかったりするので、ケースバイケース（製作者のさじ加減…）です。

// 変数化してるけど…

const isInFixedAreaStatus = isScrollInRange(from);
togglePositionFixed({
    canDisplay: isInFixedAreaStatus,
    $target: $banner,
});

// 変数化しなくても読みやすい

togglePositionFixed({
  canDisplay: isScrollInRange(from),
    $target: $banner,
});

【jQuery】見えてるものだけ取りたい場合:visibleが使える

たまに出番があるかもしれません

$targetTrigger = $('.js-accordion_trigger:visible').length;

とすると、js-accordion_triggerクラスのうち、画面に表示されているDOMだけ取得できます。
他にも:checkedなど色々あります。

★4　職人

変数の置き場所を適切にする

繰り返し使用する関数内で変数を定義すると、呼び出し毎に変数が定義されることになります。
関数呼び出し前後で不変の値であれば、関数外で呼び出すとエコで気持ちがいいです。

// 微妙なやつ

$listItem.each(function() {
  const $document = $(document);
  const $this = $(this);

// 良いやつ

// documentは不変の値なので、eachの外で定義
const $document = $(document);

$listItem.each(function() {
  const $this = $(this);

スコープを限定させるために即時関数で囲う(ES5)

即時関数のメリットと主な用途
ES5にて、ちょっとしたjsをページに読み込むときなど、即時関数を使ってスコープを限定させ、グローバルから値が読まれてしまう可能性をなくしたりします。

ES6(ES2015)でグローバルに定義したlet,const,classはグローバルオブジェクトのプロパティにならない
ES6でletやconstが使える場合はグローバルにならないため、即時関数を使わなくても大丈夫です。

Safari系列ではdivタグのclickイベントが拾えない場合があるが、cursor:pointer;を付けると拾える

【JavaScript】Safariでclickイベントが発生しない？
謎現象ですが…。
ちなみにCSS側で-webkit-tap-highlight-color: transparent;を付与するとスマホでタップ時に出る灰色のハイライトが消えます。

関数内で基準値やセレクタなどを直接指定しない

// 基準値やセレクタを直接指定している

const filterContentValue = function() {
  const $target = $('.js-age_input');
  // 値が15以上のinputを抽出する
  const thresholdValue = 15;
  ...

上記のように基準値やセレクタを直接指定してしまうと

• 覚えられない（関数を見に行かないと、どのDOMを参照しているのか？どの値から動作しているのか？が分からない）
• 今はちゃんと動いていても、将来セレクタがページから削除されて動作しなくなる可能性が増える

など危険性が増しがちです。

// 基準値やセレクタを引数で渡す

const $ageInput = $('.js-age_input');
// 値が15以上のinputを抽出する
const thresholdValue = 15;
filterContentValue($ageInput, thresholdValue);

上記のように引数でそれぞれを渡せば

• 関数実行箇所でDOM取得しているため、参照しているDOMが分かりやすい（変数でgrepすることもできる）
• 読みやすい（文脈から、15という値を基準に抽出を行うことが分かる）
• 別の基準値やDOMを渡して関数の再利用ができる

将来基準値やセレクタを変えることがあっても、関数ではなく実行箇所を修正すればよいため、テスト工数が減ります。

★5　哲学

どこまでを１つの関数にまとめるか？

シンプルだけど一番難しいです。
あまりまとめすぎても、

• 責任が多くなりすぎて再利用が難しくなったり…
• 引数が膨大になってしまい、かえって読みづらくなったり…

逆に細かくしすぎても冗長なコードになってしまうため、永遠に悩まされるポイントですね。

頂いた指摘の中では「凝集度」という指標がありました。
関数の凝集度が高いと読みやすく、かつ拡張性が高くなります。
目安としては１メソッド１動詞、くらいの気持ちで作るといい感じに区切れる気がします。

例えばこちらの関数

const toggleAccordion = function ($target, isOpen) {
  if (isOpen) {
    // 引数の状態に対して閉じたり開いたりする
    ...
  }
});

toggleAccordionという関数は、「開いた」状態と「閉じた」状態の切り替えをしているため、openAccordionとcloseAccordionに分解が可能です。
なので…

const openAccordion = function ($target) {
  // 開ける
});

const closeAccordion = function ($target) {
  // 閉じる
});

とし、関数外でisOpenの値に応じてopenAccordionとcloseAccordionを呼んであげる…という書き方ができます。
こうして関数を小さく分けておくと、「やっぱりアコーディオンを開く前に色々したい…」となったときに、新しく機能を追加しやすいなど、拡張性が高くなります。

VSCodeの拡張機能にCodeMetricsというものがあり、こちらは循環的複雑度（に近いもの）を計算して表示してくれるので、関数が大きすぎないか？の目安として使えます。

レビュー観点としてすごく参考になりました→メンバーに恨まれそうな3つのコードレビュー施策を徹底したら、逆にメンバーが爆速で成長した話

変数名・関数名を分かりやすく名付ける

• 関数名の名付けに悩む場合は、関数が大きすぎる可能性がある(分解すれば名付けやすくなるかも？)
• filteredListとするより、filteredCheckedListにする。他人が見たときに「具体的に何の値なのか？」が推測しやすい名前をつける
• getやsetは曖昧すぎるので極力名付けに使わない

などなど…。
毎日「最良の名前とは？」を考えながらコードを書いています。
（レビューで「なるほどな〜〜！」と気付かされることも多いですが！）

これについては記事がまるまる１本書けそうなトピックなので、そのうち自戒として記事に起こしたいですね…。

雑感

悲しくなることも多いのですが、「これは美しいだろ…！」と思って書いたコードが褒められると「この仕事最高〜〜！」と思えるので、これからも読みやすく美しいコードを求めて頑張ります！