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

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.