CODEWITHSUNDEEP
phpcakephp
eliagentili
eliagentili

Reputation: 619

Refactoring a model function on CakePHP

What do you thinking about this code? It's better to write only one method getByPoint() and pass it a further string parameter 'open', 'close'?

Otherwise the wrapper functions getOpenedByPoint() and getClosedByPoint() are justified?

Thank You for answering ;)

/**
 * Get all opened verbals, not cancelled, for a point
 * @param type $point_id
 * @return array
 */
    public function getOpenedByPoint($point_id = null)
    {
        $conditions = array('Verbal.cancelled' => 0, 'Verbal.close' => 0);
        return $this->getByPoint($point_id, $conditions);
    }

/**
 * Get all closed verbals, not cancelled, for a point
 * @param type $point_id
 * @return array
 */
    public function getClosedByPoint($point_id = null)
    {
        $conditions = array('Verbal.cancelled' => 0, 'Verbal.close' => 1);
        return $this->getByPoint($point_id, $conditions);
    }

/**
 * Get all verbals for a point
 * @param type $point_id
 * @return array
 */
    public function getByPoint($point_id = null, $conditions = array())
    {
        if($point_id) {
            $conditions = Hash::merge(array('Verbal.point_id' => $point_id), $conditions);
            return $this->find('all', array(
                'contain' => array('Driver','Car'),
                'fields' => array('Verbal.number','Verbal.year','Verbal.data_out','Driver.cognome','Driver.nome','Car.model','Car.plate'),
                'conditions' => $conditions,
                'order' => array('Verbal.data_out' => 'ASC')
            ));
        }
        return array();
    }

Upvotes: 0

Views: 93

Answers (3)

dhofstet
dhofstet

Reputation: 9964

Another idea is to define the different states as parameters like:

public function getByPoint($point_id = null, $open = true, $cancelled = false) {
    ...
}

To improve the readability of the code calling the getByPoint() method you can define class constants with the different states. A method call would then look like:

$this->Verbal->getByPoint(123, Verbal::OPEN, Verbal::NOT_CANCELLED);

Upvotes: 0

Guillermo Mansilla
Guillermo Mansilla

Reputation: 3879

If you maintain the current code I would change visibility of getByPoint to private, so that you force the usage of getClosedByPoint() or getOpenedByPoint() from your controllers.

If I were you, I would just use one function.

Upvotes: 0

Ayo Akinyemi
Ayo Akinyemi

Reputation: 777

  • This is much more readable than passing in 'close' or 'open' param.
  • if the $point_id is important to the getByPoint function, why default it to null in all your functions? The smell here is that when you get back an empty array from your getByPoint function, you wont know if there wasn't any result found, or the $point_id being passed in is invalid. So it is a better practice to set a checkpoint, that way, if something breaks, you'd know its an invalid point_id versus no result in the database.

Upvotes: 1

Related Questions