Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have the following code that allows easy creation, pulling, and deletion of Tickets in a CakePHP application.

App::uses('Component', 'Controller');
App::uses('Ticket', 'Model');
App::import(array('Security'));

class TicketComponent extends Component
{
    public $name = 'Ticket';

    // Create a new ticket by providing the data to be stored in the ticket. 
    function set($info = null) 
    { 
        $this->garbage(); 
        if ($info) 
        { 
            $ticketObj = new Ticket(); 
            $data['Ticket']['hash'] = md5(time()); 
            $data['Ticket']['data'] = $info; 

            if ($ticketObj->save($data)) 
            { 
                return $data['Ticket']['hash']; 
            } 
        } 
        return false; 
    } 

    // Return the value stored or false if the ticket can not be found. 
    function get($ticket = null) 
    { 
        $this->garbage(); 
        if ($ticket) 
        { 
            $ticketObj = new Ticket(); 
            $data = $ticketObj->findByHash($ticket); 
            if (is_array($data) && is_array($data['Ticket'])) 
            { 
                // optionally auto-delete the ticket -> this->del($ticket); 
                return $data['Ticket']['data']; 
            } 
        } 
        return false; 
    } 

    // Delete a used ticket 
    function del($ticket = null) 
    { 
        $this->garbage(); 
        if ($ticket) 
        { 
            $ticketObj = new Ticket(); 
            $data = $ticketObj->findByHash($ticket); 
            if ( is_array($data) && is_array($data['Ticket']) ) 
            { 
                return $data = $ticketObj->delete($data['Ticket']['id']); 
            } 
        } 
        return false; 
    } 

    // Remove old tickets 
    function garbage() 
    {         
        $deadline = date('Y-m-d H:i:s', time() - (24 * 60 * 60)); // keep tickets for 24h. 
        $ticketObj = new Ticket(); 
        $data = $ticketObj->query('DELETE from tickets WHERE created < \''.$deadline.'\''); 
    } 
}

It feels a little old now (not sure how old the code is) but wondered if certain things could be improved and if there was any parts that were considered 'bad code'.

Some observations of my own:

  • Do I need to keep creating the $ticketObj? And is using new Ticket() correct for CakePHP conventions?
  • The garbage function runs when anything ticket related is called, and makes sure that expired tickets cannot be used... But requires that it be called at the beginning of all other functions. Could this be better? Not necessarily a CRON job, but more code efficient.
share|improve this question

1 Answer 1

In order to call each time the garbage function just create a

public function beforeFilter() {
        parent::beforeFilter();
        $this->garbage();

About the new Ticket() i think it would be okay to change the

$ticketObj->..

to

$this->Ticket->..

Also i think setting your function name's as set is a bad idea because they are built in functions in cakephp

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.