Changes between Initial Version and Version 1 of EC-CUBE標準規約/リファクタリングガイドライン

Timestamp:

2011/03/23 16:33:28 (13 years ago)

Author:

nanasess

Comment:

wiki:リファクタリングガイドライン から移動

EC-CUBE標準規約/リファクタリングガイドライン

@@ +1 @@
+= リファクタリングガイドライン =
+
+== init 関数 ==
+init 関数は, クラスの初期化を目的する.
+ビジネスロジックの記述はしないこと
+
+{{{
+#!php
+<?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
+<?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
+<?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
+<?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
+<?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
+<?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
+<?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);
+    return $arrResults;
+}
+$arrResults = getResults();
+$arrResults2 = getResults($arrParams);
+
+?>
+}}}
+
+INSERT/UPDATE/DELETE は, SC_Query::insert(), SC_Query::update(),  SC_Query::delete() を使用し, SC_Query::query() は使用しないこと
+
+{{{
+#!php
+<?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 を使用すること
+
+== 端末種別の判別 ==
+
+`Net_UserAgent_Mobile::isMobile()` や `MOBILE_SITE` 定数など存在するが, `SC_Display::detectDevice()` に統一する
+
+== スーパーグローバル変数 ==
+
+`$_POST` や `$_GET` の値は, 必ず入力チェック後に使用する. `SC_FormParam` を使用していれば, `SC_FormParam::getHashArray()` で取得できる
+
+{{{
+#!php
+<?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
+<?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
+<?php
+
+/**
+ * NG 一見して, どんなデータが格納されているかわからない
+ */
+$arrRet = getProducts();
+
+/**
+ * OK 商品データの配列だと, 変数名で判別可能
+ */
+$arrProducts = getProducts();
+
+?>
+}}}
+
+リテラルを格納する (つまり配列・オブジェクト以外の) 変数名は, すべて小文字を使用し, アンダーバーで区切ること
+
+{{{
+#!php
+<?php
+
+/**
+ * リテラルを格納する変数名は, 連想配列の添字で使用されることも考慮し
+ * アンダーバー区切りの方が扱いやすい
+ */
+
+/**
+ * NG
+ */
+$productId = getProductId();
+
+/**
+ * OK
+ */
+$product_id = getProductId();
+
+?>
+}}}
+
+== 1行の文字数/行数 ==
+ * 1行の文字数は80文字までにすることを目指すこと. すなわち, コードの長さを現実的な範囲で80文字までにおさめるよう努力すべきです. しかしながら, 場合によっては少々長くなってしまってもかまいません.
+ * 1関数の行数は多くても200行程度に留めるのが望ましい. これ以上行数が増えてしまう場合は, 関数を分割する等, コーディングの見直しを行ってください
+
+== PHPDoc コメント ==
+
+関数の PHPDoc コメントは必ず記述する.
+  * 関数の説明
+  * 引数
+    * 引数の無い関数は省略可能
+  * 返り値
+    * 返り値が無くても void を明記すること
+
+== 非推奨機能 ==
+
+error_reporting(E_ALL) で, エラー及び警告が出力されないようコーディングすること.
+
+PHP5.3 で非推奨となっている関数は使用しないこと.
+
+  * call_user_method() (かわりに call_user_func() を使用します)
+  * call_user_method_array() (かわりに call_user_func_array() を使用します)
+  * define_syslog_variables()
+  * dl()
+  * ereg() (かわりに preg_match() を使用します)
+  * ereg_replace() (かわりに preg_replace() を使用します)
+  * eregi() (かわりに preg_match() で 'i' 修正子を使用します)
+  * eregi_replace() (かわりに preg_replace() で 'i' 修正子を使用します)
+  * set_magic_quotes_runtime() およびそのエイリアスである magic_quotes_runtime()
+  * session_register() (かわりにスーパーグローバル `$_SESSION` を使用します)
+  * session_unregister() (かわりにスーパーグローバル `$_SESSION` を使用します)
+  * session_is_registered() (かわりにスーパーグローバル `$_SESSION` を使用します)
+  * set_socket_blocking() (かわりに stream_set_blocking() を使用します)
+  * split() (かわりに preg_split() を使用します)
+  * spliti() (かわりに preg_split() で 'i' 修正子を使用します)
+  * sql_regcase()
+  * is_dst を mktime() に渡すこと。 かわりにタイムゾーン処理用の新しい関数を使用します。
+
+以下の機能も, PHP5.3.x で非推奨となっているが, PHP4互換のために使用しても良い
+
+  * new の返り値を参照で代入すること
+  * 呼び出し時の参照渡し
+
+参考
+http://www.php.net/manual/ja/migration53.deprecated.php
+
+== PHP4 での動作 ==
+
+PHP4.3.11 以降で動作可能な構文にすること
+
+参考
+http://svn.ec-cube.net/open_trac/ticket/854
+http://svn.ec-cube.net/open_trac/changeset/20024
+