【振り返り】研修期間中（2週間）で学んだ事・受けたコードレビューまとめ

#目次

[1.はじめに](#1-はじめに)
[2.laravelコードレビュー](#2-laravelコードレビュー)
[3.jsコードレビュー](#3-jsコードレビュー)
[4.その他コードレビュー](#4-その他コードレビュー)

#1. はじめに
入社して1ヶ月が経ちました。今回が初投稿になります。

今回、研修で受けたコードレビューについてまとめたいと思います。

ちなみに言語・フレームワークは、```「Laravel(PHP)と、jQuery（JavaScript）」```です。

**これから、エンジニアになられる方には参考になる部分があるかと思います。**

##なぜ書こうと思ったか？

- 同じミスを2度としないように、メモに残しておいて確認するため。
- 最初に指摘されるミスは大体似ると考えたため、今後入社するであろう方にも参考に。

#2. 【Laravel】コードレビュー

##○【Laravelルーティング】api用のルートを使う

全て、```web.php```に書いていたことを指摘されました。

**apiの部分は```api.php```に記載する様に修正**

コントローラーのネームスペースも、

```php
- namespace App\Http\Controllers\Auth
+ namespace App\Http\Controllers\Api;
```

##○return：何か返さないといけない

ajax通信で、laravelからレスポンスを返すとき

###修正前

```sample.php
return response()->json();
```

何も返さないと、HTTPステータスコードのみでの判断になってしまうため、apiとしては不適切

###修正後

```sample.php
return response()->json([
    'code'.   => 200,
    'message' => 'ok'
]);
```

##○【laravel】ログイン後に、sessionIDをregenerate（再発行）してあげる

```php
Auth::login();
$request->session()->regenerate(); // sessionID再発行
```

###なぜセッションIDを再発行するのか？
- セッションフィクセーション対策のため（セッションID固定化攻撃をされないため）

###対策
- ログイン前とログイン後でセッションIDを変える
- ログイン成功時にセッションIDを再発行する

##○【コントローラメソッド名】showでは他のページの描画時メソッド名に困る

```php
- public function show()
+ public function index()
```

1つのコントローラーで1つのページを描画する前提なら、```show()```でもいいが
他のページ（詳細ページや、一覧ページ）を描画する場合、これでは後々困る。

**indexとかページ名にしておく**

##○負荷対策をしましょう

全件取得はNG。
初歩的なミス、、、

###修正前

```php
return $this->get();
```

###修正後

```php
return $this->paginate(20);
```

https://readouble.com/laravel/8.x/ja/pagination.html

**その他参考に以下記事がかなりためになりました。**（他の負荷対策に役立つかも)

http://honeplus.blog50.fc2.com/blog-entry-218.html

##○HTTPのリクエストパラメータを付加するとき

セキュリティの関係上、
攻撃者から、意図しないデータが送られてくる可能性もあります。

###対策

ユーザー入力を少なくする。
どうしてもパラメータを渡す場合は、**バリデーションを入れてDBの存在チェックをする。**

##○【Route】認証はauth:sanctum

https://readouble.com/laravel/8.x/ja/sanctum.html

https://reffect.co.jp/laravel/laravel-sanctum-token#API

##○バリデーションは処理と処理の間にそれぞれ設置する

「新規登録の際、すでにユーザーが存在する場合はどうするのか？」
と、指摘が。

###修正前

```RegisterRequest.php
'email' => 'unique:users,email',
```

```RegisterService.php
public function register($userInfo)
{
    return User::create($userInfo);
```

私「バリデーションに```unique```入れて一意にしている。入り口が防げているからこれで良いのでは・・・？？」

だがこれではいけないみたいです。

###理由は？

**仮に、コントローラー以外から処理する（Serviceを通して、別の開発者が使用など）場合にそのまま処理が通ってしまうから**

フォームリクエストのバリデーションでコントローラーの処理は止めれてが、それ以外のServiceやmodelからの直接の処理はそのまま通る。

###修正後

マイグレーションファイルを修正（一意のデータ）

```php
$table->string('email')->unique() // unique追加
```

##○【PHP】（当現場において）スネークケースを使う場面は？

###修正前

```config/const.php
// ページネーション（リスト単位）
'pagingSize' => 20,
```

→```配列の添字```のように、スネークケースにすると違和感がないとの指摘を。

###修正後

```config/const.php
// ページネーション（リスト単位）
'paging_size' => 20,
```

##○【blade】view側で重い関数を呼び出さない

###修正前

```sample.blade.php
$followService->isFollowed($follower->user_id)
```

view側（blade）で、引数の情報を持つかどうかをDBから取得している処理です。
これをループで回していたこともあり、数が増えることで処理が重くなるという指摘を受けました。

viewの役割はあくまで、**データを表示させたりする、入出力したりです。**

裏側で処理して結果だけを返すように修正。

##○【ORM】

###修正前

```php
public function getFollowedRow($followeeId)
```

行はデータベースレイヤの概念。
ORMでオブジェクトの概念へと変換しているのでメソッド名に```Row```入るのは・・・「△」

###修正後

```php
public function getFollowedUser($followeeId)
```

#3. 【js】コードレビュー

##○【webpack】必要なページにだけ読み込む様にする

jsを全て```app.js```にまとめてバンドルさせていたので指摘を受けました。

読み込む必要のないページにもjsが読み込まれることはパフォーマンス的に良くないということで。

###指摘コード

全てのページをまとめて```app.js```にまとめてバンドルさせていた。

```app.js
require('./components/header.js');
require('./components/auth_modal.js');
require('./components/follow_btn.js');
require('./components/user_menu.js');
require('./components/register.js');
```

###修正後

共通部分はapp.jsのまま

```app.js
require('./components/header.js');
```

その他は、バンドルするファイルを分けて

```webpack.mix.js
mix.js('resources/js/app.js', 'public/js')
    .js('resources/js/top.js', 'public/js')
    .js('resources/js/components/auth_modal.js', 'public/js/components')
// 以下省略
```

それぞれ、bladeへ表示

```php
// 子ブレード
@push('script')
    <script src="{{ mix('js/top.js') }}"></script>
@endpush
```

```php
// 親ブレード
@stack('script')
<script src="{{ mix('js/app.js') }}"></script>
```

##○【ajax】データの送信方法にserialize()を使う

###```serialize()```
>入力された全てのElementを文字列のデータにシリアライズする。
http://semooh.jp/jquery/api/ajax/serialize/+/

###指摘コード

このように、送るform内のvalue値取得したデータを送信していた。

```js
$.ajax({
    // 省略
    data: {
        name                 : $('#registerForm input[name="name"]') .val(),
        email                : $('#registerForm input[name="email"]').val(),
        password             : $('#registerForm input[name="password"]').val(),
        password_confirmation: $('#registerForm input[name="password_confirmation"]').val(),
    },
})
```

### 修正後

シリアライズが可能で一つの文字列として表現して、送信→DB保存。
また、必要な時にDBからでシリアライズして取り出す

```js
let $form = $(this).parents('form');

$.ajax({
    // 省略
    data: $form.serialize()
})
```

###注意

ただ、シリアライズするとデータ量が増えてしまう場合もあるため、使うときは状況に応じて。

##○ajaxで送信するなら、formタグを使わずPOST出来る事を知った

formでactionを使って、preventDefaultしていると矛盾が生じる。
**formタグは使わずに、preventDefaultも使わずにPOSTする事も出来る**

ただ、HTML標準は守りましょう。

##○【ajax】APIのURLはできるだけ隠蔽する

formで送信するときに、action属性をイジられるとどんなapiでも送ることができる。
jsではurlなどを改変しようと思えばなんとでもなる。

ただ、laravelの場合sanctumで認証されたセッションしかAPIが使えないないので一応は安心。

#4. 【その他】コードレビュー

##○【見た目】位置を揃える

###修正前

```js
type    : 'POST',
dataType: 'json',
url: '/api/register', // ズレてる
```

###修正後

```js
type    : 'POST',
dataType: 'json',
url     : '/api/register',
```

##○【命名】メソッド名、クラス名の付け方がよくない

これは、最初はみんな言われるのかも。。。

https://qiita.com/KeithYokoma/items/2193cf79ba76563e3db6

##○【よくやってしまう】使っていないものは消す

```js
// import './bootstrap'
```

コメントアウトとか、使ってないパッケージとか。いらないなら消しておく。

いる場合は、コメント書いておく。
あとで使うとか、理由があって残す場合は、コメント残す

```js
// TODO: あとで使うから残しておく
// import './bootstrap'
```

##○コメントについて

###●実際の挙動に整合性を持たせる
コメントを書くなら「コード内容と、コメント内容」正しいかどうか？
pushする前にもう一度確認する

####修正前

```php
// みかん
apple
```

####修正後

```php
// りんご
apple
```

###●APIにはAPIってつけてあげる

```php
- // ログインコントローラー
+ // ログインAPI コントローラー
```

##○シンプルに

###修正前

```php
        $registerService->register([
            'name'     => $request->name,
            'email'    => $request->email,
            'password' => Hash::make($request->password)
        ]);
        if (!Auth::attempt(['email' => $request->email, 'password' => $request->password])) {
            return response()->json([
                'code' => 403,
                'message' => 'ログイン出来ませんでした',
            ]);
        }
        return response()->json([
            'code' => 200,
            'message' => 'ログインできました',
        ]);
```

###修正後

```php
        $user = null;
        try {
            $user = $registerService->register([
                'name'     => $request->name,
                'email'    => $request->email,
                'password' => Hash::make($request->password)
            ]);
        } catch (\Exception $e) {
            Log::error($e);
            return response()->json(['code' => 403, 'message' => 'ログイン出来ませんでした']);
        }
        dd($user);
        Auth::login($user);
        return response()->json(['code' => 200, 'message' => 'ログインできました']);
```

##○ローカルと、リモートのversionはどんなものも揃えましょう

nodeのバージョンが違うだけで時間を無駄にしました。

##○【DB】unsignedと、インデックス

>【id】
①**[unsigned](http://dbinfo.sakura.ne.jp/?contents_id=34)型にする**
②**indexを貼る**

###修正前

```php
$table->id();
$table->bigInteger('user_id');
```

###修正後

```php
$table->id()->unsigned();
$table->bigInteger('user_id')->index();
```

##○【Docker】ユーザーを分ける

**実行ユーザーとrootユーザーは別にする。**

###なぜか？

セキュアにするため、実行ミスを防ぐ。

https://blog.nownabe.com/2018/06/05/1321.html/