Don’t change the meaning of parameters

Published on and tagged with cakephp  software engineering

Yesterday, Kjell Bublitz (aka m3nt0r) published the following solution for using a blacklist with CakePHP’s Model::save():

// app/app_model.php
class AppModel extends Model {
    function save($data = null, $validate = true, $fieldList = array(), $blackList = false) {
        if (!empty($fieldList) && $blackList) {
            $fieldList = array_diff(array_keys($this->schema()), $fieldList);
        }
        return parent::save($data, $validate, $fieldList);
    }
}

Well, I’m not that happy with his solution ;-)

Sure, it works. However, it changes the meaning of the third parameter, $fieldList, of Model::save(). By design, it represents a whitelist, i.e. you specify with this parameter, which fields should be saved. With the solution shown above, $fieldList now represents either a whitelist or a blacklist, depending on the value of the new $blackList parameter. And this dependency is not visible in the code. If you see the following statement, and you are used to CakePHP, you automatically assume the third parameter is a whitelist, because that’s the way you learned it…

$this->MyModel->save($this->data, true, array('fieldA', 'fieldB'), true);

Ok, so what’s a better solution?

I think a better solution would be to leave Model::save() “untouched” (i.e. you don’t override it), and to move the blacklist-to-whitelist transformation functionality into its own method as shown below (though I’m not sure whether this should be in the AppModel, it probably belongs to a utility class):

// app/app_model.php
class AppModel extends Model {
    public function transformToWhitelist($blacklist) {
        return array_diff(array_keys($this->schema()), $blacklist);
    }
}

The advantages of such a solution are: a) it is easier to read, b) it is probably easier to test, and c) you don’t mix responsibilities (the responsibility of a save() method is to save data, and not to transform a list into another list).

Whether it makes sense to use a blacklist when saving data is a different question. See the discussion on teknoid’s article.

10 comments baked

  • Jonah

    I don’t like any of these solutions.

    Why not this signature:

    save($data = null, $validate = true, $whiteList = false, $blackList = false)

    You could use one, the other, or both even

  • teknoid

    I absolutely agree that from a programmatic point of view one should not hack away at the designated parameter to make it behave as something else.

    However, this functionality is desired in many cases (still IMO), and ultimately what Jonah proposed is the best approach. Yet, until it is supported by the core we are stuck with a some little hacks one way or another.

  • Brandon P

    Sorry, but issues like this perplex me. Why do people want to change the functionality of core components? There is no way that a framework can cover every use-case scenario out there.

    If a framework doesn’t provide built-in functionality for what you are trying to do, take the time and add it to AppModel or a utility class (making it reusable) that Daniel suggested rather than writing about how core components should be changed to meet your obscure needs.

    Frameworks are general, made up of classes to help you solve common/everyday/redundant tasks. Don’t get surprised or frustrated when something doesn’t work out of the box. And please don’t waste your time trying to get a framework to work for you. Let it do what it’s designed to do. Any specific tasks you want, create your own functionality decoupled from the framework as much as possible so its highly reusable.

  • poLK
    //$this->ModelName->save($data, array('blacklist' => array('fieldA', 'fieldB')))
    
    class BlacklistBehavior extends ModelBehavior {
    	public function beforeSave(&$Model, $options = array()) {
    		debug($options);
    	}
    }
  • poLK

    I forgot to point out that Model::$whitelist is public property.

  • ADmad
    //$this->ModelName->save($data, array('blackList' => array('fieldA', 'fieldB')))
    
    function save($data = null, $validate = true, $fieldList = array()) {
    	if (is_array($validate) && isset($validate['blackList'])) {
    		if(!empty($this->whitelist)) {
    			$validate['fieldList'] = array_diff($this->whitelist, $validate['blackList']);
    		} else {
    			$validate['fieldList'] = array_diff(array_keys($this->schema()), $validate['blackList']);
    		}
    	}
    	return parent::save($data, $validate, $fieldList);
    }
  • Abba Bryant

    Who is going to be first to write a full featured blacklistable behavior?

  • cakebaker

    @all: Thanks for your comments!

    @Jonah: I think the problem of such an approach is that the parameters $whiteList and $blackList are mutually exclusive. You use either a whiteList or a blackList strategy, but not both. And having such a method signature doesn’t communicate this fact…

    @teknoid: Yes, as long as it is not supported by the core, you have to find a workaround.

    @Brandon: Yes, I fully agree with you.

    @poLK: Yes, writing a behavior is a possible workaround.

    @ADmad: Yes, that’s a possible workaround, however your solution still mixes responsibilities.

    @Abba: Not me ;-)

  • ADmad

    @cakebaker I am not mixing anymore than core already does. You can pass an options array as second param. I am just adding one more key to it.

  • cakebaker

    @ADmad: Yes, that’s true. In the end it is probably a matter of preference which approach you use.

Bake a comment




(for code please use <code>...</code> [no escaping necessary])

© daniel hofstetter. Licensed under a Creative Commons License