Attaching and Detaching

Not for 'how-to' coding questions but PHP theory instead, this forum is here for those of us who wish to learn about design aspects of programming with PHP.

Moderator: General Moderators

Post Reply
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Attaching and Detaching

Post 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?
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post 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.
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post 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.
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

You could use getters to fetch the various bits of data needed. ;)
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post 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?
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post 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;
    }
}
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Post 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))
(#10850)
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post 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.
Post Reply