views:

47

answers:

3

Hi,

I often see this idiom when reading php code:

public function __construct($config)
{
    if (array_key_exists('options', $config)) {
       ...
    }
    if (array_key_exists('driver_options', $config)) {
        ...
    }
}

Here I am concern with the way the parameter is used. If I were in lisp I would do:

(defun ct (&key options driver_options) 
       (do-something-with-option-and-driver_option))

But since I am in PHP I would rather have a constructor that take a list of parameter and let them be null if there a not require.

So what do you guys think about having an array as parameter in other to do some initialization-or-whatever?

In other to answer you have to take in account the point of view of the user of the function and the designer of the API.

Also have you ever heard this has a code-smell?

thanks

EDIT: I would also address you some question/issues concerning array parameter. In particular if you have the same answer than @Brent Baisley and @ircmaxell

  • What if you misspell a key in the array?
  • What if you have some key that don't have any meaning to your function/method?
+1  A: 

Personally, I dislike that idiom. I prefer to have a long parameter list instead, if necessary.

The problem is that I can't know the elements the array can't take by looking at the function signature. On top of that, the implementations almost never check if there's any key that's not recognized, so if I mispell an array key, I get no warning.

A better alternative would be passing a configuration object. At least, there the IDE can provide me hints on the available configuration objects and the calculated default values for missing options can be moved away from the constructor you show to the getters in the configuration object. The obvious alternative is to provide setters for the several configuration options; though this doesn't help for the required ones for each no default can be provided.

Artefacto
+1  A: 

I very much like the design pattern of "options arrays". If PHP supported Python's argument expansion, then I would agree to go with a long parameter list. But I just find foo(1, 2, 'something', true, 23, array(4), $bar); to be REALLY un-readable. I typically will use arrays when there are more than about 3 or 4 parameters that need to be set...

What I would suggest to "clean up" the constructor, is create a protected method for accessing config vars (preferably in a base class):

abstract class Configurable {
    protected $options = array();
    protected $requiredOptions = array();

    public function __construct(array $options = array()) {
        $this->options = $options;
        foreach ($this->requiredOptions as $option) {
            if (!isset($this->options[$option])) {
                throw new InvalidArgumentException('Required argument [$'.$option.'] was not set');
            }
        }
    }

    protected function _getOption($key, $default = null) {
        return isset($this->options[$key]) ? $this->options[$key] : $default;
    }
}

Then, in your class, you can overload the requireOptions array to define things that need to be set

class Foo extends Configurable {
    protected $requiredOptions = array(
        'db',
        'foo',
    );

    public function __construct(array $options = array()) {
        parent::__construct($options);
        if ($this->_getOption('bar', false)) {
            //Do Something
        }
    }
}

One thing. If you do this, PLEASE document the options required. It will make life a lot easier for those that follow you.

ircmaxell
Well, this suffers from basically the same problem – your IDE cannot provide you hints to the valid options (or their type).
Artefacto
I personally hate having an array as a parameter when it could be past as a long parameter list. I understand long parameter list is not a good solution but Martin Fowler did answer this issue in his book "Refactoring Improving the design of existing code" The smell is call "Long Parameter List" and the solution is "Replace Parameter with Method, Introduce Parameter Object, ...". So instead of array I would prefer an object as @Artefacto point it.
mathk
@Artefacto Well, IDEs typically can provide you with the docblock for the method (I've tested it with NetBeans and Eclipse). So as long as you document the options (like I said in my answer), that shouldn't be a problem. Sure, you'll lose the autocomplete, but I think it's worth it since the code is more readable when written (considering `array('something'=>'foo', 'else' => 'bar')` doesn't need any type of "hint" to understand, where `foo(1,3,52,true, 'something');` is really not self-explanatory at all. I care less about writing code, then I do maintaining/debugging it...
ircmaxell
@mathk Actually, when he said introduce parameter object, he meant for grouping related parameters. The example commonly cited is if you need a date range. Instead of `foo($start_date, $end_date);` use `foo(dateRange $dateRange);` where dateRange is a class that defines both. Creating a single object for all parameters for each method would be a little bit overkill IMHO. Plus, you either just moved the problem from one method to another, or increased the lines of code required to call a method by several times...
ircmaxell
@ircmaxell Of course I don't mean to group all the parameter just for the sake of grouping it. I am sure that if you have a lot of parameter you can find some that go a long well in an object. May be even several object.
mathk
@ircmaxell Sure, the docblock is useful in those cases and it surely mitigates the harm, but saying it's more readable is not necessarily true. First, you tipically pass variables, which hopefully will have a self-explanatory name; second, you can use comments when it's not obvious what the parameter is `foo(1 /* explanation */, 2 /* dddd */)`. It's still more succint than using an array.
Artefacto
A: 

I find using arrays as parameters to be helpful when there are a lot of optional parameters. Usually I'll use an array_merge to merge the passed array with the "defaults" array. No checking required. If you have required parameters, you can use array_diff_key to determine if any required parameters are missing.

function params($p_array) {
    static $default_vals = array('p1'=>1, 'p2'=>null, 'p3'=>'xyz');
    static $rqd_params = array('p1'=>null, 'p3'=>null);
    // check for missing required params
    $missing_params = array_diff_key($rqd_params, $p_array);
    if ( count($missing_params)>0 ) {
       //return an error (i.e. missing fields)
       return array_keys($missing_params);
    }
    // Merge passed params and override defaults
    $p_array = array_merge($default_vals, $p_array);
}
Brent Baisley