A Quick Backstory
When I started working at Axelerant, my first project was ContribTracker, a website to keep track of our community contributions. The site is now being revamped for a newer look and gearing up to implement new frameworks, so we realized it about time to refactor the code-base.
Since it was written for a hackathon, there was little time to write a readable and well-maintained code. But, now we have transitioned this code-base to an internal tracker at Axelerant with an expanded team. We should be refactoring it so that it is maintainable, given the significance of it in the long-run.
How a maintainable code is related to code-refactoring? As a common maxim in programming: you code once, but you read it several times.
Refactoring code makes it more readable with a more transparent presentation by improving the internal structure and without altering the external behavior.
A code smell is an indicator of something that could be improved in the existing code-base. Though to identify what improvements could be made is gradually acquired with experience, some common code smells that we could easily spot are duplicate code, code with numerous comments, long methods, and so on. Even though a code smell could help us notice what could be improved, it doesn't tell us what to do.
With over 75-100 well-defined techniques, we will be discussing a smaller subset of the most-common refactoring techniques that we used in code-base along with their associated code smells in the upcoming sections.
In the beginning, the easiest way to start with refactoring is to do it on a method level. A quick thing to check first is if the method is too long. An extended method is a classic sign of the process being too much, and it would probably benefit by splitting it into several other smaller methods.
Here are some other key points to consider when dealing with generalizations:
- Is this method well-named? Does it convey what it is doing, and is it doing more than how it is named?
- Is the method performing only one thing or multiple things?
- Do the parameters make sense?
- Does it use a lot of temporary variables?
- Is there any duplicated code here?
- Is there too much going on? Might this benefit from being split apart?
ContributionManager::storeCommentsByDrupalOrgUser(), the scope of the class here remains to oversee the functionalities involved with storing the comments the user made in the drupal.org. Since we decided to abstract on the method level for refactoring, let's break down the operations handled by the function.
- Logger notice
- Retrieving Issue comment data
- Retrieve patch file details associated with the comment
- Determine the status of the issue
- Retrieve the project name for the issue
- Save the Issue comment
- Send Slack Notification
We could see that the first code smell is that the function is too long, and it does more than what it’s named after. One of the common refactoring technique in which this method could be refactored.
A. To look more closely and identify a few lines that logically belong together and to create a new method from those lines. One of the benefits of a smaller function is to reuse and reduce to a more specific and modular method.
B. If a section of code requires a comment to explain the logic behind it, it is a code smell prompting to move the logic to a different method instead of writing a comment.
While extracting the method to a smaller function, the best practice would be to question whether there are too many parameters in the function call. A classic code smell known as the feature envy is when a method seems to be far more interested in data from another class than the one it is in.
On further expanding from the code smell of feature envy with the focus on data, from the above example we have named the class of what we wanted the behavior to remain, but looking more closely into it, we realize that the comment is not just a string anymore, but over time this has evolved into something that has a behavior attached to it.
Design Patterns And The Importance Of Adopting Them
A design pattern is a general repeatable solution to a commonly occurring problem in design, and they provide targets for our refactoring. Following a pattern provides a template on solving a problem that can be used in many situations. Precisely, patterns are a template of reusable solutions that our code could adopt, and refactoring is the technique that we use to reach the desired pattern that we are following.
Replace Data Value With An Object - Pattern
The behaviors attached to a data value could be moved to a new pattern called Replace Data Value with an object. So, changing the primitive data value to an actual object gives the flexibility of adding additional attributes to it as the system grows and provides an opportunity altogether to add a new data value object. While moving to this pattern, be aware of refactoring and replicating the functionality as closely as possible.
By adopting the pattern to the comment object, below is the refactored code.
A quick recap
With a handful of techniques that we discussed in the above post is very much limited to the ones that we used and that helped us in refactoring our code-base, we'd suggest while starting to refactor, it is always better to focus on code smell first and then to proceed with refactoring. Start from the bottom, like renaming and extracting methods, shortening parameters list, minimizing the unnecessary use of temps, and making conditionals easier to read. Even though it's tempting to look at the bigger picture of how the overall changes that we made could make better use of the patterns, it's almost impossible to figure it out within one go. The preferable way is to address and clear the code at the lower level first and to work our way up slowly.
Before wrapping up - Importance of Automated Testing
When preparing to refactor, we’re changing code that might invite risk. The single best way to prove the changes that we make only does what we intend to and doesn't unwittingly break everything else is to have a set of automated tests that could be run before and after the changes. There is no technique in refactoring that would force us to use automated tests, but it's a reliable way to show that the changes made do not break properly functioning code