Page 1 of 1

Attaching and Detaching

Posted: Wed Jan 03, 2007 2:53 pm
by Ollie Saunders
I've got this object you can attach callbacks to. Have a read of the docblocks:

Code: Select all

class Osis_Form_Validation_Callback extends Osis_Form_Validation_Abstract
{
    private $_events = array();
    /**
     * Attach a procedural event handler
     *
     * @param string $method event to attach callback to
     * @param callback $callback
     * @param array $params parameters to call the callback with
     * @return array to be used to detach this event later
     */
    public function attach($method, $callback, array $params = array())
    {
        $this->_events[$method][] = array($callback, $params);
        end($this->_events[$method]);
        return array($method, key($this->_events[$method]));
    }
    /**
     * Detach a procedural event handler
     *
     * @param array $index
     * @return bool whether a handler was detached
     */
    public function detach(array $index)
    {
        if ($result = isset($this->_events[$index[0]][$index[1]])) {
            unset($this->_events[$index[0]][$index[1]]);
        }
        return $result;
    }
I'm worried the return from attach() is a bit of a design smell. Do you guys think it is? Is it worth having a separate object such as EventHandlerIndex?

Posted: Wed Jan 03, 2007 3:25 pm
by feyd
I think the usage of the array construct for the detach call does seem a bit smelly. An object to replace it would seem appropriate, however possibly memory and slightly time consuming during mass looping. But it can be used as an abstraction.

Posted: Wed Jan 03, 2007 3:38 pm
by Ollie Saunders
OK this is what I've done. Seem good?

Code: Select all

<?php
class Osis_Form_Validation_CallbackIndex
{
    private $_method, $_position;
    public function __construct($method, $position)
    {
        $this->_method = (string)$method;
        $this->_position = (int)$position;
    }
    public function unsetFrom(array &$queue)
    {
        if ($result = isset($queue[$this->_method][$this->_position])) {
            unset($queue[$this->_method][$this->_position]);
        }
        return $result;
    }
}

Code: Select all

class Osis_Form_Validation_Callback extends Osis_Form_Validation_Abstract
{
    private $_events = array();
    /**
     * Attach a procedural event handler
     *
     * @param string $method event to attach callback to
     * @param callback $callback
     * @param array $params parameters to call the callback with
     * @return array to be used to detach this event later
     */
    public function attach($method, $callback, array $params = array())
    {
        $this->_events[$method][] = array($callback, $params);
        end($this->_events[$method]);
        return new Osis_Form_Validation_CallbackIndex($method, key($this->_events[$method]));
    }
    /**
     * Detach a procedural event handler
     *
     * @param array $index
     * @return bool whether a handler was detached
     */
    public function detach(Osis_Form_Validation_CallbackIndex $index)
    {
        return $index->unsetFrom($this->_events);
    }
That does mean I've basically moved the detaching behaviour out from detach and into unsetFrom. I'm not sure whether that such a good idea. And also I'm making this very specific and concrete.

Posted: Wed Jan 03, 2007 3:41 pm
by feyd
You could use getters to fetch the various bits of data needed. ;)

Posted: Wed Jan 03, 2007 4:02 pm
by Ollie Saunders
I did initially but then it was just a data class and the code looks pretty ugly. You think that would be better?

Posted: Wed Jan 03, 2007 4:11 pm
by Ollie Saunders
OK you're right. Here's final version :P
I got rid of the if here decided it was pretty pointless.

Code: Select all

public function detach(Osis_Form_Validation_CallbackIndex $index)
    {
        $method = $index->getMethod();
        $position = $index->getPosition();
        $existed = isset($this->_events[$method][$position]);
        unset($this->_events[$method][$position]);
        return $existed;
    }

Code: Select all

<?php
class Osis_Form_Validation_CallbackIndex
{
    private $_method, $_position;
    public function __construct($method, $position)
    {
        $this->_method = (string)$method;
        $this->_position = (int)$position;
    }
    public function getPosition()
    {
        return $this->_position;
    }
    public function getMethod()
    {
        return $this->_method;
    }
}

Posted: Thu Jan 04, 2007 2:43 am
by Christopher
Wouldn't it be better to use a Component style:

Code: Select all

attach(new Osis_Form_Validation_Callback('myname', $method, $callback, $params))

Posted: Thu Jan 04, 2007 6:00 am
by Ollie Saunders
Oh my god I am completely unable to make a decision here. I must have changed this about five times now.

Things I like about component style:
  • Three types of data that relate to each other are stored in an object
  • Same object used to attach is also used to detach
  • Ids can be used to overwrite existing attachments
Thing I dislike about component style:
  • Have to use a long class name to attach an event
  • Have to either to an array search to find an event for detaching or the user has to supply an id has you have. A lot of the time that's extra, unwanted, effort
  • My old method allowed me to do this

    Code: Select all

    $this->_events[$method][] = array($callback, $params);
    Which allowed me to iterate over callbacks for a type of event, which I feel is also quite important. You could do that with component style but that won't really work with ids because ids would be unique only to $method (type of event) so that throws overwriting completely out.
Wait. I can feel a decision coming! I'll do both!
OK good. Thanks arborint.