Improving quality of code in CakePHP [closed]

Question

I have been using CakePHP for a few weeks now and its been an great experience. I've managed to port a site surprisingly quickly and I've even added a bunch of new features which I had planned but never got around to implementing.

Take a look at the following two controllers, they allow a user to add premium status to one of the sites linked to their account. They don't feel very 'cakey', could they be improved in any way?

The PremiumSites controller handles the signup process and will eventually have other related things such as history.

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 controller handles the interaction with 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

Sends the user to Paypal along with all the necessary data

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

Lesson 1: Don't use PHP's superglobals

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

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

Lesson 2: Views are for sharing (with the user)

Code documented "Compiles premium info and send the user to Paypal" doesn't send the user to PayPal. Are you redirecting in the view?

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

Redirect at the end of your controller and delete the view. :)

Lesson 3: Data manipulation belongs in model layer

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

Lesson 4: Models aren't just for database access

Move code documented "Enables premium site after payment" to PremiumSite model, and call it after payment:

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

You would call from controller using $this->PaypalNotification->PremiumSite->enable(...); but we aren't going to do that, so let's mix it all together...

Lesson 5: Datasources are cool

Abstract your PayPal IPN interactions into a datasource which is used by a model.

Configuration goes in app/config/database.php

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

Datasource deals with web service requests (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
    }
    ...
}
?>

Let the model do the work (app/models/paypal_notification.php)

Notifications are only saved if they are valid, sites are only enabled if the notification is saved

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

Controllers are dumb. (app/controllers/paypal_notifications_controller.php)

"Are you a post? No? .. then I don't even exist." Now this action just shouts, "I save posted PayPal notifications!"

<?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: Use provided libraries instead of native PHP

Refer to previous lessons for examples of the following:

• String instead of sprintf
• HttpSocket instead of fsock functions
• RequestHandler instead of manual checks
• am instead of array_merge

These can prevent coding errors, reduce amount of code and/or increase readability.

Answer 2

Except for all the stuff noted by deizel (great post btw), remember one of the basic cake principles: fat models, skinny controllers. You can check this example, but the basic idea is to put all your data-mangling stuff in your models. Your controller should (mostly) be just a link between your models and views. Your PremiumSitesController::index() is a perfect example of something that should be somewhere in your model (as pointed out by deizel).

Chris Hartjes has also written a book about refactoring, you might want to take a look at it if you really want to learn (it's not free, but it's cheap though). Also, Matt Curry has one, with a cool name: Super Awesome Advanced CakePHP Tips, and it's completely free for download. Both make for a good read.

I'd also like to plug my own article about cake which I like to believe is important for code quality in cake: Code formatting and readability. Though I understand if people disagree.. :-)