実践リファクタリング1

実際のプロダクトのコードを読んでみて、自分が理解しづらかったところと、リファクタリングした結果を記載します。

※実際のコードを一部変更しています。

とある伝票取得処理

以下はLaravelで書かれている伝票取得処理です。今回はshowメソッドを見るのでその他のメソッドは削除しています。

class OutflowTransportService
{

    /**
     *
     * @param
     *            $id
     * @return
     */
    public function show($id)
    {
       
        // 出庫情報の単票を取得
        // 初期クエリの設定
        $query = V_outflow_transport::query();
        // レスポンス定義
        $ret = array();
        // エラーメッセージ
        $collection = new Collection();

        // 対象データのみ取得（最新の履歴を取得）
        $t_outflow_transport = V_outflow_transport::find($id);

        if (empty($t_outflow_transport)) {

            $collection->push(Lang::get('messages.warning.outflow_transport.not_found'));

            $message = collect([
                "message" => $collection
            ]);
            throw new HttpResponseException(response()->json($message, config('const.status_unprocessable')));
        }

        // デフォルト条件（枝番を取得）
        // 削除フラグ
        $query->where('del_flag', '0');
        $query->where('outflow_transport_num', $t_outflow_transport->outflow_transport_num);

        // ソート順指定
        $query->orderBy('branch_num', 'desc');

        // 単票を取得
        $ret = $query->get();

        // 依頼書の取得
        $repair_request = null;
        $outflow_transport = $ret->first();
            // 依頼との紐づきを確認
        if (empty($outflow_transport->accept_order_id) || empty($outflow_transport->transport_req_id)) {
                $r_query = T_repair_request::query();
            $outflow_transport_id = $outflow_transport->id;
                $r_query->where('del_flag', '0');

                $r_query->where(function ($r_query) use ($outflow_transport_id) {
                    $r_query->orWhere('repair_goods_outflow_transport_id', $outflow_transport_id)
                        ->orWhere('red_deliv_paper_id', $outflow_transport_id)
                        ->orWhere('black_deliv_paper_id', $outflow_transport_id);
                });

                $repair_request = $r_query->first();
        }

        foreach ($ret as $row) {
            $row->outflow_transport_dtls;

                if (! empty($repair_request)) {
                    $row->repair_req_id = $repair_request->id;
                    $row->repair_req_num = $repair_request->repair_req_num;
                }
            }

        // デバッグSQL
        return $ret;
    }
}

初見の感想

初見でこのコードをみて思った点は以下です。

日本語のコメントが多め

コメントが多いため読み進めるのを助けられました。

ただ、少し多すぎる気もします。

コメントを書くのは悪くはないですが、コメントに頼ってしまうとわかりやすいコードを書く練習の妨げになるため、どのような処理をしているかに対するコメントは削除してもいいと思います。

コメントを書く場合は、なぜこのような処理にしているのかなどコードみただけでは分からないようなことのみにするといいと思いました。

変数名がわかりにくかった

以下の変数は、コメントなしでは何のためかわかりませんでした。

// エラーメッセージ
$collection = new Collection();

処理の流れ、塊がわかりにくい

このshowメソッドでは伝票を取得するための一連の処理が書かれていますが、どのような処理の塊があるかがわかりにくかったです。

一通り読んでみると以下のような処理の流れになっていました。

  • クエリーの作成
  • レスポンスの変数の定義
  • 最新履歴の取得と存在チェック
  • クエリーの編集
  • クエリー実行
  • 依頼書の紐付きチェック、依頼書のマージ
  • 結果を返す

クエリー関連の処理がまとまっていないので、まとめることで可読性があがると思います。

リファクタリングしてみる

動作確認までしていないので、あくまでどのようにやるかというだけです。

変数をわかりやすくする

エラーの変数をわかりやすい名前にして、コメントを削除します。これだけでこの変数が何に使われるのかがこの時点でわかります。

$errors= new Collection();

クエリーの作成処理をまとめる

クエリーの初期化と構成が離れてしまっているので一つにまとめます。

$ret = V_outflow_transport::query()
            ->where('del_flag', '0')
            ->where('outflow_transport_num', $t_outflow_transport->outflow_transport_num)
            ->orderBy('branch_num', 'desc')
            ->get();

これによって、変数の$queryを削除することが可能です。

変数が多いとそれだけ読む時にストレスがかかります。各変数がどこで使われていてどこで変更されているかなどを常に気に掛ける必要があるからです。

変数が多いとコードを上から下に読めず一旦上に戻るという作業が起きがちでこれは結構疲れます。日本語や英語を読む時と同じで、できるだけ戻らずに読めると効率が上がります。

依頼書のバインド処理をprivateメソッドに分ける

依頼書のバインド処理は別途メソッドに切り出します。

    private function getRelatedRepair($outflow_transport){
        $repair_request = null;

        $outflow_transport = $outflow_transport->first();

        // 修理依頼との紐づきを確認
        if (empty($outflow_transport->accept_order_id) || empty($outflow_transport->transport_req_id)) {
            $r_query = T_repair_request::query();
            $outflow_transport_id = $outflow_transport->id;
            $r_query->where('del_flag', '0');

            $r_query->where(function ($r_query) use ($outflow_transport_id) {
                $r_query->orWhere('repair_goods_outflow_transport_id', $outflow_transport_id)
                    ->orWhere('red_deliv_paper_id', $outflow_transport_id)
                    ->orWhere('black_deliv_paper_id', $outflow_transport_id);
            });

            $repair_request = $r_query->first();
        }

        return $repair_request;
    }

showメソッド内は getRelatedRepair を実行するという一行になりました。

また、getRelatedRepairというメソッド名から何をしているのかわかりやすくなります。

$repair_request = $this->getRelatedRepair($ret->first());

リファクタリング後

以下のようになりました。

処理のまとまりごとに記載されるようになり、不要な変数が減ったことで読みやすくなったと思います。また、依頼書の処理を別メソッドにすることで、ここだけの単体テストも可能になりました。

class OutflowTransportService
{

    public function show($id)
    {

        $ret = array();
        $errors = new Collection();

        $t_outflow_transport = V_outflow_transport::find($id);
        if (empty($t_outflow_transport)) {

            $errors->push(Lang::get('messages.warning.outflow_transport.not_found'));

            $message = collect([
                "message" => $errors
            ]);
            throw new HttpResponseException(response()->json($message, config('const.status_unprocessable')));
        }

        $ret = V_outflow_transport::query()
            ->where('del_flag', '0')
            ->where('outflow_transport_num', $t_outflow_transport->outflow_transport_num)
            ->orderBy('branch_num', 'desc')
            ->get();

        $repair_request = $this->getRelatedRepair($ret->first());

        foreach ($ret as $row) {
            $row->outflow_transport_dtls;

                if (! empty($repair_request)) {
                    $row->repair_req_id = $repair_request->id;
                    $row->repair_req_num = $repair_request->repair_req_num;
                }
            }

        return $ret;
    }

    private function getRelatedRepair($outflow_transport){
        $repair_request = null;

        $outflow_transport = $outflow_transport->first();

        if (empty($outflow_transport->accept_order_id) || empty($outflow_transport->transport_req_id)) {
            $r_query = T_repair_request::query();
            $outflow_transport_id = $outflow_transport->id;
            $r_query->where('del_flag', '0');

            $r_query->where(function ($r_query) use ($outflow_transport_id) {
                $r_query->orWhere('repair_goods_outflow_transport_id', $outflow_transport_id)
                    ->orWhere('red_deliv_paper_id', $outflow_transport_id)
                    ->orWhere('black_deliv_paper_id', $outflow_transport_id);
            });

            $repair_request = $r_query->first();
        }

        return $repair_request;
    }
}

今後は切り出したgetRelatedRepairを整理していくことも可能です。

Laravel, リファクタリング

Posted by たみぼうず