Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is K.I.S.S, but how about some error and misuse control? I am wishing it were .NET.

<?php

function array_group_by($arr, $key_selector) {
    $result = array();
    foreach($arr as $i){
        $key = $key_selector($i);
        if(!array_key_exists($key, $result)) {
            $result[$key] = array();
        }
        $result[$key][] = $i;
    }
    return $result;
}

 $data = array(
        array(1, "Andy", "PHP"),
        array(1, "Andy", "C#"),
        array(2, "Josh", "C#"),
        array(2, "Josh", "ASP"),
        array(1, "Andy", "SQL"),
        array(3, "Steve", "SQL"),
    );

$grouped = array_group_by($data, function($i){  return $i[0]; });

var_dump($grouped);

?>
share|improve this question
add comment

1 Answer

up vote 3 down vote accepted
  1. Using the [] syntax for adding array items, allows PHP to create the array as necessary. Thus you can scrap the if statement for a modest performance increase off 10 to 15% in my own testing. (Unlike if you used array_push() which would throw a warning instead.
  2. The unused $group variable appears to be a mistake, as the function appears to fulfill it's purpose without it. Code reduced to:

    function array_group_by($arr, $key_selector) {
      $result = array();
      foreach ($arr as $i) {
        $key = $key_selector($i);
        $result[$key][] = $i;
      }
      return $result;
    }
    
  3. At this point, the $key could also be left out, but it's almost free in terms of performance, and increases readability, so I left it there.
  4. I'm not sure how much misuse and error handling you need, the function is pretty simple. I would stick with type-hinting the arguments. array is available since PHP 5.1, callable since 5.4. The signature would become

    function array_group_by(array $arr, callable $key_selector);
    

    Then PHP will at run-time throw a fatal error at anyone trying to call the function with incorrect parameters. Generally I think that's good, but in this particular case, it introduces a problem. array('class_A', 'method_B'), is a valid callable, that won't work with with the fast $function($arg);. For any valid callable to get executed, you need to use call_user_func() instead, which is a bit slower. In my testing however, the removal of the if has bigger impact than the use of call_user_func(), so if this isn't performance critical, stick with callable and call_user_func().

Final code:

function array_group_by(array $arr, callable $key_selector) {
  $result = array();
  foreach ($arr as $i) {
    $key = call_user_func($key_selector, $i);
    $result[$key][] = $i;
  }  
  return $result;
}
share|improve this answer
    
Type hinting... Yeah I am rusty on PHP. This is a great answer. I'll give others a chance to bite before marking it as the answer. –  AndyClaw Mar 14 '13 at 19:06
    
Glad I could help. :) –  Letharion Mar 14 '13 at 19:07
    
How about an array walk instead of a foreach? Will that just make it less readable? –  AndyClaw Mar 14 '13 at 19:09
    
I pondered various native array functions for a moment, but I think most (all?) of them want to act directly on the individual elements of the array, which makes them unsuitable to use when you want out data with a different structure than the in data. I could be wrong on this though. –  Letharion Mar 14 '13 at 19:14
    
I changed "callable" to "closure" –  AndyClaw Mar 14 '13 at 19:57
show 2 more comments

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.