Amélioration de la qualité du code dans CakePHP

Question

J'utilise CakePHP depuis quelques semaines maintenant et ce fut une expérience formidable. J'ai réussi à mettre en ligne un site étonnamment rapidement et j'ai même ajouté un tas de nouvelles fonctionnalités que j'avais prévues mais que je n'ai jamais eu l'occasion de mettre en œuvre.

Jetez un oeil aux deux contrôleurs suivants, ils permettent à un utilisateur d'ajouter un statut premium à l'un des sites liés à leur compte. Ils ne se sentent pas très «gâtés», pourraient-ils être améliorés de quelque façon que ce soit?

Le contrôleur PremiumSites gère le processus d'inscription et aura éventuellement d'autres éléments tels que l'historique.

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))); 
    } 

} 

contrôleur PaypalNotifications gère l'interaction avec 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

envoie à l'utilisateur de Paypal ainsi que toutes les données nécessaires

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"; 

Answer 1

Leçon 1: Ne pas utiliser les superglobales de PHP

• $_POST = $this->params['form'];
• $_GET = $this->params['url'];
• $_GLOBALS = Configure::write('App.category.variable', 'value');
• $_SESSION (vue) = $session->read(); (helper)
• $_SESSION (contrôleur) = $this->Session->read(); (composant)
• $_SESSION['Auth']['User'] = $this->Auth->user();

Replacements pour $_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'])) { 
    ... 
?> 

leçon 2: Vues a re pour le partage (avec l'utilisateur)

Le code documenté "Compile les informations premium et envoie l'utilisateur à Paypal" n'envoie pas l'utilisateur à PayPal. Êtes-vous rediriger dans la vue?

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

Rediriger à la fin de votre contrôleur et supprimer la vue. :)

Leçon 3: manipulation de données appartient dans la couche modèle

<?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; 
    } 
    ... 
} 
?> 

Leçon 4: Les modèles ne sont pas seulement pour l'accès base de données

Code Déplacer documenté « Active le site prime après paiement "pour le modèle PremiumSite, et l'appeler après paiement:

<?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); 
    } 
    ... 
} 
?> 

Vous appelleriez de contrôleur à l'aide $this->PaypalNotification->PremiumSite->enable(...); mais on ne va pas le faire, donc nous allons mélanger tous ensemble ...

Leçon 5: Datasources sont cool

Résumé vos interactions PayPal IPN dans un datasource qui est utilisé par un modèle.

configuration va dans app/config/database.php

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

traite Datasource avec des demandes de service Web (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 
    } 
    ... 
} 
?> 

Laissez le modèle faire le travail (app/models/paypal_notification.php)

Notifications ne sont enregistrées que si elles sont valides, les sites ne sont activés que si la notification est enregistrée

<?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); 
    } 
    ... 
?> 

Les contrôleurs sont muets. (app/controllers/paypal_notifications_controller.php)

"Êtes-vous un poste? Non? .. alors je n'existe même pas." Maintenant, cette action ne fait que crier: "Je sauvegarde les notifications PayPal postées!"

<?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: Utiliser fourni des bibliothèques au lieu de PHP natif

Se reporter aux leçons précédentes pour des exemples de ce qui suit:

• String au lieu de sprintf
• HttpSocket au lieu de fsock fonctions
• RequestHandler au lieu des contrôles manuels
• am au lieu de array_merge

Ces erreurs peuvent empêcher le codage, réduire quantité de code et/ou d'augmenter la lisibilité.

Answer 2

Eh bien, je voudrais souligner ces deux choses :

1. vous avez un ensemble beaucoup de choses de configuration codées en dur ... utilisez le Configure de gâteau pour faire cela ... comme la variable $cost dans le premier contrôleur, ou $ paypalData ... vous pouvez l'obtenir d'ailleurs si vous voulez (par exemple le flash devrait proviennent de fichiers de langue), mais ne pas mélanger la configuration avec l'implémentation ... cela rendra les classes beaucoup plus lisibles, et la maintenance beaucoup plus facile ...
2. encapsuler tous les trucs de socket dans une nouvelle classe d'aide ... vous pouvez en avoir besoin ailleurs ... et vraiment, ça obscurcit ce qui se passe ... aussi, pensez à déplacer d'autres parties de votre contrôleur de boa ... par exemple, il suffit de glisser une autre classe en dessous, cela fait l'implémentation .. Vous devriez toujours essayer d'avoir des contrôleurs frontaux petits et concis, car il est beaucoup plus facile de comprendre ce qui se passe ... si quelqu'un se soucie des détails de mise en œuvre, il peut regarder dans les corres ponding class ...

c'est ce que je pense est cakeish ...

greetz

back2dos

Answer 3

Sauf pour tout ce qui est noté par deizel (excellent post btw), rappelez-vous l'un des principes de base du gâteau: gros modèles, contrôleurs maigres. Vous pouvez vérifier this example, mais l'idée de base est de mettre tous vos trucs de data-mangling dans vos modèles. Votre contrôleur devrait (surtout) être juste un lien entre vos modèles et vues. Votre PremiumSitesController :: index() est un exemple parfait de quelque chose qui devrait être quelque part dans votre modèle (comme l'a souligné deizel).

Chris Hartjes a également écrit un book about refactoring, vous voudrez peut-être jeter un coup d'oeil si vous voulez vraiment apprendre (ce n'est pas gratuit, mais c'est pas cher si). En outre, Matt Curry a un, avec un nom cool: Super Awesome Advanced CakePHP Tips, et il est entièrement gratuit à télécharger. Les deux font une bonne lecture.

Je voudrais aussi brancher mon propre article sur le gâteau que je crois important pour la qualité du code dans le gâteau: Code formatting and readability. Bien que je comprenne si les gens ne sont pas d'accord .. :-)