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

社内のコードレビューで先輩から指摘いただいた事をまとめました。
主に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](https://marketplace.visualstudio.com/items?itemName=streetsidesoftware.code-spell-checker)

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

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

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

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

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

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

#### 修正後
```index.vue
<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](https://lodash.com/docs/4.17.15#throttle)
* [throttleとdebounce](https://aloerina01.github.io/blog/2017-08-03-1)


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

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

```index.js
// オブジェクトのURLを作成
objectURL = URL.createObjectURL(object)
// オブジェクトURLを取り消し
window.URL.revokeObjectURL(objectURL)
```
参考
* [Web アプリケーションからファイルを扱う - Web API | MDN](https://developer.mozilla.org/ja/docs/Web/API/File/Using_files_from_web_applications#Example:_Using_object_URLs_to_display_images)

## 変数名
### 現在・過去・未来・複数を意識する
```index.js
// 修正前
const reverseColor = ['red', 'blue', 'yellow'].reverse();
// 修正後
const reversedColors = ['red', 'blue', 'yellow'].reverse();
```
修正後の変数名は、より表現が正確になりました！

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

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

```index.js
// 修正前
const isShow = false
// 修正後
const isVisible = false
```
これは `true` のときにHTML要素が表示され、 `false` のときにHTML要素が非表示になる変数です。

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

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

### ネーミングが安易
これは配列favoritesの中身（color）を取り出して表示するVue.jsのコードです。

```index.vue
  <!-- 修正前 -->
  <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な日常](https://arakan-pgm-ai.hatenablog.com/entry/2019/04/15/000000)

* 単語を検索したいとき
[プログラマーのためのネーミング辞書 | codic](https://codic.jp/)

* 似た単語を検索したいとき
[英語類語 - Weblio辞書](https://ejje.weblio.jp/english-thesaurus/)

## 省略できる系
短く難解なコードはいやですが、パッと見てわかるコードなら短いほうが良いですよね！
### lengthチェック

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

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

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

参考
* [[JavaScript] null とか undefined とか 0 とか 空文字('') とか false とかの判定について - Qiita](https://qiita.com/phi/items/723aa59851b0716a87e3)

## テスト
### vue/test-utilsのmountを正しく使う

```index.js
// 子コンポーネントをマウントしてテストしたいとき
import { mount } from '@vue/test-utils'
// 子コンポーネントをマウントせずテストしたいとき
import { shallowMount } from '@vue/test-utils'
```
テストをはじめて書いたときはサンプルコードを参考になんとなく `mount` で書いてました。
（~~テストが通らなくてなんとなくshallowMountにしたら上手くいったことも~~）

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

参考
* [一般的なヒント | Vue Test Utils](https://vue-test-utils.vuejs.org/ja/guides/common-tips.html)

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

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

## 明日の記事
ゆめみアドベントカレンダー3日目は @lovee さん！👏
[APIKit による通信リトライの実装と、OHHTTPStubs によるその実装のテスト](https://qiita.com/lovee/items/5e2c44a73557946ededd)