This article describes how we spent almost four days working on a major refactor and the lessons we learned from the process. We hope that these lessons will help you have a smoother and quicker experience the next time you undertake a similar task.

‍

A little bit of context

a. The project

The project I'm currently working on aims to engage its users by letting them complete quests on their social networks. For instance, a user can earn points by completing a quest like "Like 5 tweets from this Twitter account", which would increase engagement around the specified account. The users at the top of the leaderboard would then receive various types of rewards.

On the technical side, we have a Nest.js backend written in Typescript that is responsible for performing requests to a user engagement service, i.e., the place where the quests of each user are kept up to date, and parsing the responses so that our front-end application, written in React Native, can present them.

‍

b. The issue: a messed up adapter

One of the key responsibilities of our backend is to compute various pieces of information about each quest. These include:

• The progression of the quest, which is a ratio between the user's current progress and the target needed to complete the quest. The progression could be in the form of a duration (3min/5min), a count (3/5), or a price (20$/30$).
• The actions related to the quest, such as clicking on a quest to open Twitter and performing the quest "Like 5 tweets from this Twitter account".
• The locks of the quest, which determine whether the user is currently able to perform the quest. For example, a user without a linked Twitter account would not be able to perform a Twitter-related quest.
• The rewards that can be earned by completing the quests, which could be of different types.

Computing all this information was done in a file, called ++code>quests.adapter.ts++/code>. There were several issues with the adapter, mainly:

• Making a change to the code could take quite a long time because we would need to make sure nothing was broken;
• Some functions were long and hard to understand.
• Some computations were run several times.
• Many of the bugs we found when testing the app were connected to this adapter.

Therefore, we recognized the benefits of refactoring this file, and I was assigned the task to do so.

‍

The refactor

a. Finding the causes of our problems

The first step in the refactoring process was to identify the root causes of the problems we observed with the adapter.

i. The code was difficult to read and modify

• The adapter was written in a single file, consisting of over 400 lines of complicated code.
• Some functions did not adhere to the Single Responsibility Principle (SRP), which made modifying them require a significant amount of cognitive load.

ii. The code broke frequently

• After investigating bugs related to the adapter, we found that most of them could have been caught if our test suite was more comprehensive.
• We also realized that the complexity of the code made it more challenging to avoid introducing bugs when modifying it.

iii. Improving performance

• We did not memoized the computation of certain pieces of information, which hurt the adapter's performance.
• There was no clear diagram showing the flow of information and how it is modified from input to output. As a result, it was difficult to identify points where we could apply memoization.

‍

b. Performing the refactor

i. Planning the steps

After identifying the causes of our problems, it was easier to plan the refactor in successive steps:

1. Write a diagram that shows the flow of information in the adapter. This will provide a clearer understanding of the code and allow us to plan memoization.
2. Write the missing tests in our test suite.
3. Refactor the code:
4. Split the logic for computing progressions, actions, locks, and rewards into separate files.
5. Simplify the functions that violated the SRP.
6. Memoize the pieces of information that were computed multiple times.

‍

ii. Creating a Diagram

To determine when each piece of information needed to be computed, we began by creating a diagram illustrating the flow of a quest in the backend:

‍

First, quests that are no longer relevant, such as finished quests, are removed from those received from the user engagement service. This requires computing the progression.

Next, in the adapter, the remaining pieces of information are formatted in a user-friendly manner.

‍

iii. Writing the missing tests

We discovered several bugs related to the adapter while testing the app. These bugs were not covered by any test cases, otherwise, they wouldn't have made it to the staging version.

To address this issue, we made an effort to write test cases for each of these bugs. This provided us with a safety net during the refactor process. By having tests that detect when a bug is reintroduced, we felt much more confident in making changes to the code.

‍

iv. Performing the Refactor

We began by splitting the adapter code into multiple files so that each file and its associated adapter function could focus on computing only one piece of information. This resulted in the following files:

• ++code>quests.adapter.progressions.ts++/code>
• ++code>quests.adapter.actions.ts++/code>
• ++code>quests.adapter.locks.ts++/code>
• ++code>quests.adapter.rewards.ts++/code>

Since each file had only one simple goal, the adapter function interface could be simpler. This allowed us to pass less information to each adapter and simplify the code. The tests also became more straightforward.

To address the SRP violations, we broke down each function into several smaller and simpler functions. This led to an improvement in the overall quality and readability of the codebase.

Finally, the diagram we designed in the previous step helped us perform the adaptations in a better order. By computing the progressions first, we eliminated the performance issue of computing some quest progressions multiple times.

‍

Results

a. What went well

i. Good splitting of commits.

‍

Using small commits made it easier for the PR reviewer to follow the refactor history. Each commit performed a small and precise task, so the reviewer only had to consider whether the commit fulfilled the goal stated in its description.

The website Refactoring Guru advises that each commit in a refactor should be equal to an atomic operation listed on this webpage, such as "move method," "replace magic number with symbolic constant," or "decompose conditional". We followed this principle to the best of our ability.

‍

ii. Applying TDD

Test-driven development (TDD) is a coding practice that requires tests to be written before writing the corresponding feature code.

When refactoring, it's essential to ensure that there is at least one test covering any piece of code being edited. Having a test before updating a function makes refactoring safer, as the test will throw an error every time the function breaks. This allows us to proceed in small steps:

1. Perform an atomic refactor (e.g., extract a variable, rename a function, simplify an if/else block)
2. Check that the test still passes
3. Go back to step 1

To identify parts of the code that aren't covered by a test case, you can use the command ++code>jest —coverage++/code>. This command helps find statements, branches, functions, and lines that aren't covered by a test case.

‍

iii. Listing the bugs first

One useful action we took was to review the previous bugs we had encountered in the adapter. This allowed us to ensure that each bug was now covered by a test, preventing its reintroduction into the codebase.

This, along with TDD, helped us increase the code coverage of the adapter. We ran ++code>jest --coverage++/code> on the codebase before and after the refactor, and obtained the following results:

‍

iv. Respecting the SRP

Before the refactor, it was difficult to make changes to the adapter because a file could contain functions with different goals. Additionally, each function could perform multiple actions, breaking the SRP. As a result, we often hesitated to update a function for fear of breaking something else.

During the refactor, we focused on splitting functions or files with more than one concern. This made it easier to make changes to the adapter's code.

After the refactor, the team noticed that implementing some features related to adapting quests was easier, compared to before the refactor.

‍

v. Removing Duplicated Computations

Finally, we were able to find out that some progressions were being computed twice while studying how the adaptation of the quests was implemented. By rethinking how the information flowed in the adapter, we found a way to compute the information only once. This was the cherry on top of our findings.

‍

b. What could have been better

We have identified two things that could have been done better.

i. Split the PR into several smaller ones

The refactor was done in a single pull request that changed over 2000 lines of code. This meant that the reviewer had to spend at least 30 intense minutes checking for errors in the modified code.

To make the review process easier and more efficient, consider splitting the pull request into several smaller ones.

‍

Reviewing a pull request requires a high level of concentration, and doing so for a long time may prevent the reviewer from finding issues with the code.

Additionally, splitting a PR into several smaller ones can yield several benefits:

• The work that has been done and reviewed can be marked as complete.
• The rest of the team can gradually become familiar with the refactored code.
• There is less chance of having a merge conflict because someone else edited a part of the code that was touched by the refactor.
• The team member who performs the refactor can receive useful comments in the first pull requests that could be taken into account in the following ones (see the diagram below).

‍

ii. Perform the refactor during a stable moment.

‍

One issue we faced is that this refactor was performed at the same time we had our first public release. During the release, we noticed some bugs in the adapter that we had to fix. These fixes caused the adapter to evolve on the main branch, while the refactor branch also had to keep up with the changes. As a result, I had to resolve many merge conflicts, which was not a great developer experience.

Therefore, if possible, it is best to perform the refactor when the team knows that there won't be many updates to the code being refactored.

‍

What you can learn from our experience

This refactor gave us a lot to think about. Many things went right, but we failed to implement some good practices. Here is a small list of what we feel you should apply during your next refactor:

• Make sure that it is worth making a refactor in the first place.
• Plan your refactor during a stable moment in your project's lifecycle.
• Create a diagram of how the module/file/function you want to refactor works.
• Fill the gaps in your test suite before starting the refactor.
• Plan several small PRs with atomic commits.
• Use TDD.
• Split your files if they are too big.
• Split your functions (and simplify their interface) if they break the SRP.
• Check your test coverage at the end of the refactor.

I hope you have learned something from my experience! Let me know if you spot any mistakes or encountered any issues.