Mejorando la calidad del código en CakePHP

Question

He estado usando CakePHP desde hace unas semanas y ha sido una gran experiencia. Logré portar un sitio sorprendentemente rápido y hasta agregué un montón de características nuevas que había planeado pero que nunca llegué a implementar.

Eche un vistazo a los siguientes dos controladores, que permiten a un usuario agregar estado premium a uno de los sitios vinculados a su cuenta. No se sienten muy cachos, ¿podrían mejorarse de alguna manera?

El controlador PremiumSites maneja el proceso de registro y eventualmente tendrá otras cosas relacionadas, como el historial.

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

} 

controlador PaypalNotifications maneja la interacción con 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

envía al usuario a Paypal, junto con todos los datos necesarios

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

Lección 1: No utilice superglobales de PHP

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

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

lección 2: Vistas a re para compartir (con el usuario)

El código documentado "Compila información premium y envía al usuario a Paypal" no envía al usuario a PayPal. ¿Estás redirigiendo en la vista?

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

Redirigir al final de su controlador y eliminar la vista. :)

Lección 3: Manipulación de datos pertenece en la capa del modelo

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

Lección 4: Los modelos no son sólo para el acceso a la base de datos

mover el código documentado "Activa sitio especial después del pago "al modelo PremiumSite y llámelo después del pago:

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

Se podría llamar desde el controlador utilizando $this->PaypalNotification->PremiumSite->enable(...); pero no vamos a hacer eso, así que vamos a mezclar todo junto ...

Lección 5: fuentes de datos son frescas

interacciones Extracto su PayPal IPN en una datasource que es utilizado por un modelo.

configuración va en app/config/database.php

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

ofertas origen de datos con solicitudes de servicios 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 
    } 
    ... 
} 
?> 

dejar que el modelo haga el trabajo (app/models/paypal_notification.php)

Las notificaciones son sólo salvó si son válidos, los sitios solo se habilitan si la notificación se guarda

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

Los controladores son tontos. (app/controllers/paypal_notifications_controller.php)

"¿Eres un post? ¿No? ... entonces ni siquiera existo". Ahora, esta acción solo grita: "¡Guardo las notificaciones de PayPal publicadas!"

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

experiencia Round: Usar siempre las bibliotecas en lugar de PHP nativo

Consulte las lecciones anteriores ejemplos de lo siguiente:

• String en lugar de sprintf
• HttpSocket en lugar de fsock funciones
• RequestHandler en lugar de comprobaciones manuales
• am en lugar de array_merge

Estos pueden evitar errores de codificación, reducir la cantidad de código y/o aumentar la legibilidad.

Answer 2

así que me gustaría señalar que estas dos cosas :

1. tiene un todo muchas cosas de configuración codificadas ... use cake Configure para hacer eso ... como la variable $cost en el primer controlador, o $ paypalData ... puede obtenerlo de otro lado si lo desea (por ejemplo, el flash debería proceden de archivos de idiomas), pero no mezclan la configuración con la implementación ... esto hará que las clases sean mucho más legibles y el mantenimiento mucho más fácil ...
2. encapsula todo el zócalo en una nueva clase de ayuda ... puede que lo necesites en otro lugar ... y realmente, ofusca lo que sucede ... también, considera mudar otras partes de ese controlador boa tuyo ... por ejemplo, simplemente coloca alguna otra clase debajo de él, eso hace la implementación ... . Siempre debes tratar de tener controladores frontales pequeños y concisos, porque hace que sea mucho más fácil entender lo que sucede ... si alguien se preocupa por los detalles de la implementación, puede mirar a los compañeros. clase de estancamiento ...

eso es lo que creo que es pastel ...

greetz

back2dos

Answer 3

A excepción de todas las cosas señaladas por deizel (gran publicación por cierto), recuerde uno de los principios básicos de la torta: modelos de grasa, controladores delgados. Puede marcar this example, pero la idea básica es poner todo su material de manipulación de datos en sus modelos. Su controlador debería (en su mayoría) ser solo un enlace entre sus modelos y vistas. Su PremiumSitesController :: index() es un ejemplo perfecto de algo que debería estar en algún lugar de su modelo (como señala deizel).

Chris Hartjes también ha escrito book about refactoring, es posible que desee echarle un vistazo si realmente quiere aprender (no es gratis, pero es barato). Además, Matt Curry tiene uno, con un nombre genial: Super Awesome Advanced CakePHP Tips, y es completamente gratis para descargar. Ambos hacen una buena lectura.

También me gustaría conectar mi propio artículo sobre pastel que me gusta creer es importante para la calidad del código en el pastel: Code formatting and readability. Aunque entiendo si la gente no está de acuerdo ... :-)