実践リファクタリング2 早期Returnのサンプル

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

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

販売情報取得処理

以下はLaravelで書かれている販売取得処理です。

public function showList($customer_id) {
    // do something

        // 締めされていない販売はエラー
        if (is_null($collection_daily_reports)) {
            $object_sell->cutoff_confirm_date = null;
        } else {
            $daily_report = $collection_daily_reports->where('dept_id', $object_sell->sell_dept_id)
                            ->where('record_date', date('Y-m-d', strtotime($object_sell->record_date)))
                            ->first();
            if (is_null($daily_report)) {
                $object_sell->cutoff_confirm_date = null;
            } else {
                $object_sell->cutoff_confirm_date = $daily_report->cutoff_confirm_date;
            }
        }

    // do something
}

if文の中にさらにif文があるケースです。

このままでも問題ないですが、メソッドshowListの中では他に色々な処理を行っているので、試しにここの処理を分離して、かつ、if文のネストをなくすとどうなるかを試してみます。

メソッドに切り出す

まずはそのままメソッドに切り出します。

このメソッド内では$object_sellと$collection_daily_reportsを使っているので、引数として渡しています。

メソッド名も処理にあった名前にしておきます。

    private function checkIfDailyReports($object_sell, $collection_daily_reports) {

        if (is_null($collection_daily_reports)) {
            $object_sell->cutoff_confirm_date = null;
        } else {
            $daily_report = $collection_daily_reports->where('dept_id', $object_sell->sell_dept_id)
                ->where('record_date', date('Y-m-d', strtotime($object_sell->record_date)))
                ->first();
            if (is_null($daily_report)) {
                $object_sell->cutoff_confirm_date = null;
            } else {
                $object_sell->cutoff_confirm_date = $daily_report->cutoff_confirm_date;
            }
        }

    }

Return文を書く

この処理のあとで使っている変数は$object_sellのみなのでreturnで返すようにします。

    private function checkIfDailyReports($object_sell, $collection_daily_reports) {

        if (is_null($collection_daily_reports)) {
            $object_sell->cutoff_confirm_date = null;
        } else {
            $daily_report = $collection_daily_reports->where('dept_id', $object_sell->sell_dept_id)
                ->where('record_date', date('Y-m-d', strtotime($object_sell->record_date)))
                ->first();
            if (is_null($daily_report)) {
                $object_sell->cutoff_confirm_date = null;
            } else {
                $object_sell->cutoff_confirm_date = $daily_report->cutoff_confirm_date;
            }
        }
        
        return $object_sell;
    }

これまでの変更で、このメソッドをみた人はコードをちゃんと読まなくても、なんとなく以下のことがわかるようになります。

  • $object_sellを受け取って変更して返す
  • 第二引数の$collection_daily_reportsはおそらくこの処理の中だけで使われている(returnされていないので)
  • メソッド名から、daily_reportsをチェックして条件分岐するような処理である

if文のネストを取り除く

次に早期Returnをしてif文のネストを取り除きます。

    private function checkIfClosed($object_sell, $collection_daily_reports) {

        if (is_null($collection_daily_reports)) {
            $object_sell->cutoff_confirm_date = null;
            return $object_sell;
        }
        
        $daily_report = $collection_daily_reports->where('dept_id', $object_sell->sell_dept_id)
            ->where('record_date', date('Y-m-d', strtotime($object_sell->record_date)))
            ->first();
        if (is_null($daily_report)) {
            $object_sell->cutoff_confirm_date = null;
        } else {
            $object_sell->cutoff_confirm_date = $daily_report->cutoff_confirm_date;
        }
        

        return $object_sell;
    }

はじめのis_null($collection_daily_reports)の判定をするところで$object_sellをreturnしてしまいます。

これでif文のネストがなくなります。

if文のネストは人間にはストレス

if文のネストがあると人間がわかりにくく感じる理由を説明します。

例えば以下のコードがあるとします。

if (//条件A) {
    //処理1
} else {
    if (条件B) {
        //処理2
    } else {
        //処理3
    }
}

処理2まで読んだ時に、「条件Bの場合は処理2だけど、さらにもう一つ前に条件があったな」なんだっけ？となります。

つまり、一つ上の階層の条件(条件A)を思い出す必要が出てきます。

ここでコードを上に戻って読む必要がでてきます。

この例では二階層だからまだ良いですが、三階層・四階層となると、この前に戻って読むという作業がさらにストレスになります。

早期Returnをすることで、一階層上の条件はなんだっけ？をする必要がなくなります。

if文のネストが深くなってしまった場合、早期Returnを使えないか検討してみると良いと思います。

Laravel, リファクタリング

Posted by たみぼうず