社内のコードレビューで先輩から指摘いただいた事をまとめました。
主に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>
これは意図した動作になるものの、よくありません・・。
[修正すべきポイント]
- スクロールイベントが発火し続ける
- スクロールイベントが大量に繰り返される
忘れがちですが、どこかで 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>
参考
オブジェクトURL消し忘れ
createObjectURLは画像のプレビュー表示など、一時的にFileオブジェクトをURLにして参照できます。
わたしはrevokeを知らず書いていませんでした。動的にこれを使った際は明示的にrevokeが必要です！
（でないとメモリがかさみ、最終的にブラウザがモタつきます・・。）
// オブジェクトのURLを作成
objectURL = URL.createObjectURL(object)
// オブジェクトURLを取り消し
window.URL.revokeObjectURL(objectURL)
参考
変数名
現在・過去・未来・複数を意識する
// 修正前
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・空文字が入る場合は注意が必要です。
参考
テスト
vue/test-utilsのmountを正しく使う
// 子コンポーネントをマウントしてテストしたいとき
import { mount } from '@vue/test-utils'
// 子コンポーネントをマウントせずテストしたいとき
import { shallowMount } from '@vue/test-utils'
テストをはじめて書いたときはサンプルコードを参考になんとなく mount で書いてました。
（テストが通らなくてなんとなくshallowMountにしたら上手くいったことも）
子コンポーネントも考慮してテストしたい場合は mount 、
子コンポーネントと切り離してテストを行いたい場合、 shallowMount を使うと良いです！
参考
おわり
tips系をまとめましたが、実際には上記以外にもコンポーネント設計、データの持ち方のアドバイスを頂くことが多かったです。
一度ツッコミをもらえると、次回コードを書く上で気をつけるべきポイントがわかって良いですよね🎉
慣れた方にとっては耳にたこができる内容かもしれませんが、最後までお読みいただきありがとうございました！🎅🏻
明日の記事
ゆめみアドベントカレンダー3日目は @lovee さん！👏
APIKit による通信リトライの実装と、OHHTTPStubs によるその実装のテスト