119

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?

Improve this question
8
  • 4
    Appears or is? A quick measurement of the source files will confirm or deny your suspicions. After measuring just bring up the measurement numbers at the code review and suggest a refactoring to bring down the complexity numbers.
    – Jon Raynor
    Commented Dec 15, 2016 at 16:51
  • 4
    related: How can I tactfully suggest improvements to others' badly designed code during review?
    – gnat
    Commented Dec 15, 2016 at 17:06
  • 6
    Possible duplicate of What do you do when code review is just too hard?
    – bill
    Commented Dec 15, 2016 at 18:49
  • 5
    Please define "too complicated". Is the code too complicated because it uses well known design patterns that are simply unfamiliar to your team, or because it fails to use patterns well known to your team? The precise reasons for judging the code "too complicated" are essential to creating a correct assessment of how to go forward. A statement as simple as "too complicated" on a knowledge area as deep and complicated as code review suggests a witch hunt of the developer to me.
    – Pieter Geerkens
    Commented Dec 16, 2016 at 20:18
  • 7
    @PieterGeerkens Or, perhaps, is it too complicated because it's solving a complicated problem?
    – Casey
    Commented Dec 16, 2016 at 22:07

8 Answers 8

Reset to default
248

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.

Improve this answer
13
  • 49
    This very much. Remember that its not just you and the writer who will be reading this code. It will also be some random intern in 10 years, so you want to make sure he has a chance of being able to understand whats going on.
    – David says Reinstate Monica
    Commented Dec 15, 2016 at 21:30
  • 2
    good answer. it depends on the purpose of "code review". readability is one thing, structure another - but their very closely related. fwiw I'm working with some open source written by MAJOR corps, and it's almost unreadable because the var and fn names are so hair-brained.
    – user223083
    Commented Dec 15, 2016 at 22:41
  • 19
    @DavidGrinberg For all practical purposes, "you in six months" is a completely different person.
    – chrylis -cautiouslyoptimistic-
    Commented Dec 15, 2016 at 23:40
  • 2
    Put the code away for some time (long enough for him to not remember everything). Ask the original coder to review it. See if HE understands it.
    – Nelson
    Commented Dec 16, 2016 at 4:21
  • 4
    I disagree that code review "isn't" for finding bugs. It often does find bugs, and that is a very powerful and useful aspect of code reviews. Better yet, it helps find ways to avoid bugs entirely in future code. The point is perhaps overstated, and should be that it's not exclusively to find bugs!
    – Cody Gray - on strike
    Commented Dec 16, 2016 at 15:32
45

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.

Improve this answer
30

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.

Improve this answer
3
  • 4
    Great comment! "Most code sucks so much that it could easily be refactored 10 times in a row, getting more readable each time" Boy, have I been guilty of doing that :)
    – Dean Radcliffe
    Commented Dec 16, 2016 at 5:30
  • 1
    "Most code sucks so much that it could easily be refactored 10 times in a row, getting more readable each time." Indeed, that is how it is in the real world.
    – Peter Mortensen
    Commented Dec 17, 2016 at 21:49
  • @PeterMortensen It is indeed true that you find a lot of that in the real world. But it is not in anybody's interest to have code written that way. I think there are two reasons why it is that way. The education that developers receive put very little effort into teaching how to write readable code. And in some businesses it is perceived as a waste of time: "If the developer have already written working code, why should we care if it is readable or not? Just ship the thing."
    – kasperd
    Commented Dec 18, 2016 at 12:56
15

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.

Improve this answer
1
  • It takes much less time for me to refactor my code 10 times, then it takes me to write the first version of the code. If anyone else knows I have done this refactoring I have failed.
    – Ian
    Commented Dec 19, 2016 at 14:45
6

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.

Improve this answer
6
  • 2
    You don't just return the code saying "refactor. now" - why? I received such review comments at least once and last time I remember it turned out helpful and correct. I had to rewrite big chunk of code from the scratch and this was the right thing to do because looking back I saw it myself that old code was an unsalvageable mess. Reviewer was simply enough qualified to notice that (and I apparently wasn't)
    – gnat
    Commented Dec 16, 2016 at 21:20
  • 4
    @gnat: For one, because it's rude. You look better when you explain what's wrong with the code and take effort to help the other person improve it. In a large company doing it otherwise may get you out of the door quickly. Especially if you reviewing a more senior person's code.
    – Victor Zakharov
    Commented Dec 16, 2016 at 21:24
  • that case I referred above, it was in large established company that just happened to be careful enough about not getting out of the door their most qualified developers, at least not on the grounds of directly sharing their technical expertise when asked to
    – gnat
    Commented Dec 16, 2016 at 21:29
  • 1
    @gnat: The approach "refactor. now" may work downwards, i.e. when a senior developer with 10+ years of experience says to refactor to a junior developer who was hired 1 month ago with no experience, or similar situation. Upwards - you may have a problem. Because you may not know how much experience the other developer has, it's safe to assume respect as default behavior. It won't hurt you for sure.
    – Victor Zakharov
    Commented Dec 16, 2016 at 21:39
  • 1
    @Neolisk: An experienced developer who had to write code under time pressure and knows it isn't good enough may be only too happy if you reject the code, giving him time and an excuse to improve it. The PHB deciding it's good enough makes the dev unhappy; the reviewer deciding it's not good enough makes him happy.
    – gnasher729
    Commented Dec 18, 2016 at 14:09
2

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

Improve this answer
1
  • One approach, if using Git, is to compose a pull request of multiple commits. Each commit is as atomic and self-contained as possible, and has its own description. Then, add a helpful note in the PR body that each change can be reviewed manually. That's generally how I handle very large PRs, like global refactors or big, unassailable tooling changes.
    – Jimmy Breck-McKye
    Commented Dec 19, 2016 at 10:35
1

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.

Improve this answer
0

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.

Improve this answer

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