Pull Requests: Why you should never delay code quality
I’ve been there many times. I’ve seen it even more often when working with companies as a contractor. The code quality gets reduced or delayed because of the release schedule, a trade show, or something similar. Believe me, never go that way! Today, I want to show you why you shouldn’t and how to avoid this dilemma.
Let’s start with my most recent encounter with this problem.
The small app that gets just used by administrators
In one of my recent projects, I was working as a frontend developer for a team that consists of 7. It was my first frontend development project and my experience in Angular was very limited at the beginning. However, I was still the one who mainly developed a library that gets used by other teams and several small frontend applications. My teammates were more backend and DevOps engineers.
One of my colleagues wrote a great logging tool that allowed us to track when something went wrong. It was great and worked perfectly fine. But at some point, we needed a frontend and he did it on his own. I guided him through the process and stand by his side whenever he faced issues he couldn’t solve. But for sure, a few days are not enough to learn how to write great Angular apps.
At the end of the week, he created a pull request, and needlessly to say, it was my obligation to review it. Let’s be honest, it had a good portion of code smells. Some code was copied over and included CSS class names that were completely out of place in this application. Tests were not written at all and in general, the code quality was far from perfect. I told him about the issues and he reduced them to a certain degree. Anyway, there was still a long way to go (approximately one or two business days).
The famous words against code quality
He agreed with me on most points but on some (like the CSS classes) he just said, that he didn’t want to fix them yet. The UX team never had a glance at that stuff and they would obviously change it completely. The whole review process was then influenced by this information. In a call, he even said these (in-)famous words to me:
We will do it in a proper way later.
As the discussion went on, I also heard once:
I just spent a whole week on that task and the project manager and I just want to finish it now!
It was obvious that he wanted to finish it and don’t put in too much time. He wasn’t that familiar with the technology and short deadlines put some pressure on our team. At the end of the day, I approved it because I thought, this frontend won’t need many changes, the UX team will completely change it, and it’s just an app that administrators will use.
What I did was unprofessional
After approving this pull request and letting it slip through to our master branch it was changed over and over again. I had to do these changes and it cost me a lot of time. The effort that would have been needed to remove my concerns were easily offset by the time I put in to fix bugs and change the behavior.
You may assume, that the application is now in better shape because of the UX review and the complete recreation. Sadly, this redesign was never done as it was just an application for administrators and it was just ‘good enough’.
The final judgement could be, that it was the fault of my colleague who handed in code that wasn’t clean in any manner. But that’s just not the truth.
It was my fault and mine alone. I knew the technology and the flaws of his solution. I still hit the approve button. As a software developer, your responsibility is to produce high-quality code. As a pull request reviewer, your responsibility is to give feedback that improves the code and be the quality gate. I wasn’t. And after reading ‘Clean Coder’ by Uncle Bob, I know that I also was unprofessional. Again, it was me. I was unprofessional.
How to avoid this scenario
For sure, I should have just refused to accept the pull request as it was. Although that would have been the right thing to do, you could do even better.
Tracer-bullets and production code vs. prototypes
One thing that helped me a lot was to start thinking about the code I review as a tracer-bullet/production feature and not as a prototype. I use these two terms like David Thomas and Andrew Hunt in ‘The Pragmatic Programmer’. Both tracer-bullets and prototypes are ways to discover if something is technically feasible by proofing it with code. However, there is a big difference in the approach you use to create them.
A prototype is something you will throw away. You don’t have to test your code. Clean code is not important. The key takeaway of a prototype is that it works. Even if the code is just fine, you will not use it in production!
Tracer-bullets on the other hand get crafted with the same accuracy you use for usual feature development. The code is tested (maybe you even do test-driven development). Clean code is applied. You are proud of what you’ve done.
You review production code. Even if it is a tracer-bullet that should ‘only’ connect two dots of the bigger picture, it is production code and has to be of the same quality. Don’t look at the code you review like a prototype because it isn’t. Make sure you are aware of the context you are in.
Don’t make your code quality dependent on external factors
Many fall into the trap of thinking that the conditions will change. Don’t make your code quality depend on others. The UX team will review and change it later. Really? Are they? You don’t know, so keep your standards even if something will change later. The chances are high that you will have a much better time adapting your application to new requirements when you have clean code from the beginning.
And believe me, you won’t have more time in two or three weeks. Therefore, the best time to make something in a good way is right now. Otherwise, you have to live with the consequences.
Explain your reasoning
It’s easy to say that my team member didn’t have such high-quality standards as I did. To be honest with you, I don’t think that this is true. He is a great developer who appreciates clean code. He just didn't see it through my eyes. My experience in frontend development just exceeded his. So make sure, that you get the people on board. Probably, they just don’t know why one of the flaws could be bad in the long-term. Tell them, that you want to keep the quality as high as possible and that you are concerned that you won’t have time to fix it later. Chances are high, that they will agree, and improve their work.
Keep your stance
Be brave and confident. It’s your obligation to keep the quality of the product high. You can’t just lower your quality for one pull request. There is no way back from that. When it happens once, it will happen twice. And from that point, you will see chaos evolving soon.
Important: I’m not saying, you shouldn’t be open to other opinions. Too often, we developers tend to discuss things where is obviously no right or wrong (for sure your curly braces should start at the end of the line and not in the next line!). But don’t lower your standards for some external reasons like time pressure. You are a professional and people expect you to be. Live up to that role.
Offer your help regarding the open issues
Often times, you will have this issue with less experienced developers. Maybe they just don’t know who to improve the code. Offer your help will be beneficial for the two of you. First, the other developers will learn something from you, so in the future, they can do this on their own. Secondly, you will learn how to teach things to others. That’s a personal trait that’s very important for senior and lead developers. Lastly, your code will be in a better shape. It’s a win-win-win-situation.
Create a follow-up task
As always there will be some exceptions. The management really needs this feature released. The project manager is behind your neck while you review the pull-request and your career in the company you are working for could be very easily terminated if you don’t press that approve button.
The only option you have is to create a follow-up task that lists exactly what you want to improve and why you even let that original pull request through. Talk to your project manager and make sure you get the guarantee to fix the issues in the next two or three weeks.
You can delay the code quality but only if it is for a very short time and this happens not more than twice a year. Everything else will break your standards in the twinkling of an eye. There is no way back from poor code. It’s like a virus. It will spread and your code quality is gone forever.
Conclusion
Don’t let your code quality be influenced by external resources. Never approve a pull-request that doesn't meet your quality standards. At the end of the day, you will suffer from code smells. Your team will suffer from low code quality. Your project and the users will suffer from buggy and poorly implemented features. It’s your duty to keep the quality high.
Be a professional.
