The latest class I came across in the Discourse code base provides a good opportunity to demonstrate the idea of “Tell, don’t ask”. After seeing Sandi Metz’s talk at Ancient City Ruby, I’ve been very conscious of limiting any type checks or a lot of conditionals that collect a bunch of different logic in one method.
The starting point of the SearchObserver class in Discourse is a good example of code that does a lot of asking to determine what it is going to do.
The after_save method is full of type checks and if statements that direct the flow of logic. My initial thought is that this class is taking on too much responsibility so I want to refactor the class to put the logic where it belongs and make the code more “confident”.
In order to get to the final result, I’m going to start outside of after_save method. After examining the SearchObserver class, I found that the HtmlScrubber class is only used by the Post model. With that being the case, it makes more sense for this class to reside within Post since that’s the only place it is used in the entire app. If we needed this functionality elsewhere in the future we could put it into its own file but I don’t want to prematurely optimize anything at this point.
After copying the entire HtmlScrubber class into Post, I create a method named scrubbed_html and then make the changes inside the SearchObserver#after_save method to handle our changes.
After these changes, it makes it a little easier to see that each if statement eventually ends up passing the same three variables to the update_index method. Because these three variables are passed together, it makes sense to create a separate value object that encapsulates this information. I’m going to start by encapsulating the id and search_data variables first because there is quite a bit of logic that surrounds which class type we’re sending to the update_index method.
After moving the creation of the search_data variable into the proper location in the after_save method, I can now create an object to pass the id and search_data variables, the SearchObserver class now looks like the gist below.
Now that two of the three parameters are being encapsulated, we can turn our attention to the third parameter. This parameter is just a lowercased version of the class name which means we can pass the object itself into our value object to create this value. It will also give us access to the object’s id value that we were passing in by itself previously.
SearchObserver after the changes…
Now that all three of the individual update methods are identical, they can be removed and the value object can be passed directly to update_index.
The next step is to remove the creation of the table_name and foreign_key variables from the update_index method and move them into the ObserverSearchData class.
The update_index method looks like the gist below…
The code is in much better shape now, but the update_index still knows too much about what it needs to do. To take care of this we move the update_index method from SearchObserver to ObserverSearchData and make the necessary changes.
The SearchObserver class now looks like the gist below.
Now that we only have the after_save method in SearchObserver, we can turn our attention to the type checking and logic that builds each of the search_data variables. A better home for each of these is in the model itself. In order to accomplish this, we want to create a method in each class with the same name so that we can call the method with confidence without worrying what type of object it is. After moving the logic to each controller we end up with:
The changes above allow the SearchObserver to tell the given object that it needs to update the index which is a much better design because we can now add objects in the future without having to change SearchObserver at all. The new object would simply need to implement the update_search_data_index method with the required logic.
This refactor is an excellent example on how to make your code more confident as well as making sure that each class is better at adhering to the single responsibility principle.
There are still several more steps within each of the classes that can be performed to make this code even better. In addition, it would be better to move the tests to the spec for each model so that we don’t have to hunt down where they live in the future but for now we’re left with code that is much better than what we started with.