【Laravel編】未経験〜これまでに受けた「コードレビュー」まとめ

はじめに

対象読者

• PHPをある程度自分で勉強された方（簡単なアプリなど作ったことのある方）
• これから実務に入るにあたって技術面で意識すべきことが知りたい方

技術スタック

技術スタック
laravel

内容について

こちらの記事では、これまでに参画した現場で受けたコードレビューや、学んだことについて記載しています。
主に、技術的なこと（laravel）について記載されています。

言語や、フレームワークが異なれば理解できないことや、必要な知識としてマッチしないものがあるかと思います。
しかし、多少なりとも参考になることがあるはずです。

「重要度」の高いと思うものから記載していますので、
現場で求められているコードの書き方などが少しでも伝わればと思います。

また、各エンジニアによってレビューの内容は異なります。
同じ現場のエンジニアであっても真逆の指摘を受けることは多々あります。（実際に何度もありました。）
ここに記載されていることと全く別の指摘を受けることが今後もあると思います笑

その時は、現場に合わせるなり、自分の正しいと思うものを信じるなりしてください。

技術編

⭐️⭐️⭐️重要

○外部から呼び出されないメソッドはprivateに

sample.php

private function changeNum()

プロパティや、メソッドなど、
定義したクラス内でしか使わないメソッドの場合はまずprivate使用するべきです。

理由は？

プログラムの安全性を高めるためです。

どこからでもアクセスできるpublicの場合、外部クラスのメソッドから知らない間に参照されたり、変更されたりする場合があるからです。

まずは、privateで対応できないかを考えましょう。

○configファイルには、動的な値を持たさない

config.php

// 現在時刻
'NOW' => ['TIME' => Carbon::now()],

理由は？

本番運用の場合キャッシュするケースがほとんどなので、動的な値を保持すると期待した動作とならないからです。
（今の時刻は、2021/7/30 14:30。しかし、キャッシュしているせいで、2021/7/29 13:00が取得されてしまった。。。なんてことも）

○DB：〜するかどうかは、is_[名前] でカラム名作成

bool値を格納する場合は言語問わず。

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

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

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

対策

index、または、ページ名にしておく

○configファイルいじっても、反映されない場合の対処法（すでに、キャッシュさせてしまってる場合）

「そんなことはすでに知ってる」って方はいいですが、
知らなければ、解決するまで時間を無駄にしてしまいます。

前提

本来は、開発環境において設定ファイルはキャッシュさせるべきでないです。
細かい理由は、この記事がわかりやすいです。

すでにキャッシュさせてしまっている場合

すでに、bootstrap/cache/config.phpが制されている場合は、php artisan config:cacheを実行したためキャッシュが作成されています。
設定ファイルのキャッシュをクリアしましょう。

設定ファイルのキャッシュをクリア
php artisan config:clear

なぜ必要か？

キャッシュが残っているので、config系のファイルを編集した後は上記コマンドを実行しないと設定が反映されないからです。

○controller→viewへ。ridirect時の変数の渡し方

当たり前のように知っている方も中にはいると思いますが。
画面表示以外、リダイレクト時にデータを持たせたいという場合があると思います。
画面表示であれば、これでデータを持たせる事が出来ますが、

return view('top')->with('isLogin' => true);

リダイレクト時には、

// セッションにデータを保存
$request->session()->put('name', $request->name);

セッションに保存出来ることを覚えておくと便利です。

○負荷対策をしましょう

全件取得はNG。
かなり基本的なことやったけど、、、

修正前

DBのすべての情報を取得している（1000万件あった場合、恐ろしいことに。。。）

return $this->get();

修正後

return $this->paginate(20); // 20件ずつ取得

おまけ

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

○HTTPのリクエストパラメータは出来るだけ作らないほうが良い。

理由は？

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

対策

例えば、ユーザー指示別のためにユーザーIDを渡したいなど、
どうしてもパラメータを渡す場合は、バリデーションを入れてDBの存在チェックをする。

○DB生成のトランザクションに注意

独学の範囲では中々意識することはないどころか、知らない方も多いと思います。

ここでは、複数のテーブルに対しての、DB保存処理を一つのメソッド内で行いたい場合の話です。

指摘されたコード（コントローラー）

public function saveUserInfo(UserInfoRequest $request)
{
    // ①ユーザー認証マスタの更新
    Auth('user')->user()->fill([
        'name' => $request->name
    ])->save();

    // ②ユーザー情報マスタの更新
    Auth('user')->user()->user_info->fill([
        'birth' => $request->birth,
    ])->save();
}

トランザクションとは？

複数の処理を一つにまとめたもの

特に、複数DB処理がある時。
➡️整合性を保つため。2つの処理を1つにまとめて、error、okの判断をさせる必要があります。

修正後のコード

public function saveProfile(ProfileRequest $request)
{
    DB::transaction(function () use ($request) { // トランザクション追加
        // ①ユーザー認証マスタの更新
        Auth('user')->user()->fill([
            'name' => $request->name
        ])->save();

        // ②ユーザー情報マスタの更新
        Auth('user')->user()->user_info->fill([
            'birth' => $request->birth,
        ])->save();
    });
}

そのため、データ生成のロジックにはトランザクションを入れましょう。

○ブロック内で定義する変数は、他の言語だと動かない

PHPでは、ブロック内で定義した変数をブロックの外でも使用することができてしまいます。

修正前

if($request->pictures == null) {
    $iconImagePath = null; // ブロック内で変数定義
}
// ブロックの外（ここ）でも使用できてしまう。

ただし、他の言語ではこれでは動かないのでブロックの外で定義してあげましょう。

修正後

$iconImagePath = null; // 外で変数定義
if (!empty($request->pictures)) {

}

○returnで返す値は一定に

これは、コントローラーや、サービスクラスなどreturnで返す処理の話です。
一つのメソッド内、ある時はbool値、ある時は配列が返されるような処理は良くないです。

sample.php

public function showUserInfo()
{
    // ここでは、配列を返す
    if(true) {
        return view('top')->with([
            'userInfo' => $userInfo,
        ]);
    }
    // ここでは、bool値を返す
    return view('top')->with('isLogin' => true);
}

配列なら配列。コレクションを返すならコレクション。
統一しましょう。

sample.php

public function showUserInfo()
{
    if(true) {
        return view('top')->with([
            'userInfo' => $userInfo,
        ]);
    }
}

なぜか？

これを、無理にやるとセキュリティホールが出来てしまうため。

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

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

修正前

sample.php

return response()->json();

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

修正後

sample.php

return response()->json([
    'status' => 200,
    'message' => 'ok'
]);

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

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

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

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

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

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

if (Auth::attempt(['email' => $request->email, 'password' => $request->password])) {
    $request->session()->regenerate(); // sessionID再発行
    return response()->json([
        'status' => 200,
        'message' => 'ok'
    ]);
}

なぜセッションIDを再発行するのか？

• セッションハイジャックされて攻撃者に、勝手にログインされないため

対策

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

⭐️⭐️やや重要

○【フォームリクエスト】rulesに正規表現含む場合は、配列に。

regexとnot_regexパターンを使用する場合はルールをパイプ（縦棒）で区切らず、ルールの配列で指定する必要があります。
とくに正規表現に縦棒を含んでいる場合に該当します。

○migrationファイルを変更したら、seederも修正

テスト用データを作成するために、シーダが準備されている場合があると思います。

その際、存在するシーダを同じテーブルに該当するマイグレーションファイルを何らかの理由で修正した場合、シーダも一緒に修正してください。

理由は？

テストデータが生成できなくなってしまいます。

○【blade】何件表示はbladeの責務

コントローラー側で件数を絞って表示する。ってことはもちろんできますが、何件表示はbladeの責務になります。

表示件数の絞り込みはbladeに任せる。

○コレクションのまま渡して、viewでループする

配列にわざわざ変更する必要はなく、コレクション型のまま渡してview側でループをかけます。

修正前

return view('user')->with([
    // 配列に変換して渡している
    'userInfo' => User::where('id', auth()->id())->get()->toArray(),
]);

修正後

return view('user')->with([
    // コレクションで渡している
    'userInfo' => User::where('id', auth()->id())->get(),
]);

なぜか？

そもそも、配列に変換する意味がない（何か理由があるならいいけれど）

配列で渡された時の展開方法

{{ $info['name'] }}

コレクションで渡された時の展開方法

{{ $info->name }}

○【blade】見た目の表現に関する記述は、blade側に集約

コントローラはあくまで橋渡し的な役割として存在します。

見た目の表現はblade側の責務なので、役割を理解して管理していきましょう。

○パスはroute()で

直接パスを指定するのではなく、名前付きルートを積極的に使いましょう

なぜか？

途中でパスが変わった時にバグの温床になるから。

sample.php

- return redirect('/user')
+ return redirect()->route('user.index');

これは、bladeでも同じ。

○【マイグレーションファイル】 追加項目用ファイルの作成は、リリース後

マイグレーションファイルを作成後に、仕様変更でカラムを新しく追加したという場合もあります。

そんな時のマイグレーションについて。

リリースタイミング	説明
リリース前	既存のマイグレーションファイルに追加項目を追加
リリース後	追加でマイグレーションファイルの作成 → 追加項目、その都度記載

○bladeでCarbon使用時、nowはコントローラーから渡さなくていい

修正前

sampleController.php

    public function show($id)
    {
        return view('user')->with(['now' => Carbon::now()]);
    }

修正後

コントローラーから渡さず、now()->yearで取得

sample.php

@for ($i = now()->year; $i <= $deadline->year; $i++)

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

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

修正前

RegisterRequest.php

'email' => 'unique:users,email',

public function register($userInfo)
{
    return User::create($userInfo);

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

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

理由は？

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

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

修正後

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

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

○【ルーティング】 namespaceと、prefixでまとめる

ネストは極力浅くする。

修正前

web.php

Route::group(['prefix' => 'edit/{id}'], function() {
    Route::group(['namespace' => 'Profile'], function() {
        Route::get('/', 'EditController@editProfile')->name('user');
    });
});

修正後

web.php

Route::group(['prefix' => 'edit/{id}', 'namespace' => 'Profile'], function() {
    Route::get('/', 'EditController@editProfile')->name('user');
});

○認証系の処理はTraitからオーバーライドを使用

trait見る癖をつけると実装がスムーズにいきます。
traitに処理が書かれているので、積極的にtraitからオーバーライドして使用しましょう。

○【PHP】スネークケースを使う場面は？

phpでは、キャメルケースを使用することが多いので、
マジックナンバーを格納している定数にも、キャメルケースを適用していました。

修正前

config/const.php

// ページネーション（リスト単位）
'listNum' => 20,

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

修正後

config/const.php

// ページネーション（リスト単位）
'list_num' => 20,

⭐️参考程度に

○コレクション：多重配列中の重複しない値を取る

sample.php

// 結合
$concatProject = $brashupComment->concat($copperationOffers);

// 重複削除
$project = $concatProject->unique('project_id');

cancat()：指定した「配列」やコレクションの値を元のコレクションの最後に追加

コレクション 6.× laravel

○DI用メンバ変数定義をする

private 変数;

DIを意識してコーディングができると、未経験の枠は抜けることが出来ると思います。

○controllerにメンバ変数は定義しない

なぜか？

コントローラーが内部的にどのように生成されている（例えば、シングルトン等）か分からないため。
また、意図しない方法だった場合、期待した結果とならない可能性があるため。

○Hash化したURLには注意

トークンをURLに含めてユーザーの確認を行う事がある。

そこで、

Hash化をすると、/（スラッシュ）が含まれたものが生成される。
しかし、ハッシュ化されたトークンなどがURLに含まれていると、ディレクトリと勘違いされて404エラーに。（そんなページはないと）

解決方法

URL埋め込みの際

Hashから、base64にencode。
確認時にdecodeすることで解決出来る。

おまけ

base64は可逆（暗号化）
Hashは不可逆（Hash化）

○【フォームリクエスト】 独自のバリデーションを作成する場合について

一度しか使わない場合

クロージャーを使用する。rulesの中に記述。

sampleRequest.php

    public function rules()
    {
        return [
            'tag' => [
                'regex:/^[^#&]*$/', // #&の入力をバリデーション（/^[#&]*$/ は逆にそれのみ許可）
                function ($attribute, $value, $fail) {
                    if ($value === ',') {
                        $fail($attribute.' に , は入力できません');
                    }
                },
        ];
    }

Laravel リファレンス6.0 クロージャの使用　参照

使い回す場合

Ruleオブジェクトを作成する

Rules/sample.php

<?php

namespace App\Rules;

use Illuminate\Contracts\Validation\Rule;

/**
 * 空白文字禁止バリデーションルールクラス
 */
class WithoutWhiteSpaceRule implements Rule
{
    public function passes($attribute, $value)
    {
        return 0 === preg_match('/(\s|　)+/', $value);
    }

    public function message()
    {
        return ':attributeは空白文字を使用できません。';
    }
}

sampleRequest.php

    public function rules()
    {
        return [
            'tag' => [
               new WithoutWhiteSpaceRule,
            ]
        ];
    }

終わりに

今回の内容は約4ヶ月間で受けたコードレビューの内容です。
中には賛否を生むものを含まれていると思いますが、これから実務に入られる方に少しでも参考になれば幸いです。

分かりにくい箇所、質問等あればお気軽にコメントください。

別のコードレビュー記事もございますすのでご覧ください。
【jQuery編】未経験〜これまでに受けた「コードレビュー」まとめ
【技術全般】未経験〜これまでに受けた「コードレビュー」まとめ

最後まで、読んでいただきありがとうございました。