Refactoring the CategoryList class in Discourse

The CategoryList class in Discourse provides a good opportunity to bring up an issue that you might come across often, which is that the method that needs refactoring is a private method.  In this case, the CategoryList#add_uncategorized method is private but is in need of refactoring to break apart the many different tasks the method is performing.

This could be an issue because the tests for the class only test the final outcome of the public method that calls the private methods.  This is exactly as it should be, but I often find that a method that does way more than it should does not have sufficient testing.  The fact that this method is private complicates things because you have to make certain choices on how to proceed.  Often, the easiest way to tell if the testing is suspect is to remove a block of code that looks like it should cause the method to alter its outcome and in return cause the tests to fail.  If you do this and the tests still pass then you want to check it out more thoroughly.

My preferred method of dealing with private methods, especially in less familiar code-bases, is to temporarily make the method public and write tests to examine the code better.  Once you have done this, you can move forward with your refactoring and have much more confidence that you aren’t altering the functionality in a way you didn’t intend.

For this example, I’m going to move forward with the assumption that the tests fully cover all intended functionality but I’m not going to push my changes up to Master because I do think more tests are needed. Adding those tests may be the subject of another post in the future.

As you can see, the starting point for the CategoryList class is pretty good.  The B grade for the class is only brought down because of one complex method which is what we will be working to make less complex.



The code of the method that we’ll be refactoring is below.

Our initial look at the method shows us several places that we could start with our refactoring.  I usually look for comments and if statements that allow us to begin splitting functionality into smaller methods.  This class has several calls to the Topic and Category classes that are also hints of where we can begin to refactor.

In this case, I chose to begin by moving the functionality that creates the various variables inside the method in order to simplify things a little before I continue.

The assignment to uncategorized_topics becomes:

The assignment to totals becomes:

The assignment to uncategorized is now:

Making these changes allows us to see the add_categorized method a little more clearly now.

Next, I create two new methods to encapsulate the functionality that inserts the category in the proper location and adding the uncategorized topics to @all_topics.

This leaves add_categorized with a lot less functionality directly inside the method.

Now that we have extracted the different types of functionality into their own method, we can turn our attention to refactoring each of the methods that were extracted.

The first thing that needs to be done is to move the calls to Topic into the Topic class itself.  This functionality is tied directly to Topic so that is the class that should handle this responsibility.  To do this, I created two new methods in Topic named totals and uncategorized_topics.

The get_uncategorized_topics and get_topic_totals methods can now be removed from the CategoryList class and those calls can be replaced with calls to the new methods in Topic.

The next step is address the creation of the new Category.  A better approach is to create a new class that is named appropriately and conveys the intent of what is being done.  This is accomplished by adding a new class named UncategorizedCategory that inherits from Category.

If you look at this class closely, you see that we simply pass the same object to the constructor of the class as we did when using the method.  The one difference is that we moved the call to Topic#totals to the initialization of the new class because it isn’t used anywhere else in CategoryList.

The last thing to address is the insert_uncategorized_category method.  I’m not a big fan of initializing a variable to nil if it can be avoided.  In this case, the more idiomatic catch/throw can be used to clean things up.

One last tweak we can make in the insert_uncategorized_category method is to remove the OR in the line shown below.

Since the OR is checking to see if our previous block returns nil, we should just move the value that we want to be returned instead of the nil.  The gist below shows this change.

At this point, the functionality has been separated out so that each method is responsible for a much smaller piece of the puzzle.  After merging the branch back into my local copy of master and updating the repo on CodeClimate, we can see that the grade has increased to an A.


One Final Note

If you were paying VERY close attention, you might have caught something that I haven’t touched on yet.  If you look at line 41 of the original gist you will see that this line just disappears. This appears to be a phantom line from previous functionality because it doesn’t really to anything as far as I can tell. If this is in your code, this is a tell-tale sign that you had better have comprehensive tests in order to have confidence in what you’re putting out there.

Be the first to leave a comment. Don’t be shy.

Join the Discussion

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <s> <strike> <strong>