比較 Archive

今回は、仕事の中で自分が書いた処理を、先輩に書き直された処理を備忘録として記述します。ずいぶん勝手なエントリですが（いつもですが＞＜）お付き合いして頂ければ幸いです。

今回の教訓は、頻繁に使う処理ならば、モジュールにして可読性をよくすることと、記述そのものの差異から学んだことです。

処理としては、大エリアに紐付く中エリア、中エリアに紐付く、小エリア、小エリアに紐付く会場情報を取得し出力するものです。
大エリア（big_areas）＞中エリア（medium_areas）＞小エリア＞会場（places）
そしてそれぞれのIdを使っています。

サンプルでは多少処理の整合性が合わない部分もありますが、今回の意図はオブジェクト指向なのでご了承ください。

では、以下自分のソースです。

		$place = $place_detail->getPlaceByPlaceId($place_id);

		$small_area_id = $place['small_area_id'];
		$this->af->set('small_area_id', $small_area_id);

		$big_area_id = $area_detail->getBigAreaIdBySmallAreaId($small_area_id);
		$medium_area_id = $area_detail->getMediumAreaIdBySmallAreaId($small_area_id);

		$this->af->set('medium_areas', $area_detail->getMediumAreasByBigAreaId($big_area_id));
		$this->af->set('small_areas', $area_detail->getSmallAreasByMediumAreaId($medium_area_id));

		$this->af->set('big_area_id', $big_area_id);
		$this->af->set('medium_area_id', $medium_area_id);
		$this->af->set('big_areas', $area_detail->getBigAreas());

		$this->af->set('places', $place_detail->getAffiliatedPlacesByAgentIdAndSmallAreaId($agent_id, $small_area_id));

次に以下、先輩のソースです。

    function setAreasAndPlaces() {
        // エリアの一覧を設定しておく
        $area_detail = new AreaDetail($this->backend);
        $place_detail = new PlaceDetail($this->backend);
        $big_area_id = $this->af->get('big_area_id');
        $medium_area_id = $this->af->get('medium_area_id');
        $small_area_id = $this->af->get('small_area_id');
        // 上のエリアIDが無ければ取ってくる
        if (is_numeric($small_area_id)) {
            if (!is_numeric($medium_area_id)) {
                $medium_area_id = $area_detail->getMediumAreaIdBySmallAreaId($small_area_id);
            }
            if (!is_numeric($big_area_id)) {
                $big_area_id = $area_detail->getBigAreaIdBySmallAreaId($small_area_id);
            }
        }
        // 大エリア
        $this->af->set('big_areas', $area_detail->getBigAreas());
        // 大エリアが決定していれば中エリア
        if (is_numeric($big_area_id)) {
            $this->af->set('big_area_id', $big_area_id);
            $this->af->set('medium_areas', $area_detail->getMediumAreasByBigAreaId($big_area_id));
        }
        // 小エリアも同様
        if (is_numeric($medium_area_id)) {
            $this->af->set('medium_area_id', $medium_area_id);
            $this->af->set('small_areas', $area_detail->getSmallAreasByMediumAreaId($medium_area_id));
        }
        // 会場も同様
        if (is_numeric($small_area_id)) {
            $this->af->set('small_area_id', $small_area_id);
            $places = $place_detail->getPlacesBySmallAreaId($small_area_id);
            $this->af->set('places', $places);
            $place_options = array(); // {html_options} 用
            foreach ($places as $p) {
                $place_options[$p['id']] = $p['name_japanese'];
            }
            $this->af->set('place_options', $place_options);
        }
    }

私が初めて上記の先輩の処理を見たとき感じたことは「分かりやすい」です。
コメントがあるから分かりやすいのでしょうが、コメントだけの力なのかと思ってしまうくらいきれいと感じました。
処理はまず初めに、フォームから送られた値を取得、変数に格納。次に大エリア、中エリア、小エリア、最後に会場と処理をしています。
自分の処理もそうしているはずなのに全然違います。

さらに、差異点は値が取得できなかった場合のエラーを考慮した処理もしています。そして、この処理は複数の箇所で使うので、モジュール化したほうがよいに決まってます。
たったこのライン数で天と地の差を見ることができるのはそれはそれですごいですね。

thank you for your time.

自分の処理と人の処理を比べて、自分の見識を広げようとします。

今回のケーススタディはエラー処理の記述です。
以下自分の処理です。

$tel_names = array('telephone_1', 'telephone_2', 'telephone_3');
$tel_error_flag = false;
$tel_empty_cnt = 0;
$regex = '/^[0-9０-９]+$/';

foreach ($tel_names as $tel_name) {
	$tel = $this->af->get($tel_name);
	if( empty($tel) ) {
		$tel_error_flag = true;
		$tel_empty_cnt++;
	} elseif (!preg_match($regex, $tel)) {
		$tel_error_flag = true;
    }
    if ($tel_empty_cnt == 3 && $tel_error_flag ) {
        $this->ae->add(null, '電話番号が入力されていません', E_FORM_INVALIDCHAR);
    }else if($tel_error_flag){
        $this->ae->add(null, '電話番号が正しく入力されていません', E_FORM_INVALIDCHAR);
    }
}

以下、先輩の処理です。

$regex = '/^[0-9０-９]+$/';

$telephone_1 = $this->af->get('telephone_1');
$telephone_2 = $this->af->get('telephone_2');
$telephone_3 = $this->af->get('telephone_3');

if (!$telephone_1 && !$telephone_2 && !$telephone_3) {
	$this->ae->add(null, '電話番号が入力されていません', E_FORM_INVALIDCHAR);
} else if (!preg_match($regex, $telephone_1) || !preg_match($regex, $telephone_2) ||
	!preg_match($regex, $telephone_3)) {
	$this->ae->add(null, '電話番号が正しく入力されていません', E_FORM_INVALIDCHAR);
}

一目瞭然。どっちがみやすいか分かります。
先輩いわく、変数も短くしないようにするのだと。できるかぎり、DBの名前と一致させると分かりやすいのだと。