Improving quality of code in CakePHP

Answer 1

hi,

well i'd point out these two things:

1. you have a whole lot of hard-coded configuration stuff ... use cake's Configure to do that ... like the $cost variable in the first controller, or the $paypalData ... you can get it from elsewhere if you want (for example the flash should come from language files), but don't mix configuration with implementation ... this will make the classes much more readable, and maintenance a lot easier ...
2. encapsulate all the socket stuff into a new helper class ... you may need it elswhere ... and really, it obfuscates what happens ... also, consider moving out other parts of that boa controller of yours ... for example just tuck some other class under it, that does the implementation ... you should always try to have small and concise front controllers, because it makes much easier to understand what happens ... if anyone cares for implementation details, he can look into the corresponding class ...

that's what i think is cakeish ...

Answer 2

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 3

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.. :-)