What to do when code submitted for code review appears to be too complicated?

Question

The code is difficult to follow but it appears to be (mostly) working well, at least with superficial testing. There might be small bugs here and there but it's very hard to tell by reading the code if they are symptomatic of deeper issues or simple fixes. Verifying overall correctness manually via code review however is very difficult and time-consuming, if it is even possible at all.

What is the best course of action in this situation? Insist on a do-over? Partial do-over? Re-factoring first? Fix the bugs only and accept the technical debt? Do a risk assessment on those options and then decide? Something else?

Answer 1

If it cannot be reviewed, it cannot pass review.

You have to understand that code review isn't for finding bugs. That's what QA is for. Code review is to ensure that future maintenance of the code is possible. If you can't even follow the code now, how can you in six months when you're assigned to do feature enhancements and/or bug fixes? Finding bugs right now is just a side benefit.

If it's too complex, it's violating a ton of SOLID principles. Refactor, refactor, refactor. Break it up into properly named functions which do a lot less, simpler. You can clean it up and your test cases will make sure that it continues to work right. You do have test cases, right? If not, you should start adding them.

Answer 2

Everything that you mention is perfectly valid to point out in a code review.

When I receive a code review, I do review the tests. If the tests don't provide sufficient coverage, that's something to point out. The tests need to be useful to ensure that the code works as intended and will continue to work as intended in changes. In fact, this is one of the first things I look for in a code review. If you haven't proven that your code meets the requirements, I don't want to be investing my time in looking at it.

Once there are sufficient tests for the code, if the code is complex or hard to follow, that's also something that humans should be looking at. Static analysis tools can point out some measures of complexity and flag overly complex methods as well as finding potential flaws in the code (and should be run before a human code review). But code is read and maintained by humans, and needs to be written for maintainability first. Only if there is a reason to use less maintainable code should it be written in that manner. If you need to have complex or unintuitive code, it should be documented (preferably in the code) why the code is that way and have helpful comments for future developers to understand why and what the code is doing.

Ideally, reject code reviews that don't have appropriate tests or have overly complex code without a good reason. There may be business reasons to go forward, and for that you'd need to assess the risks. If you do go forward with technical debt in code, put tickets into your bug tracking system immediately with some details of what needs to change and some suggestions for changing it.

Answer 3

Verifying overall correctness manually via code review however is very difficult and time-consuming, if it is even possible at all.

That isn't remotely the point of a code review. The way to think of a code review is to imagine that there is a bug in the code and you have to fix it. With this mindset, browse through the code (especially comments) and ask yourself "Is it easy to understand the big picture of what is going on so that I can narrow down the problem?" If so, it's a pass. Otherwise, it's a fail. More documentation is needed at the very least, or possibly refactoring is needed to make the code reasonably comprehensible.

It's important to not be perfectionist about it unless you are sure that is what your employer is after. Most code sucks so much that it could easily be refactored 10 times in a row, getting more readable each time. But your employer likely doesn't want to pay to have the world's most readable code.

Answer 4

Verifying overall correctness manually via code review however is very difficult and time-consuming, if it is even possible at all.

Many years ago, it was actually my job to do exactly that by grading students' homework. And while many delivered some reasonable quality with a bug here and there, there were two who stood out. Both always submitted code that had no bugs. One submitted code that I could read from top and bottom at high speed and mark as 100% correct with zero effort. The other submitted code that was one WTF after the other, but somehow managed to avoid any bugs. An absolute pain to mark.

Today the second one would have his code rejected in a code review. If verifying correctness is very difficult and time-consuming, then that is a problem with the code. A decent programmer would figure out how to solve a problem (takes time X) and before giving it to a code review refactor it so that it doesn't just do the job, but obviously does the job. That takes much less than X in time and saves a lot of time in the future. Often by uncovering bugs before they even go to the stage of a code review. Next by making the code review a lot quicker. And all the time in the future by making the code easier to adapt.

Another answer said that some people's code could be refactored 10 times, becoming more readable each time. That's just sad. That's a developer who should look for a different job.

Answer 5

Is this old code that was changed slightly? (100 lines of code changed in a 10000 lines codebase is still a slight change) Sometimes there are time constraints and developers are forced to stay within an old and inconvenient framework, simply because a complete rewrite would take even longer and is way out of budget. +usually there is risk involved, which can cost millions of dollars when incorrectly assessed. If it is old code, in most cases you'll have to live with it. If you don't understand it on your own, talk to them, and listen to what they say, try to understand. Remember, it may be difficult to follow for you, but perfectly fine for other people. Take their side, see it from their end.

Is this new code? Depending on time constraints, you should advocate to refactor as much as possible. Is it okay to spend more time on code reviews if necessary. You shouldn't timebox yourself to 15min, get the idea and move on. If the author spent a week to write something, it's okay to spend 4-8 hours to review it. Your goal here is to help them refactor. You don't just return the code saying "refactor. now". See which methods can be broken down, try to come up with ideas to introduce new classes etc.

Answer 6

Oftentimes, "complicated" patches/changelists are ones that do many different things at once. There's new code, deleted code, refactored code, moved code, expanded tests; it makes it hard to see the big picture.

A common clue clue is that the patch is huge but its description is tiny: "Implement $FOO."

A reasonable way to handle such a patch is to ask that it be broken into a series of smaller, self-contained pieces. Just as the single-responsibility principle says a function should do just one thing, a patch should focus on just one thing as well.

For example, the first patches might contain purely mechanical refactorings that make no functional changes, and then the final patch(es) can focus on the actual implementation and testing of $FOO with fewer distractions and red herrings.

For functionality that requires lots of new code, the new code can often be introduced in testable chunks that don't change the behavior of the product until the last patch in the series actually calls the new code (a flag flip).

As for doing this tactfully, I usually word it as my problem and then ask for the author's help: "I'm having trouble following everything going on in here. Could you break this patch into smaller steps to help me understand how this all fits together?" It's sometimes necessary to make specific suggestions for the smaller steps.

So big patch like "Implement $FOO" turns into a series of patches like:

1. Introduce a new version of Frobnicate that takes a pair of iterators because I'm going to need to call it with sequences other than vector to implement $FOO.
2. Switch all the existing callers of Frobnicate to use the new version.
3. Delete the old Frobnicate.
4. Frobnicate was doing too much. Factor the refrumple step into its own method and add tests for that.
5. Introduce Zerzify, with tests. Not used yet, but I'll need it for $FOO.
6. Implement $FOO in terms of Zerzify and the new Frobnicate.

Note that steps 1-5 make no functional changes to the product. They're trivial to review, including ensuring that you have all the right tests. Even if step 6 is still "complicated," at least it's focused on $FOO. And the log naturally gives you a much better idea of how $FOO was implemented (and why Frobnicate was changed).

Answer 7

As others pointed out, code review is not really designed to find bugs. If you are finding bugs during code review that probably means you do not have enough automated test coverage (e.g. unit/integration tests). If there is not enough coverage to convince me that the code does what it supposed to, I usually ask for more tests and point out the kind of test cases I am looking for and usually not allow code into the codebase that does not have adequate coverage.

If the high level architecture is too complex or does not make sense I usually call a meeting with a couple of the team members to talk about it. It is sometimes hard to iterate on a bad architecture. If the dev was a novice, I usually make sure we go through what their thinking is ahead of time instead of reacting to a bad pull request. This is usually true even with more seasoned developers if the problem does not have an obvious solution that will more than likely be picked.

If the complexity is isolated to the method level that can usually be corrected iteratively and with good automated tests.

One last point. As a reviewer you need to decide whether the complexity of the code is due to essential or accidental complexity. Essential complexity relates to the parts of the software that are legitimately hard to solve. Accidental complexity refers to all the other parts of the code we write that is too complex for no reason and could be easily simplified.

I usually make sure that code with essential complexity is truly that and cannot be further simplified. I also aim for more test coverage and good documentation for these parts. Accidental complexity should almost always be cleaned up during the pull request process because those are the bulk of the code we deal with and can easily cause maintenance nightmare even in the short term.

Answer 8

What are the tests like? They should be clear, simple, and easy to read with ideally only one assert. The tests should clearly document the intended behavior and use cases of the code.

If it's not well tested that's a good place to start reviewing.