What do you do when code review is just too hard?

Question

OK so a lot of code review is fairly routine. But occasionally there are changes that broadly impact existing complex, fragile code. In this situation, the amount of time it would take to verify the safety of the changes, absence of regression, etc. is excessive. Perhaps even exceeding the time it took to do the development itself.

What to do in this situation? Merge and hope nothing slips through? (Not advocating that!) Do the best one can and try only to spot any obvious flaws (perhaps this is the most code review should aim for anyway?) Merge and test extra-thoroughly as a better alternative than code review at all?

This is not specifically a question whether testing should be done as part of a code review. This is a question asking what the best options are in the situation as described, especially with a pressing deadline, no comprehensive suite of unit tests available or unit tests not viable for the fragmented code that's changed.

EDIT: I get the impression that a few of the answers/comments so far have picked up on my phrase "broadly impact", and possibly taken that to mean that the change involved a large number of lines of code. I can understand this being the interpretation, but that wasn't really my intention. By "broadly impact", I mean for example the potential for regression is high because of the interconnectedness of the codebase, or the scope of knock-on effects, not necessarily that the change itself is a large one. For example, a developer might find a way to fix a bug with a single line by calling an existing high level routine that cascades calls to many lower level routines. Testing and verifying that the bug fix worked is easy. Manually validating (via code review) the impact of all the knock-on effects is much more difficult.

Answer 1

The premise of the question is, frankly, astounding. We suppose that there is a large change to fragile, complex code, and that there is simply not enough time to review it properly. This is the very last code you should be spending less time on reviewing! This question indicates that you have structural problems not only in your code itself, but in your methodology of managing change.

So how to deal with this situation? Start by not getting into it in the first place:

• Identify sources of complexity, and apply careful, heavily reviewed, correct refactorings to increase the level of abstraction. The code should be understandable by a fresh-out-of-college new employee who knows something about your business domain.

• Identify sources of fragility; this could be by review of the code itself, by examining the history of bug fixes to the code, and so on. Determine which subsystems are fragile and make them more robust. Add debugging logic. Add assertions. Create a slow but obviously correct implementation of the same algorithm and in your debug build, run both and verify that they agree. In your debug build, cause rare situations to occur more frequently. (For example, make a memory allocator that always moves a block on reallocation, or always allocates a block at the end of a page, or whatever.) Make the code robust in the face of changes to its context. Now you don't have fragile code anymore; now you have code that finds the bugs, rather than causes the bugs.

• Write a suite of automated tests. Obviously.

• Don't make broad changes. Make a series of small, targeted changes, each of which can be seen to be correct.

But fundamentally, your scenario is "we have dug ourselves into a hole of technical debt and each complex, unreviewed change digs us deeper; what should we do?". What do you do when you find yourself in that hole? Stop digging. If you have so much debt that you are unable to do basic tasks like reviewing each other's code then you need to stop making more debt and spend time paying it off.

Answer 2

One of the primary goal of a code review is to increase quality and deliver robust code. Robust, because 4 eyes usually spot more problems than 2. And the reviewer who has not written the additional code is more likely to challenge (potentially wrong) assumptions.

Avoiding peer reviews would in your case only contribute to increase fragility of your code. Of course, reinforcing testing with a solid and repeatable test suite could certainly improve the quality. But it should be complementary to peer review, not a replacement.

I think that complexity must be understood and mastered, and the full peer review is the occasion to share knowledge and achieve this. The investment you make in having more people understanding the strength and weakness of the fragile code, will help to make it better over the time.

A quote to conclude:

"If you want to go fast, go alone. If you want to go far, go together"

Answer 3

Welcome to the world of legacy software development.

You have 100s of thousands, millions, 10s of millions of lines of code.

These lines of code are valuable, in that they produce a revenue stream and replacing them is infeasiable.

Your business model is based off of leveraging that code base. So your team is small, the code base is large. Adding features to is required to get people to buy a new version of your code, or to keep existing customers happy.

In a perfect world, your huge code base is unit tested up the wazoo. You don't live in a perfect world.

In a less perfect world, you have the budget to fix your technical debt -- break your code down into unit testable pieces, do exensive integration testing, and iterate.

This, however, is paying down debt without producing new features. Which doesn't match the business case of "reap profits from existing code, while modifying it in order to generate incentive to upgrade".

You could take huge chunks of code and rewrite it using more modern techniques. But everywhere you interact with the existing code you'll be exposing possible break points. That hack in the system that you got rid of actually compensated for a quirk in a subsystem you didn't rewrite. Always.

What you can do is act carefully. You can find some part of the code that you actually understand, and whose behavior and interaction with the rest of the system is well understood. You can modernize that, adding unit tests and making its behavior even clearer.

Then find the parts of the rest of the app that mainly interact with it, and attack them one at a time.

As you do so, you can improve the subsystem, adding features that the customers are willing to pay for.

In short, this is the art of the possible -- making changes without breaking things that provide a business case.

But this isn't your question. Your question is, "I am doing something that is huge, and likely to break stuff, and how do I follow best practices?"

When doing something huge, it is true that if you want to do it reliably you end up spending more effort tracking down bugs and fixing them than you do writing it. This is the general rule of software development: writing stuff is easy, making it work flawlessly is hard.

You probably have a business case hanging over your head, where you have promised to some stakeholder that this massive change goes in. And it is "done", so you get pushback on saying "no, this isn't done, it just looks like it".

If you have the power and the budget, actually spend the effort generating confidence that the change works, or simply reject the change. This is going to be a matter of degree, not kind.

If you don't have that much power, but still have some, try to insist that the new system is unit testable. If you rewrite some subsystem, insist that the new subsystem is composed of small parts with well specified behavior and unit tests around them.

Then there is the worst case. You go deeper into debt. You borrow against the future of the program by having more code that is fragile and more bugs in order to get the feature out now, and damn the consequences. You do sweep-based QA to find the worst issues, and ignore the rest. This is actually sometimes the right answer from the perspective of the business, as it is cheapest now. Going into debt to generate profits is a valid business strategy, especially if clearing the debt via bankruptcy (abandoning the code) is on the table.

A large problem is that rarely are the incentives of the company owners aligned with the decision makers and the programmers. There tends to be lots of pressure to 'deliver', and doing so by generating nearly invisible (to your superiors) technical debt is a great short and sometimes medium-term strategy. Even if your superiors/stakeholders would be best served by not creating all that debt.

Answer 4

Solve the larger problems that are causing code review to be too hard.

The ones that I've spotted so far:

1. No unit test suite
2. Complex code merges that could be avoided by more sensible code structure and delegation of coding duties
3. An apparent lack of rudimentary architecture

Answer 5

1. You can send the code review back and tell the developer to break it up into smaller, more incremental changesets, and submit a smaller code review.

2. You can still check for code smells, patterns and anti-patterns, code formatting standards, SOLID principles, etc. without necessarily going through every detail of the code.

3. You can still perform tactical code inspections for proper input validation, locking/thread management, possible unhandled exceptions, etc. at a detailed level, without necessarily understanding the overall intention of the whole changeset.

4. You can provide an assessment of overall risk areas that you know may be impacted by the code, and ask the developer to confirm that these risk areas have been unit tested (or ask that he write automated unit tests, and submit those for review as well).

Answer 6

In this situation, the amount of time it would take to verify the safety of the changes, absence of regression, etc. is excessive.

Code reviews shouldn't be primarily aimed at correctness. They are here to improve code readability, maintainability and adherence to team standards.

Finding correctness bugs during a code review is a nice byproduct of doing them, but a developer should make sure their code works perfectly (including non regression) before submitting it for review.

Correctness should be built in from the start. If one developer isn't able to achieve it, have them pair program or figure out a plan with the whole team but don't treat it as something you can add as an afterthought.

Answer 7

If you think that the code review is too hard, because it changed brittle code that is near impossible to change without breaking it, then you have a problem. But the problem is not with the code review. The problem is also not with unit tests, because brittle code cannot be unit tested! If your code was unit testable then it would have been split up into small, independent units, that each can be tested, and that work together well, and that's exactly what you don't have!

So you have a heap of rubbish code (aka "technical debt"). The worst thing you can do is starting to fix that heap of rubbish code and not finishing the job because that way you'll get an even bigger heap of rubbish code. So the first thing is to get your management to buy into fixing it and to finish the job. Or you don't. In that case you just don't touch it.

When you fix it, you extract one unit from the code, make it into something that has well-defined and well-documented behaviour, write unit tests for that unit, code review it, and pray that nothing breaks. And then you do the same with the next unit, and so on.

The tricky bit comes when you run into bugs. Your rats nest of code will do the wrong things in some cases because things are so brittle and complicated, things will go wrong. As you extract units, the remaining code will become clearer. (I had a case where after some refactoring, a function started with "if (condition1 && condition2 && condition3) crash ();" which was exactly the behaviour before refactoring, only clearer. I then deleted that line :-) You will see weird and undesired behaviour clearly, so you can fix it. On the other hand, that's where you must change the behaviour of the existing code, so that needs to be done carefully).

Answer 8

Unfortunately, there's not really much you can do about this at the point of code review other than get another cup of coffee. The actual solution for this issue is to address the technical debt you've accumulated: fragile design, lack of tests. Hopefully, you at least have some sort of functional QA. If you don't have that then there's always praying over some chicken bones.

Answer 9

If you're not content to ship with buggy/non-functioning software and fix it later, then V&V effort SHOULD be longer than development effort!

If existing code is fragile, then a first question is "should you even be changing it?" Management need to make a call on whether the cost/risk of redesigning and reimplementing this code is greater than the cost/risk of fixing up the wobbly pile of junk. If it's a one-off, it may be easier to just patch it up. If there are likely to be more changes needed in future, taking the hit now to avoid more pain in future may be a better decision. You need to be raising this with your management, because giving your managers good information is part of your job. They need to be making that decision, because it's a strategic decision which is above your responsibility level.

Answer 10

From my experience, I would strongly recommend you to cover your code with a fair amount of tests, both unit and integration, BEFORE any changes are made to the system in question. It's important to remember that nowadays there's a very good number of tools for that purpose, doesn't matter the language you're developing with.

Also, there's THE tool of all tools for you to create your integration tests. Yes, I'm talking of containers and specially of Docker and Docker Compose. It beautifully provides us with a way of quickly setting up a complex application environment, with infrastructure (database, mongodb, queue servers etc) and applications.

The tools are available, use them! :)

Answer 11

I don't know why it hasn't been mentioned yet, but these 2 are the most important pieces:

• You split up the changelist into multiple smaller changelists, which you then review one after another.*
• If the review of a changelist doesn't result in a decision that the changelist seems to be good, you obviously reject the change.

*Example: You replace library A with library B. One changelist introduces library B, various different changelists replace useage of A with B piece by piece (e.g. one changelist per module), and the last changelist deletes library A.

Answer 12

Do the best one can and try only to spot any obvious flaws (perhaps this is the most code review should aim for anyway)?

Don't underestimate the potential value of code revues. They can be good at detecting bugs:

• Find bugs that would be difficult to detect though testing
• Find bugs that would be difficult to identify/fix though testing

They're also useful for other reasons:

• Help to cross-train members of the team
• Help to ensure that code meets other quality metrics, e.g. help to ensure that it's understandable and maintainable and not just bug-free

What to do in this situation?

In the best/ideal case, passing code inspection doesn't just mean "no obvious bugs": it means "obviously no bugs" (although of course you'd want to test it as well).

If you can't verify the new code-base via code inspection then it will need more-extensive "black box" testing. You might be used to a development cycle where you put code into production after it's passed inspection, but if it can't "pass inspection" then you can't "put it into production" and it needs a longer cycle: e.g. integration tests, system tests, alpha tests, acceptance tests, beta tests, etc.

no comprehensive suite of unit tests available or unit tests not viable for the fragmented code that's changed

What about integration-, system-, and acceptance-tests?

Anyway you should probably tell the project manager and product manager that the code is almost certainly buggy, with an unknown number of bugs; and that they'll "get what they inspect" instead of just getting "what they expect" -- i.e. that the code quality is no better than their testing (because the code quality hasn't been and cannot be guaranteed by code inspection).

They should possibly relay that message to the customer or users, so they do beta testing (if they're willing to be early adopters), or use the older version until the new version is out of beta (if they're not).

Answer 13

Plenty of code is written and merged in without proper code review. It can work. There's a reason why it's called code smell not "broken code" or something to that effect. The lack of code review is a warning sign, not a harbinger of doom.

The solution to this problem is that there is no one solution to fit all cases that we can pack into a StackExchange style answer. It is the strong consensus of the software development community that code review is a crucial "best practice," and in this case it is being skipped. Your development is no longer in that narrow channel of "following all best practices." You will need to find your own way.

What is a "best practice" anyways? When you get right down to it, it's a set of practices which people generally think make code better. Do they make code right? Heck no! The internet is littered with stories from companies who followed "best practices" and got themselves jammed up by it. Perhaps a better viewpoint of "best practices" is that they are the "fire and forget" solutions of the software world. I can know nothing about your company, your project, your team, and be able to quip off "best practices" as things that will help you out. They're the general "do no harm" advice.

You have clearly deviated from this plan. Fortunately, you recognize it. Good job! They say knowledge is half the battle; if so, awareness is well over half of it! Now a solution is needed. From your description, it is clear that the business environment you are in has evolved to a point where the boring advice of "go do the code review, it's best practice" isn't going to cut it. For this, I recommend a key rule that I use when it comes to software best practices:

No software development best practice trumps a business need.

Frankly, they're paying your paycheck, and the business' survival is typically far more important than the quality of the software. We don't like to admit it, but perfectly written software is useless if it is trapped in the body of a company dying from its efforts to sustain that perfectly written software.

So where do you go? Follow the trail of force. You have pointed out that, for some unstated reason, it is unreasonable to undergo a code review for some task. In my experience, that reason is always temporal. It's always either "not enough time" or "not enough money to keep the salaries flowing while you spend the time." This is business; it's okay. If it was easy, everyone would do it. Follow the trail of force upwards, and find the management that is in a position to help you understand why a code review is not an option. Language is hard, and quite often a decree will trickle down from the upper management and get distorted. The solution to your problem may be hidden in that distortion.

The answer to this is, necessarily, a specific case scenario. It's akin to trying to predict whether a coin toss will be heads or tails. The best practices say to flip it 100 times and the expectation will be roughly 50 heads and 50 tails, but you don't have time to flip it 1 time. This is where the details of your situation matter. Did you know that a coin will typically land in the same orientation it was tossed from about 51% of the time? Did you take the time to observe which way the coin was before tossing? It could make a difference.

One general solution that may be available to you is to try to find a way to draw out the code review process and make it a very low cost effort. Much of the cost of a code review process is that everyone is 100% dedicated to the code review while you are doing it. This has to be the case because, once the code review is done, the code is blessed. Perhaps you can put the code in a different branch, and do the code review in parallel with development on the main trunk. Or perhaps you can even set it up so that the software does the testing for you. Maybe you are in a business environment where your customers can be running the "new" code in parallel with the old, and having them compare the results. This turns the customers into a bunch of use case creation devices.

A key to all of these running "maybes" is that you should strive to make your code easily broken up into pieces. You might be able to "prove out" pieces of the code without relying on a formal code review by using them in less mission critical projects. It's easier to do this if the changes are in smaller pieces, even if the sum total of them is too large to peer review.

In general, look for solutions specific to your project, your company, your team. The general purpose answer was "best practices." You aren't using those, so you should look for more custom tailored solutions to this problem, this time. This is business. If everything went the way we expected all the time, IPOs would be a whole lot easier to assign values to, wouldn't they!

If replacing a code review is a struggle, remember that there has never been a single piece of code which was proven to work in a code review.* All a code review does is give you confidence in the code, and an opportunity to make corrections before they become a problem. Both of these valuable products of a code review can be acquired by other means. Code review just has a recognized value for being particularly good at it.

* Well, almost: the L4 microkernel got a code review a while back by an automated proof system which proves its code, if compiled by a conformant C++ compiler, will do exactly what the documentation says.

Answer 14

As @EricLippert points out in his excellent answer, this kind of change needs more attention, not less. If you realise a change you are working on is going to become such a change, there are a few strategies that might help:

• Commit to version control frequently. Review can progress on a commit-by-commit basis, and might be more understandable when you have smaller commits.
• Make sure you comment the reasons for each change as clearly as possible.
• If at all plausible, use pair programming for this kind of change. Having three sets of eyes on the issue rather than 2 can help avoid problems that might be missed normally, and having a pair while you're working can help you improve any comments on code you thought was obvious but which turns out to be less obvious than you believed, which in turn will help the reviewer later. The help in (a) reducing errors during development and (b) improved documentation might actually mean that fewer man hours are spent on this, despite more people being involved.

Answer 15

More answers are addressing how you got to this point. Many of them give some suggestions to remedy the situation, but I'd like to throw my answer in to give the short answer.

What to do when code reviews are "too hard?"

1. Go back to the main line code branch
2. Write tests for the functionality you've refactored (e.g. functional tests)
3. Get the tests to pass
4. Merge the tests into the code that's "hard to test"
5. Do the tests still pass?

Yes

You developers were great! Cats back for everyone!

(or for those who didn't grow up watching "The Simpsons" on U.S. television: If the tests are passing, skip trying to look at the differences and have the developer lead you on a tour of the changes)

No

Keep refactoring and adding test coverage until the tests are passing.

Answer 16

Like a multiplication, the code review gives zero result when applied to zero. It does not increase the value in such case, while in most other cases it would.

The code you need to work with is too badly designed to benefit from the code review process during further development. Use the code review process to refactor or re-develop it.

It may also be that the code is still bearable but the task is not good. It is too broad and should have been done in smaller increments.