I didn’t like the code I wrote for a particular feature. It was “notify users when someone comments in a discussion” with these requirements:
  • Notify discussion author, if not the comment creator
  • Notify users assigned to the parent model that is associated with the discussion, except:
    • the discussion author (because they have already been notified before)
    • the comment creator (because it doesn’t make sense to notify them)
  • Notify users that have left comments in the discussion, except:
    • the discussion author (again, because they have already been notified by step 1)
    • the comment creator (because it doesn’t make sense to notify them)
    • the users that are assigned to the parent model associated with the discussion

Thinking in Conditionals

I started implementing it the way I write the specs above. When I finished it, it felt a hacky and something I wasn’t really happy with, see the pseudo code I ended up with: 
$notifications = [];
$notifiedUsers = [];

if (!$comment->author->is($comment->parent->creator)) {
   $notifications[] = $this->buildForTarget($comment->authorId);
   $notifiedUsers[] = $comment->authorId;
}

foreach ($comment->assignedUsers as $assignedUser) {
    if (!$comment->isAuthor($assignedUser) 
        && !in_array($assignedUser->id, $notifiedUsers
    ) {
        $notifications[] = $this->buildForTarget($assignedUser->id);
        $notifiedUsers[] = $assignedUser->id;
    }
}

foreach ($comment->parent->commenters() as $commenter) {
    if (!$comment->isAuthor($commenter) 
        && !in_array($commenter->id, $notifiedUsers
    ) {
        $notifications[] = $this->buildForTarget($commenter->id);
        $notifiedUsers[] = $commenter->id;
    }
}

return $notifications;
There are just too many loops and conditionals and some duplication, really hacky, but I think it mirrors lots of code I’ve seen out there. Who haven’t heard the phrase “just add an if statement here and you are good” (luckily for me it’s been a while – thanks madewithlove). I tried to change this code, breaking it into small sub-methods. But then I kept passing the $notifiedUsers array as a reference just to avoid recreating a duplicate notification for the same users. That’s when I decided to take a different approach to this feature. Inspired by Adam Wathan’s “Refactoring to Collections” book/videos, I decided to refactor it using that approach.

Collection Pipeline to the Rescue

I started looking at other ways to accomplish the same task. At first, I was really coding as you read the spec, and I wasn’t happy with the end result. Then after taking a look again at what I was doing, I was asking some questions as if it was the first time I was reading that code. So here we go:
  • What is this actually doing?
    • Generating notifications
  • What are the conditionals guarding against?
    • It’s trying to avoid notifying the user that is creating the comment and also trying not to notify the same user more than once for the same comment
Aha! It clicked to me that I could take it differently. I could start by generating all notifications I wanted, without worrying about the duplicates or the comment author first, which I could easily do like so: 
$notifications = array_merge(
    $this->buildParentCreatorNotification($comment),
    $this->buildNotificationsForAssignedUsers($comment),
    $this->buildNotificationsForCommenters($comment)
);
The methods I’m using are basically doing what the previous code was doing before, but it’s now encapsulated into nice, stateless methods. After doing that, I ended up with something like this: 
> $notifications
// [
//   [ 'targetId' => 1, 'type' => 'comment_was_created'],
//   [ 'targetId' => 2, 'type' => 'comment_was_created'],
//   [ 'targetId' => 1, 'type' => 'comment_was_created'],
//   ...
// ]
Ok, next step is to merge duplicate notifications, because we don’t want to send the same notification for the same target more than once, something I got from this code: 
private function mergeDuplicates($notifications)
{
  return array_values(array_reduce($notifications, function ($newNotificationsArray, $notification) {
    $newNotificationsArray[$notification['targetId']] = $notification;
    return $newNotificationsArray;
  }, []));
}
Ok, that did the trick. Next, we have to make sure the comment creator doesn’t get a notification for their own comments. Now that we have an array of notifications without duplication, I got it working like this: 
private filterOutCommentAuthor($notifications, Comment $comment)
{
  return array_values(array_filter($notifications, function ($notification) use ($comment) {
    return !$comment->isAuthor($notification['targetId']);
  }));
}
Great. Now, let’s assemble everything together: 
$notifications = array_merge(
    $this->buildParentCreatorNotification($comment),
    $this->buildNotificationsForAssignedUsers($comment),
    $this->buildNotificationsForCommenters($comment)
);

return $this->filterOutCommentAuthor(
    $this->mergeDuplicates($notifications),
    $comment
);
Something I really liked and the code was really cleaner, it just felt right to me. I was happy with this. I think I’ll be even happier if the we get the pipe operator, this could become something like this: 
$notifications = array_merge(
    $this->buildParentCreatorNotification($comment),
    $this->buildNotificationsForAssignedUsers($comment),
    $this->buildNotificationsForCommenters($comment)
);

return $notifications
    |> $this->mergeDuplicates($$)
    |> $this->filterOutCommentAuthor($$, $comment);

Conclusion

The only downside I could think of would be if we have lots and lots of notifications or things to look after to generate the notifications. It’s not a problem yet, because the model in question here is time based, so it has a defined existence timeline. A discussion will be created, debated and closed. When we end up having millions of users discussing in a single discussion to the point that it’s consuming too much memory to build the notifications, I will be more than happy to refactor this again. What do you think of this approach? Do you think it’s better or worse than before? Do you know of different/better ways of tackling this? Leave a comment below, I’m really interested in your opinions and experiences. See y’all on the next post.

Other interesting reads

More guides on coding principles and good practices