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:
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:
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.
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:
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:
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:
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:
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:
Great. Now, let’s assemble everything together:
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:
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.