2つの関数を呼び出した後の処理について

Question

API1と2を入力の条件のよって呼び分ける必要があるのですが、1も2も処理後の返却結果が同じため下記のようにAPIの結果を変数に入れ、呼び出した後の処理をまとめたのですがこういった書き方は変でしょうか。
妙案だと思ったのですが書いた後に見ると同じ変数に値を格納しているので微妙かなと考えています。共に更新処理になります。

        //呼び出し１
        if(substr($req['id'], 0, 3) != 'XXX'){
            if($this->is_time_change($req)){
                $req["rain_flag"]= 0;
            }
            $res = $this->_mountain_sch->update($req);
        }
        //呼び出し２
        if(substr($req['id'], 0, 3) == 'XXX'){
            $req['day_clim_flg'] = 0;
            $res = $this->_mountain_reg->update($req);
        }
        if ($res["result"] === 'success') {
            $furont_result = array('result' => $res["result"]);
        }
        if ($res["result"] === 'fail') {
            if(!empty($res['over_flag'])){
                $furont_result = array('result' => $res["result"] , 'date_info' => $res["info"]);
            }
        }
        return $furont_result;

Comments

[m.ts10806]

if(substr($req['id'], 0, 3) == 'XXX')){　　←閉じかっこが１つ多いですね。転記ミスはなるべくなくしていただければ助かります。

Answer 1

別に悪くはないと思いますが、substr が2回、if も2回実行されるので、$id 等に一度退避しておいた方が綺麗になると思います。

        $id = substr($req['id'], 0, 3);
        if ($id != 'XXX') {
            //呼び出し１
            if($this->is_time_change($req)){
                $req["rain_flag"]= 0;
            }
            $res = $this->_mountain_sch->update($req);
        } else {
            //呼び出し２
            $req['day_clim_flg'] = 0;
            $res = $this->_mountain_reg->update($req);
        }

        switch ($res["result"]) {
        case 'success':
            $furont_result = array('result' => $res["result"]);
            break;
        case 'fail':
            if(!empty($res['over_flag'])){
                $furont_result = array('result' => $res["result"] , 'date_info' => $res["info"]);
            }
            break;
        default:
            throw new Exception('invalid result');
        }
        return $furont_result;

Answer 2

2つのメソッドが同時に実行されるようではないので、ifが1回1回切れるのは無駄かなと思います。
いっそのことメソッド名を変数に入れるのはどうでしょうか。
（エラーはありませんが動作自体は未検証です）

$updatemethod = "_mountain_";
$idstr = substr($req['id'], 0, 3);
//呼び出し１
if($idstr != 'XXX'){
	if($this->is_time_change($req)){
		$req["rain_flag"]= 0;
	}
	$updatemethod .= "sch";
}else{
	//呼び出し２
	$req['day_clim_flg'] = 0;
	$updatemethod .= "reg";
}

$res = $this->$updatemethod->update($req);

返却値の$furont_resultもdate_infoがあるかないかだけなのでまとめれそうですが、ひとまず参考まで。

Comments

[m.ts10806]

mattnさんのおっしゃるようにsubstr の結果を一度退避しておいたほうが綺麗ですね・・

Answer 3

可読性、今後の修正しやすさなどを考慮するとやめておいた方がいいかもしれません。
やはりそれぞれで処理を明確にした方がよいかと思います。
私なら下記のようにそれぞれ関数化すると思います。
またsubstr($req['id'], 0, 3)の処理が2回以上出てきて冗長です。

		if(substr($req['id'], 0, 3) != 'XXX'){
			//呼び出し１
			$furont_result = UpdateMountainSch($req);
		} else {
			//呼び出し２
			$furont_result = UpdateMountainReg($req);
		}
        return $furont_result;
	}

	function UpdateMountainSch($req)
	{
		if($this->is_time_change($req)){
			$req["rain_flag"]= 0;
		}
		$res = $this->_mountain_sch->update($req);

        return ResultCommon($res);
	}

	function UpdateMountainReg($req)
	{
		$req['day_clim_flg'] = 0;
		$res = $this->_mountain_reg->update($req);

        return ResultCommon($res);
	}

	function ResultCommon($res)
	{
        if ($res["result"] === 'success') {
            $furont_result = array('result' => $res["result"]);
        } else {
            if(!empty($res['over_flag'])){
                $furont_result = array('result' => $res["result"] , 'date_info' => $res["info"]);
            }
        }
        return $furont_result;
	}

Comments

[mattn]

呼び出し１と呼び出し２は if else の関係にあるべきかと思います。

[ttyp03]

呼び出し１は！＝でしたか。気づきませんでした。
それならif文で良いですね。
修正しておきます。
指摘ありがとうございます。

[enigumalu]

ありがとうございます。コーディング規約に近いため実際はこちらを咀嚼した形で利用させていただきました

Answer 4

失敗した時の後始末の処理を書くことを考えると、どちらのAPIで失敗したのかという情報を返して欲しいと思います。

質問のコードを使う側にとって、「失敗」と返ってきた時に、「えっと、どっちのAPIを呼び出したんだっけ、、substr($req['id'], 0, 3)の値を求めてっと、、、」というような事を考えないといけないのは負担ですし、バグがあった時に見つけにくいですから。

異なる機能(API)を引数に応じて呼び出しをするような関数ではなく、関数名で明確に区別できる２つの関数として実装するのが素直なように思います。

Comments

[enigumalu]

すいませんつまりケースとして
if(substr($req['id'], 0, 3) != 'XXX')
if(substr($req['id'], 0, 3) == 'XXX')
の場合をそれぞれの関数にした方が良いということでしょうか？

[coco_bauer]

そうです。$req(['id'], 0, 3) == 'XXX'という判断で呼び出されるAPIが違うというのは、使う側には(デバッグなどでコードを追う側にとっても）判りにくいので、$req(['id'], 0, 3) == 'XXX'の判断は呼び出す側で行って、２つの関数から呼び出す関数を選ぶほうが良いと思います。