144

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.

  • 91
    What about running your test suite to make sure you didn't break anything? – Vincent Savard Jun 28 '16 at 19:32
  • 130
    what if there is no pre-existing test suite? -- How about writing one? – Robert Harvey Jun 28 '16 at 20:07
  • 27
    The test suite would definitively help. But the peer review and tests are complementary. I think it's not a good idea to replace one by the other. – Christophe Jun 28 '16 at 20:18
  • 8
    @MasonWheeler: Probably a conversation for another time, and you're referring to TDD specifically in that article, using assumptions that I don't think any self-respecting TDD'er would ever make, but I've done it both ways, and I consider the benefits of unit testing to be self-evident. – Robert Harvey Jun 28 '16 at 20:24
  • 21
    Merge and hope nothing slips through? That's a notoriously bad idea. – Mast Jun 29 '16 at 12:08

16 Answers 16

306

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.

  • 74
    From what I've seen in the industry "Stopping Digging" is usually followed by swift termination which is followed by finding someone that is willing to use the shovel. This answer should add a disclaimer that lowly code-monkey peons shouldn't attempt this without being prepared for the consequences... – Luke A. Leber Jun 30 '16 at 0:59
  • 63
    @Luke if management or senior devs are so set on ploughing forwards despite the problems, and will even think about terminating anyone who tries to bring some sanity to this situation (OK, blatant insubordination aside), the company is on an irreversible death march. Leave them to it. – Julia Hayward Jun 30 '16 at 9:08
  • 14
    @JuliaHayward You are correct, but still, the situation Luke describes is common, especially on code that's already generating revenue. Really it's up to you whether it's worth it to keep working on it. – Owen Jun 30 '16 at 19:12
  • 19
    @LukeA.Leber You are correct. I've worked for these companies. What I can tell you is that death march will take years to complete, but every month will get progressively worse. The 'Code Monkeys' will be more miserable every month, but it will take years for the bad managers to realize the consequences of their actions... if ever. – JS. Jun 30 '16 at 21:46
  • 10
    @Matt: The assumption of the question is that someone cares enough about code quality to have a formal system of code review in place, and that the person asking the question is concerned about the impact of large changes on the quality of the code. If we posit instead that no one cares about code quality then sure, my answer about ways to ensure code quality does not apply, but that's not the question that was asked! – Eric Lippert Jul 1 '16 at 14:38
96

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"

  • 5
    Indeed, imagine if 'complex' was replaced with 'long' or 'poorly styled' or 'poorly documented' or any other negative feature we'd say "That's not a good reason to review - let's fix those problems so it is reviewable!" and this is no different. – corsiKa Jun 29 '16 at 9:22
  • 11
    I'd also add that if the code can't be reviewed right now, it can't be maintained 6 months from now..... – corsiKa Jun 29 '16 at 9:23
  • 3
    @corsiKa Why wait 6 months for it to be unmaintainable? – krillgar Jun 29 '16 at 11:59
  • 2
    @krillgar It umm... doesn't... that's just a number I plucked off the top of my head to represent a period of time between when you set down the code and have to pick it up again... so, yeah... – corsiKa Jun 29 '16 at 15:43
  • 16
    @krillgar: I write some "new code", I check it in, I go get lunch and when I get back, my "new code" has magically turned into "legacy code". How'd that happen? :) – Eric Lippert Jun 29 '16 at 17:17
35

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.

  • 3
    I've experienced most of the above so many times it's depressing. I mixture of shoddy programming practices, moving goalposts and unsympathetic management deadlines means that what we all know should happen, and what really happens, are two very different things – Ben Hillier Jul 1 '16 at 8:48
  • 4
    This is a good answer, because while many of the others are technical more correct - this takes offset in the real world and while we all want to live in a world where everything is well tested, well documented - we don't. Legacy code, strange implementations, misunderstandings, unreasonable stakeholders, bad weather ..... life will throw bad stuff at you and you'll have to deal with it. – Allan S. Hansen Jul 4 '16 at 6:12
25

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
15
  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).

14

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.

  • 2
    Agreed, but: Code reviews actually have a 0th purpose, which is even more important than code readability, maintainability, etc. They are for educating the team on what the team's standards are. Even if no edits were performed as a result of code review, they would still have fulfilled 75% of their purpose, because the review would educate the code author to avoid making those same type of mistakes again, repeatedly, throughout the lengthy future lifetime of this project, and the next... – Jonathan Hartley Jul 1 '16 at 16:03
  • 1
    It can certainly play that role too, but I've found pair programming to be more efficient than CRs for onboarding and early to mid term education of new team members. Think about a coach that sits besides you all along an exercise vs a teacher that only does post facto evaluation. Having your "finished" work corrected by someone is more frustrating and less educative than work done collaboratively with someone, in my experience. – guillaume31 Jul 1 '16 at 17:05
  • 2
    @JonathanHartley: In that case, the (minus first) reason for a code review is to make developers write code that they are not ashamed to show to someone else in a code review :-) – gnasher729 Jul 5 '16 at 23:04
  • Agreed absolutely with both guillaume31 & gnasher729 above. – Jonathan Hartley Jul 6 '16 at 12:30
11

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

  • 3
    The hard part is explaining to the business that, "Yes, we will introduce some bugs, but we'll fix them and fix them quickly. A little patience now will get you new features and bug fixes faster in the future." – RubberDuck Jun 30 '16 at 12:06
3

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.

3

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.

1

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

1

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.

1

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

0

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.

  • 2
    Your answer suggests that the 'automated proof system' reviewed L4's source code automatically. Actually it reviewed a human-written proof of L4's correctness. The proof took years to complete. Nonetheless there is a lot to learn from this endeavour about how to write correct code. (To be clear, this isn't a pen-and-paper proof but a machine-readable proof that actually 'imports' the entire source code and reasons about it. See ssrg.nicta.com.au/publications/nictaabstracts/3783.pdf) – Artelius Jul 5 '16 at 5:13
0

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.
0

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.

-1

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.

  • 2
    @Downvoter, the code review is not a replacement for the bad design, and attempts to apply it anyway usually result in changes never being approved, because the reviewer does not understand these junky changes in the junk. Sorry to ruin your vision. – h22 Jul 1 '16 at 19:01

protected by maple_shaft Jun 29 '16 at 13:56

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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