In general I prefer composition over inheritance. However, recently there were a couple bugs around a certain group of objects in our codebase that pointed out a refactoring that was begging to be made, and in this case inheritance was the right way to go. Today I’m going to go over that refactoring and explain it a bit.
Let’s start with a short explanation of the problem domain! In our app we have a lot of third party data that we handle. The problem is, much of it is wrong! We spend a lot of time cleaning that data, and to help with that we try and run a bunch of algorithms determine if we feel an object exhibits markers that might make it ripe for cleaning. We call this process Flagging, and it is very important ;)
Flagger classes that are given an object to flag (note: that’s the verb this time) and are responsible for calling all of those methods and then passing on the results to a class to handle updating our database. That class looked something like this:
So, it’s a pretty simple class, right? However, this approach led to bugs! First off, whenever we added a new Flag, we had to remember to touch this class to add it to
FLAGGING_METHODS and also to add the instantiaion of that object and injection of dependencies so we can call it. Also, as these classes got larger, there was eventually some issue with confusion around naming in each of the
Flagging method objects, so ideally we wanted to find a way to make that naming consistent, but we’ll solve one problem at a time.
So, let’s start with addressing the Open/Closed violation that we have going on here. To fix the problem of having to touch the
DistrictFlagger class every time we add a new flag, we can use the Registry pattern. Now the Registry pattern is generally good, but it comes with the downside of relying on shared global state. Kind of a bummer, but it’s the only way to make it work, really. So, here’s what one of our flagging classes might look like after we’ve done that:
We’re now relying on the presence of a
::DISTRICT_FLAGS hash to serve as our registry, and each flagging class registers itself in that registry when the code is evaluated.
And then we’d change our
call method to look like this:
So we’ve removed the need for that
FLAGGING_METHODS constant in the
DistrictFlagger class, but we still need to create that method to be called. The problem there, though is, that each class can have different dependencies that we need to inject. Sounds like we’re going to have to use some metaprogramming!
Let’s change our registration code in our
Now in that registry we’re registering a class name, as well as the dependencies that the class needs injected when it’s instantiated. This is getting better! We can go back to our
DistrictFlagger class and do some more modification to that
That’s a little more gnarly, but basically what we’re doing is:
1) take the registered dependecies, which correspond to methods implemented in the
DistrictFlagger class, and map over them to get the values we need to inject to our flagging class
2) instantiate and call that flagging class
3) pass the results on to the
The real great part about this is now we don’t need to define methods in the
DistrictFlagger to instantiate and call our flagging objects!
We’ve made it this far without any inheritance (or composition for that matter), but there’s one other refactoring that I wanted to do here. If we look in our
NoSchools class, we’re referencing the dependencies (in this case,
num_schools in a bunch of places. Let’s see if we can declare those dependencies once and then have a bunch of other stuff happen for us automatically!
I’m going to skip the middle ground and get right to the solution. In the end, our flagging class looks like this:
I think that’s a pretty good API for those classes - clear, concicse, and we’re only dealing with certain ideas in one place rather than in multiple places.
And here’s the base class that we’re inheriting from:
There’s some metaprogramming going on there to evalute our
initialize method and set our instance variables correctly based on our dependencies, and we’re also creating
attr_reader methods for those dependencies as well.
There are a couple reasons I reached for inheritance here. First and foremost, you coudln’t really get this behavior with composition. I mean, technically you could, but it would be confusing. Also, in this case every one of our child classes uses all of the behavior of the parent class. Lastly, there are no reverse dependencies (when something in the parent class depends on an implementation in a child class). Those are pretty much my rules for using inheritance, and I’ve found that it suits me well.
The one issue I have with this final implementation is that I wonder if it’s too “magic”. Unless you dug into the code for
Flag, you might not know that the
initialize method is defined for you. I’m ok keeping it for now, though, because it solved an actual problem we were seeing and it wasn’t something that was added because I thought it might be nice to have. Also, after I finished this refactoring I pulled over a couple of our less experienced engineers and asked them if they knew what was going on, and they did! That’s usually my test for if my code is too clever - if the engineer with the least experience on the team can figure it out easily, then it’s ok. Personally I think that’s a great heuristic to go by!