A Short Cautionary Tale About Refactoring
This week I was making changes in a part of the codebase I was unfamiliar with. While reading the code I spotted an inlined PNG image. This was not a small icon but an image of about 100kb in size. I searched the project and found the same inlined image in a couple of different components. Maybe it was because I was in a need of some coffee, but at that moment I could not think of a good reason for this approach. As the components with the inline image were quite similar, I surmised that they were copied from each other and the inline image is just one of those weird things that you find in legacy code.
I decoded the embedded image from Base64 and created a separate PNG file from it. The final step was to replace the inlined image source with a relative path to the new image file. The diff for the commit was almost perfect: large red blocks of deleted code and just a few green lines of added code. A quick and easy refactoring win and a textbook example of the boy scout rule. Or so I thought.
I took a short break and grabbed some much-needed coffee. Fast forward a few hours and I completed the task that I was originally working on. I was in the final stages of testing when I noticed a bug. At that same moment, I also realized the exact purpose of the inlined image. Can you guess?
To give you more context, we use the components with the image to display error messages. The image is a cute illustration of the company mascot with the expression “Oops something went wrong”. One of the possible errors that can happen, is when the user losses their connection or goes offline. So when that happens and you render a component that still needs to download an image, the image will fail to load. Oh yeah…
I thought I had learned the hard way earlier in my career not to presume I know better than the original author. Of course, there are situations where a less experienced coder (ie. yourself from a couple of years ago) worked on a feature or where someone had to cut corners (eg. due to a deadline). But usually, there is a good reason why someone took the approach that they did.
Luckily I spotted the issue before I pushed my branch. I could use git reset --soft and alter my changes. I extracted the inlined image into a separate React component and used it where needed. This way we still get both benefits: the error components are much cleaner and easier to read, and the inlined image is located in one place. After that, I reapplied all the other commits.
I know, this bug would not be the end of the world. But this small example shows how even the best of our refactoring intentions can do more harm than good. If you find yourself looking at a piece of code that looks strange or needlessly too complex, ask your coworkers. This is especially true if the original author is still at the company. You might be missing some context and you’ll get to learn something new. Or you can indeed improve the code, in which case the original author may learn the better approach. In both cases, it’s a win. Also if there is a good reason for the current approach, document it. Add a more descriptive name or a comment to avoid confusion in the future.
Learn from my small mistake and research before making refactorings. This is 10 times more important for larger changes and when editing the core files of the project. And the most important thing: don’t forget to drink some coffee before starting.
Update: As pointed out by others, I should have also added a comment in the new component explaining why the image is inlined. In my conclusion of this post, I did point this out but missed doing it while submitting my commits. So a bonus mistake I made so you don’t have to.
Update 2: If there are no unit tests for the code you want to refactor, add them before starting. Having unit tests ensures that you have correctly refactored the code and have not introduced new bugs. Future developers can also read the unit tests to better understand what problems the code is solving and see edge cases that they may otherwise miss.
Update 3: Users on Hacker News have pointed out Chesterton’s Fence principle. If this is the first time you’re hearing about it and have 8 minutes to spare, I highly recommend you read the linked post.
See the discussion about this post on Reddit and Hacker News.