[フロント]会員登録

[kotani]

▼リファクタリング対象ファイル
/entry/LC_Page_Entry.php
/entry/LC_Page_Entry_Complete.php
/entry/LC_Page_Entry_EmailMobile.php
/entry/LC_Page_Entry_Kiyaku.php
/regist/LC_Page_Regist.php
/regist/LC_Page_Regist_Complete.php

---

リファクタリングガイドライン
http://svn.ec-cube.net/open_trac/wiki/リファクタリングガイドライン

---

[kajiwara]

kimoto様

株式会社ロックオンの梶原でございます。

会員登録という巨大な部分にアサインいただき、誠に恐縮です。
何卒よろしくお願い致します！

まず現時点で、一度動作確認をさせていただきました。

■仮会員登録 1．管理画面＞システム設定＞パラメータ設定　で、「CUSTOMER_CONFIRM_MAIL」を「true」に変更。（仮会員ONにする）
2．会員登録
3．本下院登録案内メール本文にあるURLへアクセスするとエラー

仮会員登録の３のところでエラーとなります。
申し訳ございませんが、ご確認いただけると幸いです。

また、別途ソースの確認を行い、フィードバックさせていただきます。

リファクタリングを開始したばかりで、色々分からないことがあると思いますが、随時フィードバックさせていただきますので、何卒よろしくお願い致します！！

[kimoto]

株式会社ロックオンの梶原さま

■仮会員登録 1．管理画面＞システム設定＞パラメータ設定　で、「CUSTOMER_CONFIRM_MAIL」を「true」に変更。（仮会員ONにする）
2．会員登録
3．本下院登録案内メール本文にあるURLへアクセスするとエラー

仮会員登録の３のところでエラーとなります。
申し訳ございませんが、ご確認いただけると幸いです。

もうしわけありません、確認漏れでした。 修正しましたのでご確認ください。

量があって時間がかかっていますが、なんとかしたいと思いますので今後ともよろしくお願いします

[kajiwara]

kimoto様
株式会社ロックオンの梶原でございます。

今回、リファクタリングガイドラインが結構多岐にわたっており、そんな中で作業いただいており、本当にありがとうございます。
というところで、少し時間がかかってしまいましたが、ソースの確認をさせていただきました。

以下の点がリファクタリングガイドラインから外れておりますので、修正いただけるとありがたいです。

（1）http://svn.ec-cube.net/open_trac/wiki/リファクタリングガイドライン#端種別の判別

• Net_UserAgent_Mobile::isMobile() や MOBILE_SITE 定数など存在するが, SC_Display::detectDevice() に統一する

（2）http://svn.ec-cube.net/open_trac/wiki/リファクタリングガイドライン#データベースアクセス

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

（3）http://svn.ec-cube.net/open_trac/wiki/リファクタリングガイドライン#ビジネスロジック

• 宣言を除き, 引数や返り値が無く, すべて内部のメンバ変数で処理するような関数は極力作成しない

こちらは分かりにくいと思いますので、lfMakeSqlVal　の修正方法を例にとり、説明させていただきます。
修正したファイルは本チケットに添付させていただきました。

<?php

/**
 * lfMakeSqlVal XXX 関数名は無くても良い
 *
 * 会員登録に必要なsqlを作成する
 *
 * XXX
 * 実際にSQL文を生成するけではないので「会員登録に必要なSQLパラメータを生成する」という概要の方が良いと思われる
 *
 * @access public XXX 外部からのアクセスは想定しないと思われるので protected では？
 * @return void XXX 連想配列を返しているので array SQLパラメータの配列 等とすべき
 */
function lfMakeSqlVal() {
    /*
     * XXX
     * 変数名 $arrRet は, どのようなデータか判別できないので不適切
     * 入出力が解らなくなるので, getHashArray() の結果と getDbArray() の結果を, 関数の引数で受けとるべき
     */
    $arrRet     = $this->objFormParam->getHashArray();
    $sqlval     = $this->objFormParam->getDbArray();

    // 登録データの作成 XXX 生年月日を作成している
    $sqlval['birth']  = SC_Utils_Ex::sfGetTimestamp($arrRet['year'], $arrRet['month'], $arrRet['day']);

    // 仮会員 1 本会員 2
    $sqlval["status"] = (CUSTOMER_CONFIRM_MAIL == true) ? "1" : "2";

    /*
      secret_keyは、テーブルで重複許可されていない場合があるので、
      本会員登録では利用されないがセットしておく。
    */
    $sqlval["secret_key"] = SC_Helper_Customer_Ex::sfGetUniqSecretKey();        // 会員登録キー

    $CONF = SC_Helper_DB_Ex::sfGetBasisData();
    $sqlval["point"] = $CONF["welcome_point"]; // 入会時ポイント

    // XXX SC_Display::detectDevice() == DEVICE_TYPE_MOBILE を使用する
    if ($this->isMobile === true) {
        // 携帯メールアドレス
        $sqlval['email_mobile']     = $sqlval['email'];
        //PHONE_IDを取り出す
        $sqlval['mobile_phone_id']  =  SC_MobileUserAgent::getId();
    }

    return $sqlval;
}

?>

主に、引数や返り値が無く、メンバ変数の値を直接変更している点においての違反となっておりまして、引数や返り値で制御ができる関数であることが望ましいという意図です。

お忙しいところ大変申し訳ございませんが、ご確認いただき、再度チャレンジいただけませんでしょうか。

会員登録分、量が多く、恐縮ですが、何卒よろしくお願い致します！！

[kajiwara]

kimoto様

株式会社ロックオンの梶原でございます。

もう一点、以下の部分もご確認いただければありがたいと思います。
returnせずに関数内に遷移が入っているものがありましたのでご報告します。

以下、コメントを入れております。

<?php
/**
 * lfCheckReferer
 * XXX 関数の概要を記載する
 *
 * @access public XXX 外部からのアクセスは想定されていないと思われるので protected では？
 * @return void XXX 遷移の可否を boolean で返して, 遷移は action 関数で行う
 */
function lfCheckReferer(){
    /**
     * 規約ページからの遷移でなければエラー画面へ遷移する
     * XXX 関数の説明は, 関数コメントに記載する
     */
    if ($this->isMobile === FALSE // XXX SC_Display::detectDevice() を使用する
        && empty($_POST) // XXX 直接スーパーグローバル配列を使用しない
        && !preg_match('/kiyaku.php/', basename($_SERVER['HTTP_REFERER'])) // XXX 直接スーパーグローバル配列を使用しない
        ) {
        // XXX 遷移は action 関数で行うこと
        SC_Utils_Ex::sfDispSiteError(PAGE_ERROR, "", true);
    }
}

?>

こちらを直すと以下のようになるかと思いますので、是非ご参照ください。

<?php
/**
 * kiyaku.php からの遷移の妥当性をチェックする
 *
 * 以下の内容をチェックし, 妥当であれば true を返す.
 * 1. 規約ページからの遷移かどうか
 * 2. PC及びスマートフォンかどうか
 * 3. $post に何も含まれていないかどうか
 *
 * @access protected
 * @param array $post $_POST のデータ
 * @param string $referer $_SERVER['HTTP_REFERER'] のデータ
 * @return boolean kiyaku.php からの妥当な遷移であれば true
 */
function lfCheckReferer(&$post, $referer){

    if (SC_Display::detectDevice() != DEVICE_TYPE_MOBILE
        && empty($post)
        && !preg_match('/kiyaku.php/', basename($referer))) {
        return true;
    } else {
        return false;
    }
}

?>

それでは、どうぞよろしくお願い致します！

[kimoto]

株式会社ロックオンの梶原さま

ご指摘ありがとうございます /entry/LC_Page_Entry.phpを修正してみました。

        // パラメータ管理クラス,パラメータ情報の初期化
        $this->objFormParam = new SC_FormParam();
        $this->lfInitParam();

は初期化と解釈してinit()内で行っておりますが、action内で定義すべきでしょうか？

[kajiwara]

kimoto様
本当にありがとうございます！

そして、とても良いご指摘、誠にありがとうございます。

ご指摘ありがとうございます /entry/LC_Page_Entry.phpを修正してみました。

        // パラメータ管理クラス,パラメータ情報の初期化
        $this->objFormParam = new SC_FormParam();
        $this->lfInitParam();

は初期化と解釈してinit()内で行っておりますが、action内で定義すべきでしょうか？ 

そうですね。
このコードは少し厄介なことに、ページごとで呼ぶタイミングが異なってる場合があるので、ここはaction内で行うということで統一したいと思います。

こちらのソースが参考になると思います。
http://svn.ec-cube.net/open_trac/changeset/20071

よろしくお願い致します。

[nanasess]

branches/version-2_5-dev/data/class/pages/entry/LC_Page_Entry_EmailMobile.php

@@ -121 +124 @@
 
     /**
-     * lfRegister
      *
+     * 携帯メールアドレスが登録されていないユーザーに携帯アドレスを登録する
+     *
+     * 登録完了後にsessionのemail_mobileを更新する
+     *
+     * @param mixed $objFormParam
      * @param mixed $objCustomer
-     * @access public
+     * @access private
      * @return void
      */
-    function lfRegister(&$objCustomer) {
-        $customer_id    = $objCustomer->getValue('customer_id');
-        $email_mobile   = strtolower($this->objFormParam->getValue('email_mobile'));
-
-        $objQuery       = new SC_Query();
-        $objQuery->update('dtb_customer', array('email_mobile' => $email_mobile), 'customer_id = ?', array($customer_id));
+    function lfRegistEmailMobile($email_mobile, $customer_id) {
+        $objQuery = SC_Query::getSingletonInstance();
+        $objQuery->update('dtb_customer',
+                          array('email_mobile' => $email_mobile),
+                          'customer_id = ?', array($customer_id));
 
         $objCustomer->setValue('email_mobile', $email_mobile);

LC_Page_Entry_EmailMobile::lfRegistEmailMobile() で SC_Customer を参照しているため, エラーが発生するようです.

[nanasess]

/entry/LC_Page_Entry_Kiyaku.php で, Net_UserAgent_Mobile::isMobile() が使用されています

[kimoto]

nanasess への返信

/entry/LC_Page_Entry_Kiyaku.php で, Net_UserAgent_Mobile::isMobile() が使用されています

ありがとうございます 修正しました

[kimoto]

LC_Page_Entry_EmailMobile::lfRegistEmailMobile() で SC_Customer を参照しているため, エラーが発生するようです.

凡ミスですみません。修正しました