フロントエンドエンジニア１年目はコードレビューでどんな指摘を受けるのか

社内のコードレビューで先輩から指摘いただいた事をまとめました。
主にJavascript（Vue.js）のコードです。

本記事の修正前のコードは、（Qiita用に簡略化してますが）実際に自分が書いたダメコード。
ちゃんと失敗を活かしていけよと自分へのメッセージを込めて書きます！

筆者のスキル感

• エンジニア歴（≒プログラミング歴）ちょうど１年
• フロントエンドエンジニア
• Vue.js/Nuxt.jsで開発

凡ミス

一番指摘されると申し訳なく感じるところ。
凡ミスがマージされるとバグの温床になるので、気をつけなきゃですね・・。

デバッグの痕跡が残っている

console.log('hoge')

現プロジェクトはgitのpre-commit hookを使ってをコミットしないようしてるのですが、
そこをすり抜けてデバッグコードを潜ませてしまう事があります・・。

タイポ

const enviroment
これは真中の n がないですね。environmentが正解です。

わたしはVSCodeのスペルチェッカーを使ってタイポが減りました。
（スペルミスの箇所をハイライトしてくれるので、とても便利です！）
Code Spell Checker - Visual Studio Marketplace

作ったら消す

スクロールイベント消し忘れ

スクロールしたらHTML要素が消えたり、ボタンが現れたりする実装をよくしますよね（？）
これはaddEventListenerでスクロールイベントを発火するVue.jsのコードです。

修正前

<script>
export default {
  // インスタンスがマウントされたとき
  mounted() {
    window.addEventListener('scroll', this.handleScroll());
  },
  methods: {
    handleScroll() {
      // 処理
    }
  }
};
</script>

これは意図した動作になるものの、よくありません・・。

[修正すべきポイント]
1. スクロールイベントが発火し続ける
2. スクロールイベントが大量に繰り返される

忘れがちですが、どこかで removeEventListener でイベントを削除すべきです。
ページ遷移など使わなくなったタイミングで削除するのならdestroyed（Vueのライフサイクル）が使えます！

また、スクロールイベントは大量に繰り返されるので、一定期間lodashの throttle などで間引くのがベターです。

修正後

<script>
import _ from 'lodash';

export default {
  mounted() {
    window.addEventListener('scroll', this.handleScroll());
  },
  methods: {
    handleScroll: _.throttle(() => {
      // 1秒ごとに処理を実行
    }, 1000)
  },
  // インスタンスが破棄された後
  destroyed() {
    window.removeEventListener('scroll', this.handleScroll());
  }
};
</script>

参考
* Lodash Documentation
* throttleとdebounce

オブジェクトURL消し忘れ

createObjectURLは画像のプレビュー表示など、一時的にFileオブジェクトをURLにして参照できます。

わたしはrevokeを知らず書いていませんでした。動的にこれを使った際は明示的にrevokeが必要です！
（でないとメモリがかさみ、最終的にブラウザがモタつきます・・。）

// オブジェクトのURLを作成
objectURL = URL.createObjectURL(object)
// オブジェクトURLを取り消し
window.URL.revokeObjectURL(objectURL)

参考
* Web アプリケーションからファイルを扱う - Web API | MDN

変数名

現在・過去・未来・複数を意識する

// 修正前
const reverseColor = ['red', 'blue', 'yellow'].reverse();
// 修正後
const reversedColors = ['red', 'blue', 'yellow'].reverse();

修正後の変数名は、より表現が正確になりました！

変数 reverseColor にはreverse済みの配列が入るので、過去分詞形のreversedが適切、
色は複数あるので、Colorsのほうが分かりやすくなります。

CSSやJavascriptのネーミングを参考にする

// 修正前
const isShow = false
// 修正後
const isVisible = false

これは true のときにHTML要素が表示され、 false のときにHTML要素が非表示になる変数です。

修正後はCSSのvisibilityプロパティに合わせて isVisible としています。
どちらも意味は分かりますが、より直感的になりました！

@Riliumph さんより
isShowはまず英語的にNGです。be動詞と一般動詞が混在しています。
↑勉強になりました、ありがとうございます。

ネーミングが安易

これは配列favoritesの中身（color）を取り出して表示するVue.jsのコードです。

  <!-- 修正前 -->
  <div v-for="(item, i) in favorites" :key="i">
    <p>好きな色: {{ item.color }}</p>
  </div>
  <!-- 修正後 -->
  <div v-for="(favorite, i) in favorites" :key="i">
    <p>好きな色: {{ favorite.color }}</p>
  </div>

これは要素が少ないので見やすいですが、要素が増えてネストしていくとitem が何のアイテムなのか想像ができません・・！
他にも、value、num、boolなど情報が少ない単語は避けた方がベターです。

ネーミングで参考にしているサイト

• プログラミングでよく使いそうな動詞を探したいとき
  プログラミングの変数・メソッドの命名でよく使う英単語を整理（備忘） - "BOKU"のITな日常

• 単語を検索したいとき
  プログラマーのためのネーミング辞書 | codic

• 似た単語を検索したいとき
  英語類語 - Weblio辞書

省略できる系

短く難解なコードはいやですが、パッと見てわかるコードなら短いほうが良いですよね！

lengthチェック

// 修正前
const isEmpty = contents.length === 0
// 修正後
const isEmpty = !contents.length

配列 contents が空なのか判定します。
このくらいなら見やすさが変わらないと思うかもしれませんが、他にも比較演算子や数字が出てくるとき、 === 0 がないだけで、見やすくなります！

また、! を使う場合、正常系で0・false・空文字が入る場合は注意が必要です。

参考
* [JavaScript] null とか undefined とか 0 とか 空文字('') とか false とかの判定について - Qiita

テスト

vue/test-utilsのmountを正しく使う

// 子コンポーネントをマウントしてテストしたいとき
import { mount } from '@vue/test-utils'
// 子コンポーネントをマウントせずテストしたいとき
import { shallowMount } from '@vue/test-utils'

テストをはじめて書いたときはサンプルコードを参考になんとなく mount で書いてました。
（テストが通らなくてなんとなくshallowMountにしたら上手くいったことも）

子コンポーネントも考慮してテストしたい場合は mount 、
子コンポーネントと切り離してテストを行いたい場合、 shallowMount を使うと良いです！

参考
* 一般的なヒント | Vue Test Utils

おわり

tips系をまとめましたが、実際には上記以外にもコンポーネント設計、データの持ち方のアドバイスを頂くことが多かったです。

一度ツッコミをもらえると、次回コードを書く上で気をつけるべきポイントがわかって良いですよね🎉
慣れた方にとっては耳にたこができる内容かもしれませんが、最後までお読みいただきありがとうございました！🎅🏻

明日の記事

ゆめみアドベントカレンダー3日目は @lovee さん！👏
APIKit による通信リトライの実装と、OHHTTPStubs によるその実装のテスト