Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I have a function that takes in a set of parameters, then applies to them as conditions to an SQL query. However, while I favored a single argument array containing the conditions themselves:

function searchQuery($params = array()) {
    foreach($params as $param => $value) {
        switch ($param) {
            case 'name':
                $query->where('name', $value);
                break;
            case 'phone':
                $query->join('phone');
                $query->where('phone', $value);
                break;
        }
    }
}

My colleague preferred listing all the arguments explicitly instead:

function searchQuery($name = '', $phone = '') {
    if ($name) {
        $query->where('name', $value);
    }

    if ($phone) {
        $query->join('phone');
        $query->where('phone', $value);
    }
}

His argument was that by listing the arguments explicitly, the behavior of the function becomes more apparent - as opposed to having to delve into the code to find out what the mysterious argument $param was.

My problem was that this gets very verbose when dealing with a lot of arguments, like 10+. Is there any preferred practice? My worst-case scenario would be seeing something like the following:

searchQuery('', '', '', '', '', '', '', '', '', '', '', '', 'search_query')

share|improve this question
1  
If the function expects specific keys as parameters, at least those keys should be documented in a DocBlock - that way IDEs can show the relevant information without having to delve into the code. en.wikipedia.org/wiki/PHPDoc –  Ilari Kajaste Oct 1 '13 at 6:29
2  
Performance tip: The foreach is unnecessary in this instance, you could use if(!empty($params['name'])) instead of foreach and switch. –  chiborg Oct 1 '13 at 7:19
1  
You now have one method which you use. I would suggest to take a look here: book.cakephp.org/2.0/en/models/… for creating more methods. They can even be magically generated for standard finds and developed in custom for specific searches. In general that makes a clear api for the users of the model. –  Luc Franken Oct 1 '13 at 8:54
    
this example seems to call for a builder patterns really –  ratchet freak Oct 1 '13 at 9:26
1  
related: Passing compound object for parameters –  gnat Oct 1 '13 at 14:15
add comment

5 Answers

up vote 18 down vote accepted

IMHO your colleague is correct for the above example. Your preference might be terse, but its also less readable and therefore less maintainable. Ask the question why bother writing the function in the first place, what does your function 'bring to the table'- I have to understand what it does and how it does it, in great detail, just to use it. With his example, even though I am not a PHP programmer, I can see enough detail in the function declaration that I do not have to concern myself with its implementation.

As far as a larger number of arguments, that is normally considered a code smell. Typically the function is trying to do too much? If you do find a real need for a large number of arguments, it is likely they are related in some way and belong together in one or a few structures or classes (maybe even array of related items such as lines in an address). However, passing an unstructured array does nothing to address the code smells.

share|improve this answer
    
As for the need of a large number of arguments, the function is essentially taking zero or more arguments, and then limiting the result set by those arguments. The arguments themselves don't have much to do with each other (as distinct SQL clauses), and may not even have the same structure (one can be a simple WHERE, but another would require several JOINs in addition to the WHERE). Would it still be considered a code smell in this specific case? –  xiankai Oct 1 '13 at 7:11
1  
@xiankai In that example I would maybe make one array param for where arguments, one for join specifiers etc. By naming them appropriately that would still be self-documenting. –  Jan Doggen Oct 1 '13 at 8:05
add comment

My answer is more or less language agnostic.

If the only purpose of grouping arguments in a complex data structure (table, record, dictionary, object...) is to pass them as a whole to a function, better avoid it. This adds a useless layer of complexity and makes your intention obscure.

If the grouped arguments have a meaning by themselves, then that layer of complexity helps understand the whole design: name it layer of abstraction instead.

You may find that instead of a dozen individual arguments or one big array, the best design is with two or three arguments each grouping correlated data.

share|improve this answer
add comment

In your case, I would prefer your colleague's method. If you were writing models and I was using your models to develop over them. I see the signature of your colleague's method and can use it immediately.

While, I would have to go through the implementation of your searchQuery function to see what parameters are expected by your function.

I would prefer your approach only in the case when the searchQuery is limited to searching only within a single table, so there will be no joins. In that case my function would look like this:

function searchQuery($params = array()) {
    foreach($params as $param => $value) {
        $query->where($param, $value);
    }
} 

So, I immediately know that the elements of array are actually the column names of a particular table which the class having this method represents in your code.

share|improve this answer
add comment

Also you could pass a string resembling a query string, and use parse_str (because it seems you are using PHP, but other solutions are probably available in other languages) to process it into an array inside the method:

/**
 * Executes a search in the DB with the constraints specified in the $queryString
 * @var $queryString string The search parameters in a query string format (ie
 *      "foo=abc&bar=hello"
 * @return ResultSet the result set of performing the query
 */
function searchQuery($queryString) {
  $params = parse_str($queryString);
  if (isset($params['name'])) {
    $query->where('name', $params['name']);
  }
  if (isset($params['phone'])) {
    $query->join('phone');
    $query->where('phone', $params['phone']);
  }
  ...

  return ...;
}

and call it like

$result = searchQuery('name=foo&phone=555-123-456');

You can use http_build_query to convert from an associative array to a string (the reverse that parse_str does).

share|improve this answer
add comment

Do both, sort of. array_merge allows for an explicit list at the top of the function, as your colleague likes, while keeping the parameters from getting unwieldy, as you prefer.

I also strongly suggest using @chiborg's suggestion from the question comments - it's a lot clearer what you intend.

function searchQuery($params = array()) {
    $defaults = array(
        'name' => '',
        'phone' => '',
        ....
    );
    $params = array_merge($defaults, $params);

    if(!empty($params['name'])) {
        $query->where('name', $params['name']);
    }
    if (!empty($params['phone'])) {
        $query->join('phone');
        $query->where('phone', $params['phone']);
    }
    ....
}
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.