2009-07-03 3 views
13

Я использую CakePHP в течение нескольких недель, и это было отличным опытом. Мне удалось быстро вывести сайт на место, и я даже добавил множество новых функций, которые я планировал, но так и не смог реализовать.Улучшение качества кода в CakePHP

Посмотрите на следующие два контроллера, они позволяют пользователю добавлять статус премиум-статуса на один из сайтов, связанных с их учетной записью. Они не чувствуют себя очень «тошнотворными», могут ли они быть улучшены каким-либо образом?

Контроллер PremiumSites обрабатывает процесс регистрации и в конечном итоге будет иметь другие связанные вещи, такие как история.

class PremiumSitesController extends AppController { 

    var $name = 'PremiumSites'; 

    function index() { 
     $cost = 5; 

     //TODO: Add no site check 

     if (!empty($this->data)) { 
      if($this->data['PremiumSite']['type'] == "1") { 
       $length = (int) $this->data['PremiumSite']['length']; 
       $length++; 
       $this->data['PremiumSite']['upfront_weeks'] = $length; 
       $this->data['PremiumSite']['upfront_expiration'] = date('Y-m-d H:i:s', strtotime(sprintf('+%s weeks', $length))); 
       $this->data['PremiumSite']['cost'] = $cost * $length; 
      } else { 
       $this->data['PremiumSite']['cost'] = $cost; 
      } 

      $this->PremiumSite->create(); 
      if ($this->PremiumSite->save($this->data)) { 
       $this->redirect(array('controller' => 'paypal_notifications', 'action' => 'send', $this->PremiumSite->getLastInsertID())); 
      } else { 
       $this->Session->setFlash('Please fix the problems below', true, array('class' => 'error')); 
      } 
     } 

     $this->set('sites',$this->PremiumSite->Site->find('list',array('conditions' => array('User.id' => $this->Auth->user('id'), 'Site.is_deleted' => 0), 'recursive' => 0))); 
    } 

} 

PaypalNotifications Контроллер обрабатывает взаимодействие с Paypal.

class PaypalNotificationsController extends AppController { 

    var $name = 'PaypalNotifications'; 

    function beforeFilter() { 
     parent::beforeFilter(); 
     $this->Auth->allow('process'); 
    } 

    /** 
    * Compiles premium info and send the user to Paypal 
    * 
    * @param integer $premiumID an id from PremiumSite 
    * @return null 
    */ 
    function send($premiumID) { 

     if(empty($premiumID)) { 
      $this->Session->setFlash('There was a problem, please try again.', true, array('class' => 'error')); 
      $this->redirect(array('controller' => 'premium_sites', 'action' => 'index')); 
     } 

     $data = $this->PaypalNotification->PremiumSite->find('first', array('conditions' => array('PremiumSite.id' => $premiumID), 'recursive' => 0)); 

     if($data['PremiumSite']['type'] == '0') { 
      //Subscription 
      $paypalData = array(
       'cmd' => '_xclick-subscriptions', 
       'business'=> '', 
       'notify_url' => '', 
       'return' => '', 
       'cancel_return' => '', 
       'item_name' => '', 
       'item_number' => $premiumID, 
       'currency_code' => 'USD', 
       'no_note' => '1', 
       'no_shipping' => '1', 
       'a3' => $data['PremiumSite']['cost'], 
       'p3' => '1', 
       't3' => 'W', 
       'src' => '1', 
       'sra' => '1' 
      ); 

      if($data['Site']['is_premium_used'] == '0') { 
       //Apply two week trial if unused 
       $trialData = array(
        'a1' => '0', 
        'p1' => '2', 
        't1' => 'W', 
       ); 
       $paypalData = array_merge($paypalData, $trialData); 
      } 
     } else { 
      //Upfront payment 

      $paypalData = array(
       'cmd' => '_xclick', 
       'business'=> '', 
       'notify_url' => '', 
       'return' => '', 
       'cancel_return' => '', 
       'item_name' => '', 
       'item_number' => $premiumID, 
       'currency_code' => 'USD', 
       'no_note' => '1', 
       'no_shipping' => '1', 
       'amount' => $data['PremiumSite']['cost'], 
      ); 
     } 

     $this->layout = null; 
     $this->set('data', $paypalData); 
    } 

    /** 
    * IPN Callback from Paypal. Validates data, inserts it 
    * into the db and triggers __processTransaction() 
    * 
    * @return null 
    */ 
    function process() { 
     //Original code from http://www.studiocanaria.com/articles/paypal_ipn_controller_for_cakephp 
     //Have we been sent an IPN here... 
     if (!empty($_POST)) { 
      //...we have so add 'cmd' 'notify-validate' to a transaction variable 
      $transaction = 'cmd=_notify-validate'; 
      //and add everything paypal has sent to the transaction 
      foreach ($_POST as $key => $value) { 
       $value = urlencode(stripslashes($value)); 
       $transaction .= "&$key=$value"; 
      } 
      //create headers for post back 
      $header = "POST /cgi-bin/webscr HTTP/1.0\r\n"; 
      $header .= "Content-Type: application/x-www-form-urlencoded\r\n"; 
      $header .= "Content-Length: " . strlen($transaction) . "\r\n\r\n"; 
      //If this is a sandbox transaction then 'test_ipn' will be set to '1' 
      if (isset($_POST['test_ipn'])) { 
       $server = 'www.sandbox.paypal.com'; 
      } else { 
       $server = 'www.paypal.com'; 
      } 
      //and post the transaction back for validation 
      $fp = fsockopen('ssl://' . $server, 443, $errno, $errstr, 30); 
      //Check we got a connection and response... 
      if (!$fp) { 
       //...didn't get a response so log error in error logs 
       $this->log('HTTP Error in PaypalNotifications::process while posting back to PayPal: Transaction=' . 
        $transaction); 
      } else { 
       //...got a response, so we'll through the response looking for VERIFIED or INVALID 
       fputs($fp, $header . $transaction); 
       while (!feof($fp)) { 
        $response = fgets($fp, 1024); 
        if (strcmp($response, "VERIFIED") == 0) { 
         //The response is VERIFIED so format the $_POST for processing 
         $notification = array(); 

         //Minor change to use item_id as premium_site_id 
         $notification['PaypalNotification'] = array_merge($_POST, array('premium_site_id' => $_POST['item_number'])); 
         $this->PaypalNotification->save($notification); 

         $this->__processTransaction($this->PaypalNotification->id); 
        } else 
         if (strcmp($response, "INVALID") == 0) { 
          //The response is INVALID so log it for investigation 
          $this->log('Found Invalid:' . $transaction); 
         } 
       } 
       fclose($fp); 
      } 
     } 
     //Redirect 
     $this->redirect('/'); 
    } 

    /** 
    * Enables premium site after payment 
    * 
    * @param integer $id uses id from PaypalNotification 
    * @return null 
    */ 
    function __processTransaction($id) { 
     $transaction = $this->PaypalNotification->find('first', array('conditions' => array('PaypalNotification.id' => $id), 'recursive' => 0)); 
     $txn_type = $transaction['PaypalNotification']['txn_type']; 

     if($txn_type == 'subscr_signup' || $transaction['PaypalNotification']['payment_status'] == 'Completed') { 
      //New subscription or payment 
      $data = array(
       'PremiumSite' => array(
        'id' => $transaction['PremiumSite']['id'], 
        'is_active' => '1', 
        'is_paid' => '1' 
       ), 
       'Site' => array(
        'id' => $transaction['PremiumSite']['site_id'], 
        'is_premium' => '1' 
       ) 
      ); 

      //Mark trial used only on subscriptions 
      if($txn_type == 'subscr_signup') $data['Site']['is_premium_used'] = '1'; 

      $this->PaypalNotification->PremiumSite->saveAll($data); 

     } elseif($txn_type == 'subscr-cancel' || $txn_type == 'subscr-eot') { 
      //Subscription cancellation or other problem 
      $data = array(
       'PremiumSite' => array(
        'id' => $transaction['PremiumSite']['id'], 
        'is_active' => '0', 
       ), 
       'Site' => array(
        'id' => $transaction['PremiumSite']['site_id'], 
        'is_premium' => '0' 
       ) 
      ); 

      $this->PaypalNotification->PremiumSite->saveAll($data); 
     } 


    } 

    /** 
    * Used for testing 
    * 
    * @return null 
    */ 
    function index() { 
     $this->__processTransaction('3'); 
    } 
} 

/views/paypal_notifications/send.ctp

посылает пользователю Paypal вместе со всеми необходимыми данными

echo "<html>\n"; 
echo "<head><title>Processing Payment...</title></head>\n"; 
echo "<body onLoad=\"document.form.submit();\">\n"; 
echo "<center><h3>Redirecting to paypal, please wait...</h3></center>\n"; 

echo $form->create(null, array('url' => 'https://www.sandbox.paypal.com/cgi-bin/webscr', 'type' => 'post', 'name' => 'form')); 

foreach ($data as $field => $value) { 
    //Using $form->hidden sends in the cake style, data[PremiumSite][whatever] 
    echo "<input type=\"hidden\" name=\"$field\" value=\"$value\">"; 
} 

echo $form->end(); 

echo "</form>\n"; 
echo "</body></html>\n"; 

+0

Исправлено форматирование кода для вас – PatrikAkerstrand

+0

Спасибо, я пробовал это раньше, не слишком уверен, почему он не работает – DanCake

ответ

28

Урок 1: Не используйте суперглобальные РНР

  • $_POST = $this->params['form'];
  • $_GET = $this->params['url'];
  • $_GLOBALS = Configure::write('App.category.variable', 'value');
  • $_SESSION (вид) = $session->read(); (вспомогательные)
  • $_SESSION (контроллер) = $this->Session->read(); (компонент)
  • $_SESSION['Auth']['User'] = $this->Auth->user();

Замены для $_POST:

<?php 
    ... 
    //foreach ($_POST as $key => $value) { 
    foreach ($this->params['form'] as $key => $value) { 
    ... 
    //if (isset($_POST['test_ipn'])) { 
    if (isset($this->params['form']['test_ipn'])) { 
    ... 
?> 

заниматься 2: Просмотров a re для совместного использования (с пользователем)

Код, документированный «Информация о премиях компиляции и отправка пользователю в Paypal», не отправляет пользователя в PayPal. Вы перенаправляетесь в представление?

<?php 
    function redirect($premiumId) { 
     ... 
     $this->redirect($url . '?' . http_build_query($paypalData), 303); 
    } 

Перенаправление в конце вашего контроллера и удаление представления. :)

Урок 3: манипулирование данного принадлежит в модельном слое

<?php 
class PremiumSite extends AppModel { 
    ... 
    function beforeSave() { 
     if ($this->data['PremiumSite']['type'] == "1") { 
      $cost = Configure::read('App.costs.premium'); 
      $numberOfWeeks = ((int) $this->data['PremiumSite']['length']) + 1; 
      $timestring = String::insert('+:number weeks', array(
       'number' => $numberOfWeeks, 
      )); 
      $expiration = date('Y-m-d H:i:s', strtotime($timestring)); 
      $this->data['PremiumSite']['upfront_weeks'] = $weeks; 
      $this->data['PremiumSite']['upfront_expiration'] = $expiration; 
      $this->data['PremiumSite']['cost'] = $cost * $numberOfWeeks; 
     } else { 
      $this->data['PremiumSite']['cost'] = $cost; 
     } 
     return true; 
    } 
    ... 
} 
?> 

Урок 4: Модели не только для доступа к базе данных

Переместить код документированы «Включает премиум сайта после оплаты »на модель PremiumSite и назовите ее после оплаты:

<?php 
class PremiumSite extends AppModel { 
    ... 
    function enable($id) { 
     $transaction = $this->find('first', array(
      'conditions' => array('PaypalNotification.id' => $id), 
      'recursive' => 0, 
     )); 
     $transactionType = $transaction['PaypalNotification']['txn_type']; 

     if ($transactionType == 'subscr_signup' || 
      $transaction['PaypalNotification']['payment_status'] == 'Completed') { 
      //New subscription or payment 
      ... 
     } elseif ($transactionType == 'subscr-cancel' || 
      $transactionType == 'subscr-eot') { 
      //Subscription cancellation or other problem 
      ... 
     } 
     return $this->saveAll($data); 
    } 
    ... 
} 
?> 

Вы назвали бы от контроллера с помощью $this->PaypalNotification->PremiumSite->enable(...);, но мы не будем этого делать, так что давайте смешать все это вместе ...

Урок 5: Источники данных прохладные

Аннотация ваш PayPal IPN взаимодействия в datasource, который используется моделью.

Конфигурация идет в app/config/database.php

<?php 
class DATABASE_CONFIG { 
    ... 
    var $paypal = array(
     'datasource' => 'paypal_ipn', 
     'sandbox' => true, 
     'api_key' => 'w0u1dnty0ul1k3t0kn0w', 
    } 
    ... 
} 
?> 

DATASOURCE сделок с запросами веб-служб (app/models/datasources/paypal_ipn_source.php)

<?php 
class PaypalIpnSource extends DataSource { 
    ... 
    var $endpoint = 'http://www.paypal.com/'; 
    var $Http = null; 
    var $_baseConfig = array(
     'sandbox' => true, 
     'api_key' => null, 
    ); 

    function _construct() { 
     if (!$this->config['api_key']) { 
      trigger_error('No API key specified'); 
     } 
     if ($this->config['sandbox']) { 
      $this->endpoint = 'http://www.sandbox.paypal.com/'; 
     } 
     $this->Http = App::import('Core', 'HttpSocket'); // use HttpSocket utility lib 
    } 

    function validate($data) { 
     ... 
     $reponse = $this->Http->post($this->endpoint, $data); 
     .. 
     return $valid; // boolean 
    } 
    ... 
} 
?> 

Пусть модель сделать работу (app/models/paypal_notification.php)

Уведомления сохраняются только если они действительны, сайты разрешены только в том случае, если уведомление сохранено.

<?php 
class PaypalNotification extends AppModel { 
    ... 
    function beforeSave() { 
     $valid = $this->validate($this->data); 
     if (!$valid) { 
      return false; 
     } 
     //Minor change to use item_id as premium_site_id 
     $this->data['PaypalNotification']['premium_site_id'] = 
      $this->data['PaypalNotification']['item_number']; 
     /* 
     $this->data['PaypalNotification'] = am($this->data, // use shorthand functions 
      array('premium_site_id' => $this->data['item_number'])); 
     */ 
     return true; 
    } 
    ... 
    function afterSave() { 
     return $this->PremiumSite->enable($this->id); 
    } 
    ... 
    function validate($data) { 
     $paypal = ConnectionManager::getDataSource('paypal'); 
     return $paypal->validate($data); 
    } 
    ... 
?> 

Контроллеры являются немыми. (app/controllers/paypal_notifications_controller.php)

«Вы почта? Нет? .. тогда я даже не существует». Теперь это действие просто кричит: «Я сохраняю опубликованные уведомления PayPal!»

<?php 
class PaypalNotificationsController extends AppModel { 
    ... 
    var $components = array('RequestHandler', ...); 
    ... 
    function callback() { 
     if (!$this->RequestHandler->isPost()) { // use RequestHandler component 
      $this->cakeError('error404'); 
     } 
     $processed = $this->PaypalNotification->save($notification); 
     if (!$processed) { 
      $this->cakeError('paypal_error'); 
     } 
    } 
    ... 
} 
?> 

Bonus Round: Использование при условии библиотеки вместо родной PHP

Приведите предыдущие уроки для примеров следующего:

  • String вместо sprintf
  • HttpSocket вместо fsock функции
  • RequestHandler вместо ручных проверок
  • am вместо array_merge

Это может предотвратить ошибки кодирования, уменьшить количество кода и/или повысить читаемость.

+0

Прежде всего, спасибо вам за отличную запись, о которой вы мне много подумали. Lession 1: Раздел process() не был написан мной, я только что внес несколько изменений. Мне показалось странным, что человек использовал $ _POST, а не $ this-> данные, я, вероятно, переписал его. Lession 2: Да, он перенаправляет пользователя в представлении, но не является стандартным перенаправлением. Взгляните на мой оригинальный пост. Урок 3: Я согласен, плюс ваш код намного более изящный, чем я бы это сделал. Lession 4: Я начну использовать модели больше, он определенно сползает вниз по контроллеру и читает проще. – DanCake

+0

Хит ограничение персонажа и потеря форматирования делает его немного трудно читать. Lession 5: PayPal IPN действительно является регулярным api, данные отправляются вместе с пользователем, который может сделать использование DataSource сложным для реализации. Я знаю, что код в представлении довольно плох, я не мог найти другого способа добиться этого. Бонус: Я действительно должен правильно рассмотреть CakePHP api, я экспериментировал с компонентом HttpSocket, но не с RequestHandler. Еще раз спасибо за вашу помощь – DanCake

+0

Благодарим за предоставленный хороший пример кода, мне действительно понравилось думать о том, как можно реорганизовать его. :) В ответ на ваши вопросы: 1. Я обновил это до $ this-> params ['form'], поскольку данные $ this-> применяются только к данным, представленным формами, созданными с помощью FormHelper Cake; 2. Я бы проверил, работает ли GET здесь, поскольку количество данных не является чрезмерным. Если PayPal принимает только POST, я могу предложить только добавить кнопку отправки для тех, у кого нет JavaScript; 3/4. Это было придумано кем-то, как методология «Толстая модель, худой контроллер», если вам нужно вставить ее в несколько слов; – deizel

1

хорошо я бы указать на эти две вещи :

  1. у вас есть целый много жестко закодированных конфигурационных материалов ... используйте cake's Configure, чтобы сделать это ... как переменная $cost в первом контроллере или $ paypalData ... вы можете получить ее из другого места, если хотите (например, вспышка должна из языковых файлов), но не смешивайте конфигурацию с реализацией ... это сделает классы более читабельными и упростит обслуживание ...
  2. инкапсулирует все материалы сокета в новый класс-помощник ... вам может понадобиться это где-то ... и действительно, это запутывает то, что происходит ... также, подумайте о том, чтобы переместить другие части этого контроллера boa вашего ... например, просто подложите под него другой класс, что делает реализацию. вы всегда должны стараться иметь небольшие и лаконичные передние контроллеры, потому что гораздо легче понять, что происходит ... если кто-то заботится о деталях реализации, он может заглянуть в соответствие ponding class ...

вот что я думаю, это торт ...

Greetz

back2dos

+0

Я планирую переместить все, чтобы настроить в конце концов, это было просто hardcoded, пока я тестирую, чтобы убедиться, что все работает по-настоящему. Я, вероятно, должен начать переводить все на языковые файлы, хотя у меня еще не было возможности поиграть с ними. Пока мы на нем, в чем разница между l10n и i18n? – DanCake

+0

10 и 18 представляют количество символов между первой и последней буквой. Они означают L-ocalizatio-n и I-nternationalizatio-n соответственно. Первый из них является фактическим произведением перевода для определенного языка, а последний - действием, позволяющим позже переводить (или локализовать) приложение (т. Е. Используя функцию __() в Cake). – deizel

+0

Отлично! Спасибо за помощь. – DanCake

5

За исключением всех материалов, отмеченных deizel (отличное сообщение от btw), вспомните один из основных принципов пирога: модели жира, тощие контроллеры. Вы можете проверить this example, но основная идея состоит в том, чтобы поместить все ваши данные в ваши модели. Ваш контроллер должен (в основном) быть только ссылкой между вашими моделями и представлениями. Ваш PremiumSitesController :: index() является прекрасным примером того, что должно быть где-то в вашей модели (как указано в deizel).

Chris Hartjes также написал book about refactoring, вы можете взглянуть на него, если вы действительно хотите учиться (это не бесплатно, но это дешево). Кроме того, у Matt Curry есть один, с классным именем: Super Awesome Advanced CakePHP Tips, и он абсолютно бесплатный для скачивания. Оба делают хорошее чтение.

Я также хотел бы подключить свою собственную статью о торте, который мне нравится верить, важно для качества кода в торте: Code formatting and readability. Хотя я понимаю, если люди не согласны .. :-)

+0

Вы тот, кто создал NeutrinoCMS? Я обнаружил, что в начале и просмотре кода была большая помощь. Кроме того, я обязательно прочитаю их в течение следующих нескольких дней. – DanCake

+0

Да, это было бы я :) Я рад, что это помогло вам учиться, это, безусловно, помогло мне. Хотя в последнее время свободного времени было мало. –