Archive for the ‘Discourse’ Category

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.

category_list_starting_grade

 

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.

category_list_ending_grade


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.

Tell, Don’t Ask. Refactoring the SearchObserver class in Discourse

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.

Extracting the InviteRedeemer class from the Invite model in Discourse – Part 3

The results of previous post left us in decent shape to finish up the refactoring of the InviteRedeemer#redeem method.  The gist below shows the starting point for this portion of the refactoring. The private methods are left out for brevity.

The first thing to tackle is the multiple assignments to the result variable.  The first change is to replace the nil assignment in ‘result = nil‘ with a call to a new method named invited_user.

By adding the method above, the else branch of the if clause can be removed. In order to make sure everything still works as expected, the tests are run and everything is green.

The second benefit of this addition is that it is no longer necessary to pass the result to the methods because the user can be retrieved from the invited_user method from the method itself.

The next step is to replace the ‘mark_invite_redeemed == 1‘ line with something that has a name that is more explanatory. To do this, a private method named invite_was_redeemed?.

After the change above, move the steps inside the if clause into their own private method named process_invitation.  

The redeem method now looks like the gist below.

At this point, things are looking really good …except for the repeated references to result. But upon taking a better look, it becomes obvious that we can rearrange things a little bit to contain all of the user retreival functionality in one place.

The changes above mean that the get_invited_user method will first check for the an existing user and then create a new user only if one doesn’t exist.

By keeping the user retrieval inside one method and combining it with the lazy initialization of invited_user, it means that we can remove result from the redeem method completely which gives us our final result.

A final check on Code Climate shows that the score for the Invite model has gone from C to A.

Image

This change was submitted to Discourse and was merged as pull request #927

Extracting the InviteRedeemer class from the Invite model in Discourse – Part 2

In the previous post, the InviteRedeemer struct was extracted in order to isolate the functionality required to redeem an invite.  When we left off, the redeem method had been moved to InviteRedeemer and all the tests were passing which gives the assurance that we have preserved all of the functionality required.

The starting point for the next steps are based around the comments within the method.  Each highlights different pieces of functionality that can be split out into a private method with a name that is indicative of its responsibility. I started by extracting a private method for each of the actions marked by a comment.

At this point, InviteRedeemer looks better because we can see that there are very procedural steps to what is happening but there are still many things that need to be corrected.

The multiple assignments to the result variable as well as the number of times result is passed around shows us that there’s still a lot of work to be done.

One thing it does do is to make the next step of cleaning up the row_count assignment and comparison a little easier to see.  The result of this step is shown below.

This still looks a little weird with the result of the update being checked to see if it is equal to one, but we will deal with that in the next post.

 

 

Extracting the InviteRedeemer class from the Invite model in Discourse – Part 1

After poking around the Discourse project on Code Climate, I found that the Invite#redeem method was given a high complexity score.

Starting Grade

After studying the redeem method for a bit, I decided it would be a good subject for the next refactoring. The method that we started with is shown below.

While looking at the original method, there are a few things I saw that guided this refactoring.  The first is that the class is named Invite but the redeem method performs a lot of actions that go above and beyond what would be considered a single responsibility.

Invite shouldn’t care about how it is redeemed, it just needs to know if it has been redeemed or not.  With this as the starting point, I created a new InviteRedeemer struct that is called from within the Invite#redeem method.

The Invite#redeem method becomes the following:

To get the functionality moved, copy the contents of the redeem method from Invite to InviteRedeemer.  Running the tests will guide you to the next few steps which will be to preface any method calls that were previously called on the invite object itself with the invite attribute that is passed into InviteRedeemer. Repeat this until all of the tests are green again.

Now that the functionality is moved and all the tests are green, the process of refactoring the internals of the InviteRedeemer#redeem method can begin. This is where I’ll pick up in the next post.