I work at a badly designed, overengineered project where mandatory code reviews have been introduced few months ago.
I've been asked to review a substantial piece of code that implements a new feature. It bears the same flaws as the rest of our codebase. I understand that to a large degree, those flaws creep into the new code, eg. by having to inherit from badly designed class or implement a badly designed interface while retaining compatibility, and I can't really offer much better solution that wouldn't involve rewriting half of the codebase. But I feel that from engineering standpoint, it does no good to an already broken codebase that we're breaking it even more. The code I'm reviewing is definitely bad, but it has to be if the feature is to be implemented.
How should I behave with regards to this particular review? Is there a way for me to maintain integrity and remain constructive?
Please note that I am asking about the boundary of code review in this context. I do realize that the problem is caused by other factors including organization and work culture, but what I want to know is how to handle the review itself.
Best Answer
Simple:
1. Document your technical debt
You've identified a piece of code that works but has some technical issues with it. This is technical debt. Like other kinds of debt it gets worse over time if not dealt with.
This first step in dealing with technical debt is to document it. Do this by adding items in your issue tracker which describe the debt. This will help you get a clearer picture of the magnitude of the problem and will also help with devising a plan to tackle it.
2. Gradually pay back your debt
Modify your software development process to take paying back technical debt into account. This might involve the occasional hardening sprint, or simply resolving debt items at a regular interval (eg, 2 items per week). The important part to make sure you're chipping away at your debt faster than its accruing (debt, even technical debt, has interest on it).
Once you've reached a point where you no longer have a Technical Deficit you're on your way to a healthier codebase :)