入社１年目のWEBエンジニアが、コードレビューで教わったことを振り返ってみる #JavaScript

アドベントカレンダー何書こうかな〜と考えていたところ、フロントエンドエンジニア１年目はコードレビューでどんな指摘を受けるのかという投稿を読み、私も良い機会なので書いてみることにしました。

私のステータス

昨年11月にqnoteに入社
コールセンターからエンジニアにジョブチェンジ
学生時代にプログラミング未学習
Laravel + Vue.js を使ったプロジェクトに参画
フロントエンド / バックエンド共に行い、機能別に issue を立てて、issue 毎に担当。

初歩的なこと

typo

私はこれがひどい。

date と data を間違える
変数名が camelCase なのに snake_case で書いて値が取れないと騒ぐ
delete を detele と書く

エディタでスペルチェックをしてもらうのが良い。
Sublime Text なら、 spell_check: true にするか F6 でスペルチェックをしてくれる。
VSCode でもプラグインを入れればスペルチェックしてくれる。

真偽値の統一

true、'1'、1 は全て true と判断できるが、コーディング規則に従い統一する

rand ではなく mt_rand

テストで乱数を生成するときに、rand を使用した。理由はググったら最初に出できたから。
しかし、rand の上位互換である mt_rand を使用するように教わった。
理由は mt_rand の公式ドキュメントにある。

古い libc の多くの乱数発生器は、怪しげであるか特性が不明であったりし、また低速でした。 mt_rand() 関数は、古い rand() の代替品となるものです。この関数は、その特性が既知の乱数生成器 » メルセンヌ・ツイスターを使用し、平均的な libc の rand()よりも 4 倍以上高速に乱数を生成します。
PHP: mt_rand - Manual

命名規則に従う

変数名などの表記方法は一般的に以下の3つになる。

camelCase キャメルケース
PascalCase パスカルケース（キャメルケースの頭文字大文字版）
snake_case スネークケース

参画したプロジェクトでは、変数名は camelCase、クラス名とファイル名は PascalCase にすることになっていた。
これはプロジェクト毎で規則があると思うが、変数やファイル名などの規則がバラバラにならないように気をつける。

エラーを読む

これ、よくやりがち。
エラーが出た
->怖くなって先輩に質問しに行く。
->先輩がエラー見る
->先輩『エラー文言読んだ？』
->私『読んでません。（汗』

エラーは、ちゃんと問題点を書いてくれていることが多い。
先輩に聞きに行く前に、翻訳してみたり、ググってみればわかることは多い。
5分調べて、10分試して解決しなければ聞きに行く。くらいで良いと思う。

公式ドキュメントを読もう（英語でも）

困った時、ググる。
すると Qiita とかはてブとか teratail とかどっかのオンラインスクールとかが、検索上位に出てくる。
そういう記事は、タイトルが困ってる状態と似ていることが多いのだが、自分の環境と違ったり、間違っていたりすることもある。
で、結局解決せずに、先輩に聞きに行く
->先輩『公式ドキュメント読んだ？』
->私『読んでません(汗』

内容によるが、基本的な使い方は公式ドキュメントに書いてある。
公式ドキュメントが英語のみってこともある。lodash とかね。
けど、そんな難しい言い回しはしていない。
翻訳しつつ、頑張って読んでいれば、だんだん読めるようになってくる。

可読性向上

三項演算子やエルビス演算子で済むなら、if文は使わない。

最近の流行りというのもあるかもだけど、単純な条件分岐なら、if文を使わない方が読みやすい。

countによってメッセージを変える処理.js

msg() {
  if (this.count > 10) {
    return 'カウントが 10回 を超えました。'
  }
  return ''
},

// 三項演算子
msg() {
  return this.count > 10 ? 'カウントが 10回 を超えました。' : ''
},

一行を長くしすぎない（改行する）

一行の文字が多く、横に長すぎるとコードが読みにくい。

ESLintを導入していれば、エラーで教えてくれる。
max-len - Rules - ESLint - Pluggable JavaScript linter

DRY

Don't repeat yourself（繰り返すな）。
同じような処理を複数回書くことは、修正が必要な時に修正漏れなどのリスクが増す。
また、同じような処理なら、別functionとして切り離せばテストもしやすい。

テスト

DI を使う

以下のような機能のテストを書いているときに教わった。

DIを使用していない.php

class Sample
{
    public function execute(array $params)
    {
        if ($userCd = Arr::pull($value, 'user_cd')) {
            $userId = app(GetUser::class)
                ->getUserId($userCd);

            // 何かする
        }
    }
}

このテストを書く場合、GetUser::getUserId() が未実装だとテストが行えない。

そこで DI を使用する。

DIを使用した場合.php

class Sample
{
    private $getUser;

    public function __construct(GetUser $getUser)
    {
        $this->getUser = $getUser;
    }

    public function execute(array $params): string
    {
        if ($userCd = Arr::pull($value, 'user_cd')) {
            $userId = $this->getUser
                ->getUserId($userCd);

            // 何かする
        }
    }
}

このように DI を使用すると、この Sample クラスのテストをする際に、GetUser::getUserId() をモックすれば、テストが行える。

モックオブジェクトを使ったテスト.php

class SampleTest extends TestCase
{
    $getUser = $this
        ->getMockBuilder(GetUser::class)
        ->setMethods(['getUserId'])
        ->getMock();

    $getUser->expects($this->once())
        ->method('getUserId')
        ->willReturn(123);

    $sample = new Sample($getUser);

    $parames = [
        'user_cd' => 'hogehoge123'
    ];

    $res = $sample->execute($parames);
    $this->assertEquals($res, '期待値');
}

機能を細かく分ける

理想は、１クラスに１機能。
そうすることで、テストが容易になる。

SOLID原則

敬愛なるゆいもっぷ先輩の記事をご参照ください💁‍♂️
SOLID原則について簡単に書く

猫

猫社員の見分け方

名前	特長
ふたば	クリーム色
みるく	茶虎、口元が白い、尻尾が少し鍵しっぽ
はな	茶虎、全体的にシュッとしてる
ちまき	錆色、尻尾が縞様、おでこハゲ
さくら	錆色、顔の桜色の部分多め
りぼん	錆色、小さい、かわいい
みぃ	白黒

猫社員たちをご覧になりたい方は、是非遊びにきてください！
株式会社qnote

猫は腰を叩かれると喜ぶが、叩きすぎは良くない

叩きすぎるとヘルニアになる可能性がある。

最後に

いつもコードレビューや質問に応えていただいている @adwin さんや @poipoisaurus さんをはじめとする qnote の皆さんに本当に感謝しています。
ありがとうございます。
そして、来年もよろしくお願いします🙇