Help us understand the problem. What is going on with this article?

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

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

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

筆者のスキル感

  • エンジニア歴(≒プログラミング歴)ちょうど1年
  • フロントエンドエンジニア
  • 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のコードです。

修正前

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
* throttleとdebounce

オブジェクトURL消し忘れ

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

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

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

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

変数名

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

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など情報が少ない単語は避けた方がベターです。

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

省略できる系

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

lengthチェック

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

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

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

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

テスト

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

おわり

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

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

明日の記事

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

Why not register and get more from Qiita?
  1. We will deliver articles that match you
    By following users and tags, you can catch up information on technical fields that you are interested in as a whole
  2. you can read useful information later efficiently
    By "stocking" the articles you like, you can search right away
Comments
No comments
Sign up for free and join this conversation.
If you already have a Qiita account
Why do not you register as a user and use Qiita more conveniently?
You need to log in to use this function. Qiita can be used more conveniently after logging in.
You seem to be reading articles frequently this month. Qiita can be used more conveniently after logging in.
  1. We will deliver articles that match you
    By following users and tags, you can catch up information on technical fields that you are interested in as a whole
  2. you can read useful information later efficiently
    By "stocking" the articles you like, you can search right away
ユーザーは見つかりませんでした