LoveWithFood Tech

LWF

Refactoring in Code Reviews: My Experience

| Comments

I’ve found that refactoring helps me review someone else’s code. Before I started using refactoring, I could read the code, understand some degree of it, and make suggestions. Now when I come up with ideas, I consider whether they can be easily implemented then and there with refactoring. If so, I refactor. When I do it a few times, I can see more clearly what the code looks like with the suggestions in place. I don’t have to imagine what it would be like, I can see what it is like. As a result, I can come up with a second level of ideas that I would never have realized had I not refactored.

I found this gem when I was reading “Refactoring” by Martin Forwler. It was an interesting idea. Like any other programmer, I like to refactor. You’re turning messy code into a showcase of craftmanship. It’s a very therapeutic process.

Code reviews on the other hand can be stressful. You’re trying to understand someone else’s code primarily through reading. You’re also trying to understand two sets of code: the code before the pull request and code after it.

Thus, I had situations before when I just accepted code without truly understanding it. I would say something like “I don’t understand the code completely, but it looks well-structured and easy to change.”

Turned out those code weren’t easy to change.

So why not apply refactoring as a way to understand code while reviewing it?

Why did I decide to try refactoring during code reviews?

As mentioned, my primary reason for refactoring code during code reviews is to help me understand the submitted code better. Refactoring is our equivalent of the newpaper editor’s red pen; we can use it to correct code style issues, rename variables, extract missing methods and delete entire classes.

I have also come to the conclusion that it’s a fast way to do code reviews. In the same way that refactoring code before adding new features is faster in the long run than just slogging through ugly code, I believe that refactoring during code reviews is faster than reading someone else’s code, commenting about it, clarifying the comments with the code’s developer and waiting for the developer to make the changes.

By making the changes yourself and explaining the motivation for the changes in the source control commits, you have already communicated all that needs to be communicated with how you think the code should be. And with tests in place, you have demonstrated that the changes you wanted to add won’t break the code.

What and what not to refactor during code reviews

That being said, refactoring during code reviews doesn’t mean you can just change any code in the pull request. You have to follow one rule, the rule that defines what refactoring is. You don’t want to change behavior.

Refactoring means changing the internals of the program without changing its behavior. With code reviews, this would result in a refactored pull request being logically equivalent to the original pull request. With tests in place, you can be confident that you haven’t introduced bugs while making changes to the code. This allows you and the original developer to focus your discussions on the quality of the code changes, not on whether or not the code changes are valid.

What if you find bugs while refactoring? Should you fix them? Unless it’s the simplest of bug fixes, I think it’s best to let the original developer fix the bug. Two things are working against you: you don’t have the clearest understanding of the code and you’re working with an unstable/invalid environment. Finally, if you do try to fix the bug yourself, you and original developer might end up having different understandings of the code. That would make discussions about your refactorings more difficult.

Finally, there may be cases during refactoring when you notice that you can’t move forward without making major changes to the design of the code. For example, lately I reviewed a pull request where a dialog box was using Ajax to fetch the contents of the box. As I was refactoring the pull request, I had the idea that instead of Ajax, we could easily render the contents of the dialog box as part of the page. That way we don’t have to require JavaScript when running the acceptance test for the feature.

Since I felt such a change would be drastic for the feature, I asked the pull request developer first what he thought of the idea. After some discussion, we decided for him to do the change. I think that was for the best since he knew the pull request better than I did.

How do I incorporate refactoring during code reviews?

What I’m doing right now is to create a separate branch and pull request for the code review refactoring. This gives the original developer a chance to review and respond to the refactorings. The only exception I follow is when the pull request is already good enough to merge except for a few minor issues, like for example, incorrect indentation. In these cases, I go ahead and make the changes in the branch before merging them to the master branch.

I start reviewing pull requests through the acceptance tests. We use RSpec for our apps, so I take advantage of RSpec’s let, describe, and context features in order to understand the test code better. If I find any test variables confusing, I rename them or extract new let variables from them.

I then follow the flow of the acceptance tests, leading me to the implemented classes and their own tests. I concentrate on the major classes, refactoring them from the bottom-up. I start with code style issues, moving up to renaming variables, then extracting methods and finally to refactoring classes.

I try to keep commits as small as possible, essentially making a step-by-step record to how my mind was processing the code. These small step-by-step commits would help the original developer follow the code transformations easier.

What things did I learn by refactoring during code reviews?

Overall, here were some tips I learned when refactoring during code reviews.

  • Create a separate pull request for the code review. Give the original developer a chance to review and respond to the refactorings.

  • Refactor from the bottom up. It’s often easier to refactor a class one method at a time, building on the methods to eventually refactor the class itself. At the same time, such sequence of transformations tend to be easier to follow for the original developer of the code.

  • Prefer small commits. This way, the original developer can easily follow the sequence of the refactorings and confirm that they haven’t changed the logic of the original pull request.

Finally, it’s important that you’re willing to let go of the refactorings if needed. When you refactor code, you improve the code based on your coding principles. Sometimes though, those coding principles would differ from the principles followed by the original developer, or other developers in your team. The refactorings would eventually bring up those differences to surface.

And that’s okay. That’s how the process should work. Most of the time, you’d find common ground. But in cases when you don’t find common ground, don’t feel bad about shelving the refactorings you’ve worked on. You may think they were a waste of time, but they weren’t: the refactorings allowed you to understand the code.

Eventually, time is the best judge of any kind of code. More often than not, time is very forgiving, with good code and sometimes even with butt-ugly ones. Time can forget about code that we treat now as life-and-death issues.

However, when time does protest loudly about code being too difficult to handle, your experience with refactoring the code will allow you to have a more mature understanding of the code. You’ll be able to make better decisions on how to improve the code because of your experience with refactoring it.

Comments