18

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.

  • 1
    Did the developer document why it is done like this and maybe even wrote an issue for the core problem on the issue tracker? – Luc Franken Jul 12 '16 at 20:26
  • Scantly, and no. – Red Jul 12 '16 at 20:28
  • 4
    Is there any appetite within your organization to do something about the technical debt? – Robert Harvey Jul 12 '16 at 20:28
  • 5
    If the code is not wrong considering the environment I would not attack the code. Mostly since you wrote you don't see real improvement possible at this moment. It would create conflict without opportunity to improve. What I would do is teach to document better as a first step. Second create a procedure to have them writing clear issues. That will result in 2 things: documentation on the issue so the next one can work faster. And in addition to that you get a list of concrete issues to be potentially fixed. I like GitHub mentions because you see how many times it reoccurs if they tag it. – Luc Franken Jul 12 '16 at 20:37
  • 1
    related (possibly a duplicate): How to deal with too much pragmatism in the project? – gnat Jul 12 '16 at 20:38
25

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 :)

5

As a side note: search for a new job. This one wouldn't get any better.

The goals of the code you are reviewing are:

  • To ship a feature, which should work according to the requirements.

  • To reduce the growth of technical debt.

The first goal is reviewed by checking that the unit, integration, system and functional tests are here, that they are relevant, and that they cover all the situations which have to be tested. You also have to check the beliefs the original author may have about the programming language, which could lead to subtle bugs or to the code pretending to do something different from what it actually does.

The second goal is the one your question is focused on. On one hand, new code is not expected to increase technical debt. On the other hand, the scope of the review is the code itself, but in the context of the whole codebase. From there, you, as a reviewer, can expect two approaches from the original author:

  • The outside code is not my fault. I just implement the feature and don't care about the whole codebase.

    In this perspective, the code will copy the flaws of the codebase, and so inevitably increase the technical debt: more bad code is always worse.

    While this is a valid short-term approach, in long term, it would result in increasing delays and low productivity, and eventually lead to development process being so expensive and risky, that the product would stop evolving.

  • Writing new code is an opportunity to refactor legacy one.

    In this perspective, the effect of the flaws of the legacy code on the new one could be limited. Moreover, the technical debt could be reduced, or at least not increased proportionally to the code growth.

    While this is a valid long-term approach, it has its short-term risks. The major one is that, short-term, it would sometimes take more time to ship the specific feature. Another important aspect is that if the legacy code is untested, refactoring it presents a huge risk of introducing regressions.

Depending on the perspective you want to encourage, you may be inclined to advise the reviewees to refactor more or not. In all cases, don't expect flawless, clean piece of code with nice architecture and design inside a crappy codebase. What you shouldn't encourage is the behavior where a knowledgeable developer who has to work on a crappy codebase tries to do his part well. Instead of making things simpler, it only makes them more complicated then before. Now, instead of uniform bad code, you have a part with design patterns, another part with clean, clear code, another part which was extensively refactored over time, and no unity whatsoever.

Imagine, for instance, that you are discovering a legacy codebase of a medium-size website. You're surprised by the lack of any usual structure and the fact that the logging, when it's done, is done by appending stuff to a text file by hand, instead of using a logging framework. You decide for the new feature to use MVC and a logging framework.

Your colleague is implementing another feature and is very surprised by the lack of an ORM where one would make perfect size. So he starts using an ORM.

Neither you, nor your colleague is able to go through hundreds of thousands of lines of code to make the use of MVC, or a logging framework, or an ORM everywhere. Actually, it would require months of work: imagine introducing MVC; how long would it take? Or what about an ORM in situations where SQL queries were chaotically generated through concatenation (with occasional places for SQL Injection) inside code no one could understand?

You think you did a great job, but now, a new developer who joins the project have to face much more complexity than before:

  • The old way of treating the requests,

  • The MVC way,

  • The old logging mechanism,

  • The logging framework,

  • The direct access to the database with SQL queries built on the fly,

  • The ORM.

On one project I was working, there were four (!) logging frameworks used side by side (plus manual logging). The reason is that every time someone wanted to log stuff there was no common approach to do it, so instead of learning a new framework (which in all cases was used only in 5% of the codebase), one would simply add another one he already knows. Imagine the mess.

A better approach would be to refactor the codebase one step at a time. Taking once again the example of logging, refactoring would consist of the following small steps:

  • Find all the places where legacy logging is done (i.e. when the log file is accessed directly) and ensure they all call the same methods.

  • Move this code to a dedicated library, if applicable. I don't want logging storage logic in my shopping cart class.

  • Modify, if needed, the interface of the logging methods. For instance, we can add a level indicating whether the message is informal, or is a warning or an error.

  • Use the newly refactored methods in the new feature.

  • Migrate to the logging framework: the only affected code is the code within the dedicated library.

  • 1
    Great answer until the last paragraph. Whether you intended it or not, you're implying that good practices shouldn't be used for new code. -1 – RubberDuck Jul 12 '16 at 21:50
  • 2
    @RubberDuck: it's not about good practices, it is about writing code radically different from the remaining codebase. I've been that developer and I've seen the consequences of what I've done: having drastically better code among bad code makes only things worse; what makes it better is to improve the codebase through small refactoring steps. I'll add an example in my answer in a few minutes. – Arseni Mourzenko Jul 13 '16 at 7:40
  • I would DV again if I could. The edit makes it worse. Having four different methods for doing something a new way is bad, but is the fault of the guy who adds the second logging framework. It's not bad in and of itself to draw a line in the sand and have clean code next to rotten code. – RubberDuck Jul 13 '16 at 9:06
  • @RubberDuck: It's not about adding the second logging framework, but the first one. The guy who adds the second logging framework does that just because the first one is used on only one small feature; if the codebase was refactored as I advise, this wouldn't happen. – Arseni Mourzenko Jul 13 '16 at 9:14
  • I think you and I agree, but your answers reads in a way that discourages improvement. I just can't get on board with that. – RubberDuck Jul 13 '16 at 9:43
3

If you are doing code reviews as part of your development process; then you must set the rules against which you 'judge' the code you are reviewing.

This should go into your 'definition of done' and could be a style guide, architecture document for the codebase or static analysis, checking for legal requirements, whatever the company decides is its requirements for 'code quality'

Once you have this in place code reviews become a matter of course, has the style guide been followed, do we have the required percentage of test coverage etc etc.

If you don't have this in place then code reviews can just become a battle of who has the biggest coding willy or, as in your situation, questioning of judgement calls about refactoring vs features done before the deadline. Which is just a waste of everyone's time.

3

Your main problem is that a code review on a significant new feature is the wrong time to have this discussion. At that point it's too late to make anything but minor changes. The right place is in the planning stages, or in a preliminary design review at the latest. If your company isn't doing those early reviews at least informally, you should work on changing that culture first.

The next step is to get yourself invited to those meetings, and have productive ideas at those meetings. Mostly that means not trying to change everything overnight, but looking for bite-sized chunks you can isolate and tackle. Those chunks will add up significantly over time.

In other words, the key is to regularly suggest smaller changes toward the start of projects, rather than getting shot down suggesting larger changes toward the end.

2

In the places where I've been doing code review, I would accept and OK the code submission, if it comes with a commitment to do (at least) some refactoring. Be that as a filed bug, as a story, or a promise to send another review with (some) refactoring done.

In these cases, if I am the person writing the code to review, I usually prepare two changes, one with my new feature(s) or bug fixes, then another that has that AND some clean-up. That way, the cleanup changes don't detract from the new feature(s) or bug fix(es), but is easily pointed to as a token "yes, I know this doesn't fix these issues, but over there is "pure clean-up" that does".

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.