wiki:リファクタリングガイドライン

Context Navigation

Version 3 (modified by nanasess, 15 years ago) (diff)
体裁修正

リファクタリングガイドライン

init 関数

init 関数は, クラスの初期化を目的する. ビジネスロジックの記述はしないこと

<?php
function init() {

    /**
     * NG ビジネスロジックの記述はしない
     */
    if (count($this->arrPayment) > 0) {
        $i = 0;
        foreach ($this->arrPayment as $val) {
            $this->payment[$i] = $val;
            $i++;
        }
    }

    /**
     * OK クラスの初期化のみ行う
     */
    $this->tpl_mainpage = 'index.tpl';
    $this->arrDISP = $masterData->getMasterData("mtb_disp");
}

action 関数

action 関数は MVC のコントローラにあたり以下の処理を記述する.

条件分岐
VIEW からの入力
VIEW に渡すメンバ変数($this->variable_name)への代入
宣言
ビジネスロジックの呼び出し

action 関数に, ビジネスロジックを直接記述したり, SQL を実行する処理を記述しないこと.

<?php
/**
 * action 関数のサンプル
 */
function action() {

    /** OK VIEW からの入力を処理する */
    $this->arrForm = $this->lfConvertParam($_POST);

    /** OK 入力に応じて条件分岐する */
    switch ($this->getMode()) {
    case 'edit':

        /** OK エラーの内容に応じて条件分岐する */
        $this->arrErr = $this->lfProductClassError($this->arrForm);
        if (empty($this->arrErr)){
            $this->tpl_mainpage = 'products/product_class_confirm.tpl';
            $this->lfProductConfirmPage($this->arrForm);
        } else {
            $this->doPreEdit($this->arrForm, false ,true);
        }
        break;

    case 'delete':
        $this->doDelete($this->arrForm);
        break;

    case 'disp':
        /** NG ビジネスロジックの記述をしてはならない */
        foreach ($this->arrForm as $key => $val) {
            $foo[$key] = $val;
        }

        /** NG SQLを実行する処理を記述してはならない */
        $objQuery =& SC_Query::getSingletonInstance();
        $arrResults = $objQuery->select("*", "table_name");

        $this->doDisp($this->arrForm);
        break;

    case 'complete':
        $this->registerProductClass($this->arrForm, $this->arrForm['product_id']);
        SC_Response_Ex::sendRedirect("complete.php");
        break;

    default:
    }
}
?>

MODE パラメータ

$_POST['mode'] や $_GET['mode'] に対する条件分岐は, LC_Page::getMode() を使用し, switch 文で記述する.

<?php
/** OK switch 文で mode の分岐を行う */
switch ($this->getMode()) {
case 'disp':
    $this->doDisp($this->arrForm);
    break;

case 'complete':
    SC_Response_Ex::sendRedirect("complete.php");
    break;
default:
}

/** NG mode の条件分岐に if 文を使用してはならない */
if ($_POST['mode'] == 'disp') {
    $this->doDisp($this->arrForm);
} elseif ($_POST['mode'] == 'complete') {
    SC_Response_Ex::sendRedirect("complete.php");
}
?>

ビジネスロジック

ページ間で重複する処理が存在する場合は, Helper などの共通クラスへ記述する
ページ固有の処理は, ローカル関数で記述する
可能であれば, PHPUnit を使用してテストケースを残すこと
宣言を除き, 引数や返り値が無く, すべて内部のメンバ変数で処理するような関数は極力作成しない
- ステートレスな処理を心掛けること

<?php

/**
 * NG 引数ではなく, クラスのメンバ変数を使用して振舞いを変更する
 */
function lfSendMail() {
     $objPurchase = new SC_Purchase_Ex();
     $objPurchase->sendOrderMail($this->order_id)
}

/**
 * OK 引数で振舞いを変更できる
 */
function lfSendMail($order_id) {
     $objPurchase = new SC_Purchase_Ex();
     $objPurchase->sendOrderMail($order_id)
}

/**
 * NG 引数や返り値が無く, メンバ変数の値を直接変更している
 */
function lfConvertLoginPass(){
    if(strlen($this->arrForm["login_pass"]) < 1 ) {
        return;
    }
    $this->arrForm["login_pass"] = trim($this->arrForm["login_pass"]);
    $this->arrForm["login_pass1"] = $this->arrForm["login_pass"];
    $this->arrForm["login_pass2"] = $this->arrForm["login_pass"];
}

/**
 * OK ローカル関数内では, ローカル変数を使用することで, 拡張性, 保守性が向上し, ユニットテストも書きやすい
 */

$this->arrForm = $this->lfConvertLoginPass($this->arrForm);

function lfConvertLoginPass(&$arrForm){
    if(strlen($arrForm["login_pass"]) < 1 ) {
        return;
    }
    $arrForm["login_pass"] = trim($arrForm["login_pass"]);
    $arrForm["login_pass1"] = $arrForm["login_pass"];
    $arrForm["login_pass2"] = $arrForm["login_pass"];
    return $arrForm;
}

/**
 * ユニットテストの例.
 */
function testLfConvertLoginPass() {
    $login_pass = "login_pass_value";
    $arrForm = array("login_pass" => $login_pass);

    $expected = array("login_pass" => $login_pass,
                      "login_pass1" => $login_pass,
                      "login_pass2" => $login_pass);

    $actual = lfConvertLoginPass($arrForm);

    $this->assertEquals($expected, $actual);
}
?>

ループ処理などで, メンバ変数や, スーパーグローバル変数は直接使わない

<?php

/**
 * NG ループ処理で, メンバ変数やスーパーグローバル変数を直接扱っている
 */
foreach ($_POST['params'] as $key => $val) {
     $this->arrForm[$key] = $val;
}

/**
 * OK メンバ変数や, スーパーグローバル変数に作用するループ処理は, 別途関数を作成する
 */
$this->arrParams = $this->getParams($_POST['params']);

function getParams($arrParams) {
     $arrResults = array();
     foreach ($arrParams as $key => $val) {
         $arrResults[$key] = $val;
     }
     return $arrResults;
}

?>

データベースアクセス

特に理由の無い場合は SC_Query::getSingletonInstance() を使用してインスタンスを取得すること

<?php

/**
 * NG
 * PHP4 環境では, 新たなインスタンスが生成されてしまう
 */
$objQuery = new SC_Query();

/**
 * OK シングルトンが保証され, トランザクションの扱いが容易になる
 */
$objQuery =& SC_Query::getSingletonInstance();

?>

SQL 文を散乱させない. SELECT id, name, foo, bar FROM table_name と SELECT id, name, foo, bar, too FROM table_name が存在する場合は, 後者を共通関数にリファクタリングすること

<?php

/**
 * NG 類似した SQL を散乱させてはいけない
 */
$arrResults = $objQuery->select("id, name, foo, bar", "table_name");
$arrResult2 = $objQuery->select("id, name, foo, bar, too", "table_name", "too = ?", $arrParams['too']);

/**
 * OK カラム名や, WHERE の違いなどは共通関数で吸収する
 */
function getResults($arrParams = array()) {
    $where = "";
    $arrValues = array();
    if (isset($arrParams['too'])) {
        $where .= "too = ?";
        $arrValues[] = $arrParams['too'];

    }
    $objQuery =& SC_Query::getSingletonInstance();
    $arrResults = $objQuery->select("*", "table_name", $where, $arrValues);
}
$arrResults = getResults();
$arrResults2 = getResults($arrParams);

?>

INSERT/UPDATE/DELETE は, SC_Query::insert(), SC_Query::update(), SC_Query::delete() を使用し, SC_Query::query() は使用しないこと

<?php

/** NG SC_Query::query() は使用しない */
$objQuery =& SC_Query::getSingletonInstance();
$objQuery->query("INSERT INTO table_name (col1, col2) VALUES (?, ?)", arrary($col1, $col2));

/** OK SC_Query::insert(), SC_Query::update(),  SC_Query::delete() を使用する */
$objQuery =& SC_Query::getSingletonInstance();
$objQuery->insert("table_name", array("col1" => $col1,
                                      "col2" => $col2));

?>

入力チェック

入力チェック用のクラスには, SC_FromParam と SC_CheckError が存在するが, 極力 SC_FormParam を使用すること

スーパーグローバル変数

$_POST や $_GET の値は, 必ず入力チェック後に使用する. SC_FormParam を使用していれば, SC_FormParam::getHashArray() で取得できる

<?php

/** NG $_POST の値を入力チェックせずに使用する */
$this->register($_POST);

/** OK 必ず入力チェックを行う */
$objFormParam = new SC_FormParam();
$objFormParam->addParam("商品規格ID", "product_class_id", INT_LEN, "n 
", array("EXIST_CHECK", "MAX_LENGTH_CHECK", "NUM_CHECK"));
$objFormParam->setParam($_POST);
$objFormParam->convParam();
$arrErr = $objFormParam->checkError()
if (SC_Utils_Ex::isBlank($arrErr)) {
     // 入力チェック後の値が取得可能
     $arrForm = $objFormParam->getHashArray();
}
$this->register($arrForm);
?>

$_SESSION は, SC_CartSession クラス等, セッションを扱うビジネスロジックを通じて使用するのが望ましい.

どうしても, グローバル変数を使用したい場合は, global キーワードは使用せず, $GLOBALS を使用すること.

<?php
/**
 * NG global キーワードの使用すると, 変数名衝突の原因となる
 */
function globalSample() {
    global $conn;

    // 外部からは $conn という変数を使用しているのがわからない
    if (is_null($conn)) {
        $conn =& new ClassName();
    }
}

/**
 * OK $GLOBALS を使用すると, ローカル変数との衝突の心配は無い
 */
function globalSample() {
    if (is_null($GLOBALS['_conn'])) {
        $GLOBALS['_conn'] = new ClassName();
    }
}

?>

変数名

変数名で, どんなデータか識別できるよう考慮すること

<?php

/**
 * NG 一見して, どんなデータが格納されているかわからない
 */
$arrRet = getProducts();

/**
 * OK 商品データの配列だと, 変数名で判別可能
 */
$arrProducts = getProducts();

テーブルカラム名と共通の変数名は, キャメル式を使用せず, アンダーバーで区切ること

<?php

/**
 * テーブルカラム名と共通の変数名は, 連想配列の添字で使用されるため,
 * アンダーバー区切りの方が扱いやすい
 */

/**
 * NG
 */
$productId = getProductId();

/**
 * OK
 */
$product_id = getProductId();