Laravel触って1年経ったので過去の戒め（ソースコード）を改善してみた

Laravelのリファクタ記事にしようと思ったが、ただただ間違ったコードを直すだけの記事でした。

久々に過去練習用として作成したサイトのソースコードを覗いたら無駄な処理書いてたり、
Laravelの機能活かせれてなかったりそもそもPHP使えてなかったりと散々だったので、
いい反面教師コードだと思って改善記事（改善というか修正レベル）書いてみました。

目に付いた部分しか改善しないので、一貫性はありませんし、
他の方が絶対やらないような事も多々ありますが、戒めとしてどうか生温かい目で見守ってください。

環境

Laravel5.7
PHP7.1

注意

改善とはいえ、これがベストとは思ってません。
あくまで出来る時間で簡単な改善（修正）を行っただけです。
その辺り、ご容赦くださいm(._.)m

関係ないところは一部省略します。
例外処理等も省略します。

フォームリクエストの謎の使い方

FormRequest便利ですよね。
Controllerにバリデーションを書かずに済むので良く使っています。
（他にもControllerで受け取る前の前加工とか）

さて過去のソースコードを覗いてみましょう

過去コード

CreateController.php

use App\Http\Requests\Validation;

public function store(Validation $request)
{
    //Validation Requestからルールを取得
    $rules = $request->rules();
    //Validation Requestからエラーメッセージを取得
    $messages = $request->messages();

    $data = Validator::make($request->all(), $rules, $messages);
}

フォームリクエストから受け取ってる時点でバリデーション処理は済んでるはずなので、色々と不要ですね。
それとValidationというクラス名だと分かりづらいので、クラス名も変更します。

改善コード

CreateController.php

use App\Http\Requests\ProjectRequest;

public function store(ProjectRequest $request)
{
    $data = $request->all();
}

ProjectRequestと命名した理由は案件登録で使用するのが一つと作成と更新で使用するのであえてCreateProjectRequestとしませんでした。

改善ポイント

• 無駄な記述を削除して本来の使い方を実装
• クラス名の役割が抽象的すぎたので、少し具体的にして明確化出来た？

fillable便利だよ

fillableもありがたいですよね〜
デフォルトで存在するUser.php見ると最初から存在するコイツですね

User.php

protected $fillable = [
    'name', 'email', 'password',
];

何が便利なのか、まずは悪い方の過去のコードを見てみましょう。

過去コード

CreateController.php

use App\Http\Requests\ProjectRequest;

public function store(ProjectRequest $request)
{
    $project = new Project;
    $project->name = $request->name;
    $project->responsible_id = $request->responsible;
    $project->category_id = $request->category;
    $project->description = $request->description;
    $project->url = $request->url;
    $project->release_year_at = $request->release_year;
    $project->release_month_at = $request->release_month;
    $project->image_id = $image_id;
    $project->start_year_at = $request->start_year;
    $project->start_month_at = $request->start_month;
    $project->end_year_at = $request->end_year;
    $project->end_month_at = $request->end_month;
    $project->save();
}

うーん行が無駄に多いのと保存処理のたびにこれを書くのはめんどくさいですね。

では改善コード

fillableも勿論活用しますが、そもそも保存処理をControllerでやるのはMVCとしてよろしくないのでModelに責務を分けましょう。

改善コード

ちなみに引数にモデル（というかクラス名）をタイプヒントするとLaravelがインスタンスを用意してくれます。

CreateController.php

use App\Http\Requests\ProjectRequest;
use App\Project;

public function store(ProjectRequest $request, Project $project)
{
    $results = $project->storeProject($request->all());
}

ではModel

Project.php

protected $fillable = [
    'responsible_id',
    'category_id',
    'image_id',
    'name',
    'url',
    'description',
    'release_year_at',
    'release_month_at',
    'start_year_at',
    'start_month_at',
    'end_year_at',
    'end_month_at',
];

public function storeProject(array $data)
{
   return $this->create($data);
}

保存の部分が1行で済みました！
fillableというのは保存するときのホワイトリストなんですね〜
勿論更新の時にも適用されます！

逆にブラックリストのguardedもあります
[Laravel 5.7] Eloquent Modelのfillableとguardedの違い

Project.php

public function storeProject(array $data, int $id)
{
   return $this->where('id', $id)->update($data);
}

改善ポイント

• 入力内容が増えるたびに書き足す必要がなくなった
• MVCで責務をちゃんと分けた
• コード行短縮

resourceの恩恵を得ていない

Controllerを作成する時に良くみますね〜
説明は別記事に託します。

Laravelのリソースコントローラを理解して使ってみよう！

ではルーティングをみてみましょう。

web.php

Route::get('project/', 'ProjectsController@index');

Route::get('project/create', 'CreatesController@index');

Route::post('project/create', 'CreatesController@store');

Route::get('project/edit', 'CreatesController@edit');

Route::post('vupdate', 'CreatesController@update');

Route::get('project/delete', 'CreatesController@delete');

突っ込みどころは多いですけど一旦スルー

改善コード

web.php

Route::prefix('project')->name('project.')->group(function () {
    Route::resource('/', ProjectsController, ['except' => 'show']);
});

改善ポイント

• Projectに関連した物しかなく、restfullで完結できそうだったので、ProjectsControllerファイルにまとめました（これはたまたまなので、無理矢理真似なくてOK）
• groupにまとめて無駄な記述が減らせたのと関連したルーティングが見易くなった。
• Routeメソッドをresourceに変える事でメソッド毎に定義しなくて済んだ（showは使わないのでexceptで除外していますが本来はonly派）

ちょこっと番外編（LaravelというかPHP）

検索機能を付けていたのだが、過去のコードを見てみましょう。

過去コード

ProjectsController

public function index(Request $request)
{

    $query = Project::query();

    if (isset($request))
    {
        $category_id = $request->get('category_id');

        $responsible_id = $request->get('responsible_id');

        $tag_ids = $request->get('tag_id');

        $release_year = $request->get('release_year');

        $release_month = $request->get('release_month');

        $start_year = $request->get('start_year');

        $start_month = $request->get('start_month');

        $end_year = $request->get('end_year');

        $end_month = $request->get('end_month');

        //カテゴリ
        if (!empty($category_id))
        {
            $query->where('category_id', 'like', '%' .$category_id .'%');
        }

        //ユーザー
        if (!empty($responsible_id))
        {
            $query->where('responsible_id', 'like', '%' .$responsible_id .'%');
        }

        //リリース年
        if (!empty($release_year))
        {
            $query->where('release_year_at', 'like', '%' .$release_year .'%');
        }

        //リリース月
        if (!empty($release_month))
        {
            $query->where('release_month_at', 'like', '%' .$release_month .'%');
        }

        //開発開始年
        if (!empty($start_year))
        {
            $query->where('start_year_at', 'like', '%' .$start_year .'%');
        }

        //開発開始月
        if (!empty($start_month))
        {
            $query->where('start_month_at', 'like', '%' .$start_month .'%');
        }

        //開発終了年
        if (!empty($end_year))
        {
            $query->where('end_year_at', 'like', '%' .$end_year .'%');
        }

        //開発終了月
        if (!empty($end_month))
        {
            $query->where('end_month_at', 'like', '%' .$end_month .'%');
        }
    }
}

なげーーーー

しかも検索条件増えるたびに書き足さないといけない、、
という事で今回は複雑な検索条件は無かったので短縮化

ProjectsController

public function index(Request $request, Project $project)
{
    $query = $project->searchProject($request);
}

Modelに記述

Project.php

    public function searchProject(Request $request)
    {
        $query = $this->query();

        if ($request->query() !== '') {
            foreach ($request->all() as $key => $val) {
                !empty($val) ? $query->where($key, 'LIKE', '%' .$val .'%') : '';
            }
        }

        return $query;
    }

これで検索条件が増えても安心ですね^^（簡単な条件に限るけど）

改善ポイント

• コード行短縮
• 条件が増えるたびに書き足す必要がない

Bladeのoldヘルパー使えてる？

良く編集画面とかで動的に変わる選択ボックスとか見かけますよね？？（この辺割と記事ない印象）
そういう時入力値を優先させて、old()がなければDBの値を反映して一致した時にselectedとかif文でゴニョゴニョしまくってませんか？

勿論過去の僕はしてます。
しかも型を厳密にチェックしてません。
悪い子です。

過去コード

edit.blade.php

<select name="category_id" class="form-control">
    @foreach ($categories as $category)
        <option value="{{ $category->id }}"
                @if(!empty(old('category')))
                    @if(old('category') == $category->id)
                        selected
                    @endif
                @elseif($project->category_id == $category->id)
                    selected
                @endif
            >{{ $category->name }}</option>
    @endforeach
</select>

上のコードもold()の第二引数を使用すればここまでスッキリかけます！
（ちょっと横に長いけど、改行してもよし）

改善コード

edit.blade.php

<select name="category_id" class="form-control">
    @foreach ($categories as $category)
        <option value="{{ $category->id }}" @if ($category->id === (int) old('category_id', $project->category_id)) selected @endif>{{ $category->name }}</option>
    @endforeach
</select>

ちなみにold()はstringで返ってくるのでintに変換してます。

改善ポイント

• 無駄なif文を省いて見易くなった

終わり

誰もこんなクソコードを量産して欲しく無かったので戒めとして書きました。
皆さんも昔のソースコード久々に覗いてみては？
乾いた笑いが出ると思います。