3
\$\begingroup\$

With functional programming all the rage, I feel I should be using PHP's array_* functions more. But when I compare these two identical implementations, I can't help but think foreach is more readable. Am I just being a dinosaur?

(The foreach implementation takes 1/4 the time, but performance is not of interest for me here, now).

Which do you like better and why?

// Create $attendeeNames from $allPeople who are in the $selectedPeople array
foreach ($allPeople as $person) {
    $id = $person['id'];
    $name = $person['name'];
    if (in_array($id, $selectedPeople)) {
        $attendeeNames[] = $name;
    }
}
// Same, but with array functions
$attendees = array_filter($allPeople, 
    function($item) use ($selectedPeople) {
        $id = $item['id'];
        return in_array($id, $selectedPeople);
    });

$attendeeNames = array_map(
    function($attendee) {
        return $attendee['name'];
    }, $attendees);
\$\endgroup\$
  • \$\begingroup\$ Generally, I agree with Sith Lord Common (stick with the loop), but your coding style should also be consistent. In other words, if you're actually writing a functional structure use the functions. In any other case use foreach. \$\endgroup\$ – I wrestled a bear once. Aug 3 '17 at 12:16
2
\$\begingroup\$

If your functional code is a filter and then a map it's likely a good candidate for array_reduce.

One of the main advantages of a function first approach here is that you can name the inner operation (the filtering operation), but using a closure (or a foreach) robs of you of a name (which could have been used to signal intent) and often robs of us of reuse so wrap the Closure in a named, reusable context (another named function).

A named function will also allow us to pre-inject the variable $selectedPeople into the Closure before array_ functions interact with the Closure inside.

See: http://www.phptherightway.com/pages/Functional-Programming.html for more information on this.

Now we can type hint all the inputs.

function selected_attendee_names(array $selectedPeople) : Closure {
  return function (array $attendeeNames, array $attendee) use ($selectedPeople) : array {
    $isSelectedAttendee = in_array($attendee['id'], $selectedPeople);
    $isSelectedAttendee ? $attendeeNames[] = $attendee['name'] : NULL;
    return $attendeeNames;
  };
}

$attendeeNames = array_reduce($allPeope, selected_attendee_names($selectedPeople), []);

Another approach that could have increased reuse is to array_filter and then use array_column.

function is_selected_attendee(array $selectedPeople) : Closure {
  return function (array $attendee) use ($selectedPeople) : bool {
      $isSelectedAttendee = in_array($attendee['id'], $selectedPeople);
      return $isSelectedAttendee;
  };
}

$selected_attendees = array_filter($allPeople, is_selected_attendee($selectedPeople));
$selected_attendees_names = array_column($selected_attendees, 'name');

Generally speaking I'd prefer the bottom approach because you'll usually find you want to do something else with the selected attendees later in your program.

\$\endgroup\$
  • \$\begingroup\$ There a lot of comments in this thread and a lot of people in the PHP community in general that would advocate a loop here, the downsides of a loop are that you cannot reuse what is inside the loop (without calling a function), none of the inputs of the loop are listed (or type hinted), and the looping operation has no name which could signal intent to the developer which would help other developers read what the code does, I suggest being super weary of anything that is not a function that contains a code block (which is anything between braces i.e. if() {} foreach () {} while () {} etc) \$\endgroup\$ – arcanine Aug 4 '17 at 9:43
  • \$\begingroup\$ You can always put a loop into function \$\endgroup\$ – Your Common Sense Aug 4 '17 at 9:51
  • \$\begingroup\$ Honestly how many times have you seen a developer create a function for one loop? often they will add much more than just a single loop, which will then break reuse, if your function is the operation inside an iteration of a loop you get to type hint everything coming out on every iteration and you telegraph to the reader what variables are going to be used, array_map, array_filter, array_reduce, array_walk cover every scenario for looping, and then telegraph what's going to happen in the loop before you even read the function they call \$\endgroup\$ – arcanine Aug 4 '17 at 10:15
  • \$\begingroup\$ Every single good programmer who is following good practices would move this loop into a method \$\endgroup\$ – Your Common Sense Aug 4 '17 at 10:21
  • \$\begingroup\$ There's no argument from me that any 'good' programmer would already be working inside the context of a method or a function to run any code (that was an assumption I'd already made), what I don't see in the wild is programmers wrapping every foreach loop in a separate function, foreach loops let developers get away with leaving things unnamed, untyped hinted, unclear (where did my variable come from?) and gives no hints away from what is going to happen in the loop \$\endgroup\$ – arcanine Aug 4 '17 at 10:37
2
\$\begingroup\$

Separation of concerns and single responsibility are the keys here.

Your for loop does two things, but because you know what they are and how they are done it seems (now) easier to read.

When you or (more importantly) someone else will come to the same code later, you will be asking yourself first: "What is going on here?" and only after that: "How is it done?".

And then the array_filter, array_map implementations will be much easier to comprehend. Each chunk of code does exactly one thing. So if you want to know how people are filtered, you look inside the array_filter and if you want to know how the extracting is done - look inside array_map.

You can extract those chunks to separate well-named methods and your code will look even better (Read Clean Code by Uncle Bob). And if your filtering or extracting strategies change you can modify them separately without worrying about breaking the other.

\$\endgroup\$
1
\$\begingroup\$

I am in the same boat (or you can call me a dinosaur too). The foreach loop is almost as readadable as natural English, whereas lambda makes you always to stumble. So I would stick to foreach.

As of the code review, I would initialize the resulting array first and also use the O(1) search instead of O(n) as array keys can be found real fast without looping over all values.

$selectedPeopleKeyed = array_flip($selectedPeople);
$attendeeNames = [];
foreach ($allPeople as $person) {
    $id = $person['id'];
    $name = $person['name'];
    if (array_key_exists($id, $selectedPeopleKeyed)) {
        $attendeeNames[] = $name;
    }
}
\$\endgroup\$
  • \$\begingroup\$ You're arguing for array_key_exists instead of in_array for performance reasons. As stated, I'm not interested in performance. To me 'in_array' is much more expressive than mentioning keys. \$\endgroup\$ – Greg Bell Aug 5 '17 at 2:12
  • \$\begingroup\$ array_flip() is still O(n) operation so you don't get O(1) lookup within the loop for free. This does however deliver overall O(m + n) vs. O(m*n) when using array (where m and n are size of the two arrays involved). So even though @GregBell mentions not caring about performance, this is something that should be looked at more closely if you expect to work with large data sets, as linear complexity is much better than exponential. I would think that revisiting if arrays are appropriate data structures might be in order as well. \$\endgroup\$ – Mike Brant Aug 10 '17 at 14:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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