コードレビュー指摘ポイント PHP Laravel

Laravel

RepositoryでfindWhere(array []), fetchList(array [])みたいなのを実装しない

❌ 　$r_user->findWhere(array [])
⭕️ 　$r_user->findById(int $user_id)

findWhere(array [])みたいに何が入ってくるかわからないメソッドを作らない。

修正しようとした時に汎用的すぎるメソッドで実装されていると、影響範囲が膨大になってしまう。

Serviceの中でテーブル検索した処理を書かない

リポジトリで定義すること


class UserService
{
    public function getUser(int $user_id)
    {
        return $r_user->findById($user_id);
    }
}

$xxx_flagは使わない。

$is_xxxを使う

フラグは$is_の形にすると意味がはっきりする $xxx_flagは使わない

❌ 　$original_file_flag = true;
⭕️ 　$is_original_file = true;

ネストの階層が3以下になっているか

4以上になっていたら、アーリーリターンや関数化して外出ししよう。

この規約による効果は強力なので、チームのコード規約として定義しよう。

インスタンス変数は原則final

アーリーリターン

if (is_null($item)) {
    return;
}

4xx系, 5xx系の設計

404, 422, 429で良いところを5xxにしない
パラメータが不足しているなど、攻撃者からの予期しない外部リクエストで5xxが発生しないようにする
5xxはシステム内部のエラーのみ扱う

理由はサーバへのAPIリクエストをSLIとしたいから。

5xxはシステムエラーのみを扱わないと、SLIをAPIリクエストのサーバステータスとした場合にSLO, SLAを定義できなくなるから。

関数化しよう

１つのメソッドは50行程度まで
2回以上同じ処理を別の関数で利用したり、長くなりすぎる場合は外だしすると可読性があがる

    /**
     * Execute the console command.
     *
     * @return mixed
     */
    public function handle()
    {
        //

        // 登録処理
        $this->_registorProcess($rows);
    }


    /**
     * 登録処理
     *
     * @param array $rows
     * @return void
     */
    private function _registorProcess(array $rows)
    {
        //
    }

その際の関数はprivate function _xxx()という形で、_をつけるとprivate メソッドで他のクラスを探さなくて良い

短い処理でも、関数化することで意味と型チェックができる

これを、

$full_name = "{$last_name} {$first_name}";

こうする

/** 
　　* フルネームを生成して取得
 *
　　* @param string $first_name
　　* @param string $last_name
 * @return string
 */
public function getFullName(string $first_name, string $last_name): string
{
    return "{$first_name} {$last_name}";
}

PHPDoc, 型チェックもできて大変わかりやすい🐱

また、Value Objectにすることも検討する。

elseifは使わない

if ($class === 'low') {
  //
} elseif ($class === 'high') {
  //
}

可読性が落ちる。

ではどうするか？

ifを並列にならべる

if ($class === 'low') {
  //
}
if ($class === 'high') {
  //
}

分岐が2つまでならifで対応する。

それ以上の場合、ifの順番で処理が変わってしまう場合はswitch

もしくはswitchを利用する

switch ($class) {
    case 'low':
        //
        break;
    case 'middle':
        //
        break;
    case 'high':
        //
        break;
}

配列の初期化

配列で初期化を忘れずに。

$array = [];

型指定、戻り値指定漏れ

メソッドでは忘れないように指定しよう。

/**
 * 職種更新
 *
 * @param Request $request
 * @param integer $section_id
 * @param integer $job_id
 * @return array
 */
public function updatejob(UpdateRequest $request, int $section_id, int $job_id): array
{
    $this->job_service
        ->setCustomerId($request->customer)
        ->setsectionId($section_id)
        ->setjobId($job_id)
        ->update(json_decode($request->getContent(), true);
    return [];
}

配列のキーが存在するかを確認してから参照しているか

PHP 判定チェック isset empty if is_null

foreachでループしすぎていないか。適当な関数があるのでは？

検索系

array_search() … 値を検索
array_key_exists() … キーを検索
array_diff() … 配列A, Bの差分要素を取得
array_intersect()による要素の共通項
array_intersect_keyによるキーの共通項
array_intersect_assocによる「キー => 値」の共通項

操作系

array_shift() … 配列から最初の要素を切り出す
array_filter($array, ‘{関数名}’) … 配列を関数を指定して要素のtrue, falseでフィルターする
array_unique() … 配列の要素をユニークにする
ex. PHP 配列の重複をチェックする

など

JOIN EXISTS INの使い分け

JOIN, EXISTS, IN使い分け

コレクション

多次元配列の操作ならこれ😼

https://www.yuulinux.tokyo/tag/php-collection/

冗長なループ

子テーブルのデータ取得編

アカン例.

例えばこんな感じ

$engineer_staffs = app()->make(\App\Repositories\Staff\StaffRepository::class)->byWhere([
        'type' => 'engineer',
    ]);
$engineer_staff_ids = [];
foreach ($engineer_staffs as $engineer_staff) {
    $engineer_staff_ids[] = $engineer_staff->id;
}
$r_staff_history = app()->make(\App\Repositories\Staff\StaffHistoryRepository::class);
$staff_history_ids = [];
foreach ($engineer_staff_ids as $engineer_staff_id) {
    $staff_history = $r_staff_history->findWhere([
        'staff_id' => $engineer_staff_id
    ]);
    if (is_null($staff_history)) {
        continue;
    }
    $staff_history_ids[] = $staff_history->id
}
var_dump($staff_history_ids);

冗長すぎや🐱💦

whereIn()を使う

$engineer_staff_ids = app()->make(\App\Repositories\Staff\StaffRepository::class)
    ->byWhere([
        'type' => 'engineer',
    ])->pluck('id')
      ->toArray();
$staff_history_ids = app()->make(\App\Repositories\Staff\StaffHistoryRepository::class)
    ->byWhereIn('staff_id', $engineer_staff_ids)
    ->pluck('id')
    ->toArray();
var_dump($staff_history_ids);

これや🐱✨

可読性良くなった！

リレーションを使ったり、リポジトリで定義してもええな。

* whereInを抽象化したbyWhereIn()をリポジトリのtraitで作って活用している。

もしくはWhereHas()やWhereExists()を使う

$staff_history_ids = app()->make(\App\Entities\StaffHistory::class)
    ->whereExists(function($query){
        $query->from('staffs')
            ->whereRaw('staffs.type', 'engineer');
})->pluck('id')
    ->toArray();
var_dump($staff_history_ids);

ええな、すっきりや🐱🌟

2回以上必要になるケースがあるなら、リポジトリで関数定義してもええ

ORMでの戻り値のnullや配列の判定

nullや空配列の判定してからプロパティを参照しているか

Laravel

first()はnullで返却される
if (!is_null($instance)) { … }
get()は空のCollectionで返却される
・配列に変換してからempty()で判定するパターン
if (empty($collection->toarray())) { … }
・->isEmpty()で判定するパターン
if ($collection->isEmpty()) { … }
どっちでも良いが->isEmpty()の方が可読性が高い

集計処理

処理速度優先、次に可読性
→リポジトリにSQLで集計関数を定義
Collectionで集計関数Sum()などを使うと、重くて使い物にならなくなったりする。集計はSQLで書く。

一括処理

バルクインサート、バルクアップデートで処理すること
→バルクインサート、バルクアップデートをTraitに定義しよおう

Laravel bulk insert バルクインサート

JSONのキー、初期値の管理

DB側でJSONのキーの管理はできない。

コード上で初期値を表現するとスパゲティになるのでやめよう。

config/content/xxxx.phpファイルを用意し、JSONを定義すること

JSONは初期値、key, valueはLaravelのconfigで配列で定義しておくこと

定数はコンフィグで定義しよう

基本はコンフィグに書くこと

config/const/xxx.php

URIのパラメータ

パスパラメータを利用すること
OK … /content/{content_id}/detail/{detail_id}
BAD … /content/detail?content_id=1&detail_id=1

パスパラメータであればルーティングにかけるので仕様を共有しやすいです。

論理削除されているデータの取得

->get() … 論理削除されているものは除外して取得
->all() … 論理削除の区別なく取得

これを利用しているリポジトリのメソッドに注意する。

リポジトリは直接ORMのメソッドを利用せずtraitで多様するメソッドを定義しておくと良い
定義しておかないと、メソッド毎にチェックすることになる。

変数やメソッドの命名の適切性

嘘をついていないか
わかりやすい名前であること
省略していないこと。冗長でもわかりやすく

ネストとキャメルのコード規約

例)

関数やクラスはキャメル
変数はスネーク

参照渡しを利用している

参照渡しは私的には非推奨。追いにくくなるし、処理を追加で実装しにくくなる。

特に配列の加工での参照渡しは非推奨。

参照渡しを利用しなくても実装できるぞ😼

PHP 参照渡しによる加工処理

インスタンス変数の濫用

バグを産むのでローカル変数で処理をするのが良い。他の関数にはパラメータで渡す。
データの塊の配列やオブジェクトをインスタンス変数に安易に入れないこと。
絶対に1度の処理で1回しか値が入らないものなら良い
例) __construct() {}で定義される変数

リポジトリで__construct()で他のリポジトリを生成しないこと

ループする可能性があるぞ！やめよう
しかもエラーがでないのでわかりにくい

リポジトリはEntity内の処理で納めて、他のリポジトリを使う場合はServiceで行いましょう。

レコードの存在判定で他のテーブルをJOINしたデータを利用したいならQueryビルダーで判定処理するのはあり。

同じコードは2度書かない。コピペしない

共通化
関数にして外だしする
用途のサービスクラスを作って使い回しや保守がしやすいように
ポリモーフィズム設計
２回以上利用する配列はコンフィグで定義する
都道府県など

バリデーション

３つのバリデーションを利用することでServiceクラスの中でバリデーションを書かなくて済むので、すっきりします。

Middleware
$requestやヘッダーの値を利用して$requestに配列をmergeしたり、バリデーションするのに使えます。
・$requestを共通加工したい時
・ヘッダーの値でバリデーション
Request
・バリデーション、Ruleによってバリデーションする
ServiceValidator
・バリデーションのメッセージが特殊な場合に使いやすい