Hacker News Reader

Comment by legerdemain | Hacker News Reader

Almost all code review fails right out the gate at hurdle #1, "is the code correct." When you're looking at an interleaved diff on Github, you have little ability to see whether the code is broken, especially when the diffing heuristic awkwardly mashes together unrelated functions and declarations.

Over a decade of experience with merging enthusiastically approved, hopelessly broken code, I've come to believe that code review won't, and often can't, and isn't really designed to find bugs, only nits. And this pertains to both local bugs (this function is wrong) and systemic bugs (this function makes false assumptions about surrounding code).

What visual code review can uncover:

- style nits

- a lack of unit tests

- feelings of dread when the diff touches a critical part of the codebase

What actually uncovers bugs:

- thorough tests

- in-person code walkthroughs

- design documents

View on HN

I'm currently at a place with a culture of thorough code reviews. IMO, it entirely depends on the people and the culture. It's on the reviewer to actually thoroughly review the code and ensure it does what it's supposed to do, including local checkout and testing if needed. And of course on the management structure to provide senior enough reviewers with enough time to do that. It certainly is easier and less time-consuming to just skim it and comment on some style nitpicks and unit tests, but there's no reason it has to be.

The flip side of course is that the original coder does still need to be reasonably competent. Relying on even exhaustively thorough and repeated code reviews to turn basically garbage into reasonable quality code is a bad bet.

At the moderately large, very successful companies where I've worked, code is mostly disposable. It's written and read, on average, once. Code craftsmanship doesn't make any sense in these environments. We treat code review as a formality. Like, we just kind of skim the diff and click the approve button, but most of the time none of us really have the time to understand the code on a deep level.

Correctness is one competent dev banging on the code until it works, and then getting git-blames and bug reports sent directly to them when it stops working. We don't blame the reviewer for our own bugs. If the dev leaves, it's faster for the next dev to paper over the bug or rewrite the feature than it is to do a close reading of the existing codebase. That's what "rapid iteration" and "agile development" is all about.

I am vehemently against making reviewers checkout and test code. You should have tests that demonstrate correctness with very high confidence, and a release process that can find bugs early.

Expecting reviewers to checkout and run/test things is such a time sink, and over the course of a year of doing it at a new work place, I've never seen it be the thing that caught a bug. If anything I feel like it caused more bugs because people end up getting laxed with tests.

It's as though code reviews are a waste of time.

I have to say, now over 30 years as a developer, I've only been introduced to code reviews in the last few years.

Somehow, I have no idea how, we shipped code for nearly 30 years without code reviews or unit tests.

Thank god we now no longer ship with bugs.

Although I think you’re being sarcastic, I’m sort of in the same camp. We started doing code reviews a few years ago after years of informal releases, and I found they acted like a sieve; I reckon we halved our deployed bug count, at reasonably low cost, but also code style and knowledge improved across the board.

Code reviews are not a silver bullet, and we still released some whoppers, but halving the deployed bug count and improving the code base is a worthwhile outcome.

We’ve recently started doing CD with E2E tests and I reckon E2E has halved the deployed bug count again. It’s definitely stopped bugs from entering production.

There always seems to be a trade off between the cost of action (in resources, time to release, flexibility) and quality, there is no right answer (despite what people will tell you), and it’s up to you to set the acceptable bar for released bugs, and to deal with the consequences.

For me, code reviews improved bug counts less than I’d expected but had other quality benefits that I felt more than made up for the modest cost of the additional ceremony.

I think you have a point about spreading knowledge. Rather than code review though, I think having engineers pair up on a feature is another (better?)way to knowledge share that probably is more efficient time-wise?

I prefer pairing up. Giving feedback on code that has already been written, and by more junior member of the team, is fraught with problems. People get defensive thinking they have already completed the task and now I am delaying the closure.

It is much easier to share friendly hints and observations while writing the code. When the code is written it is difficult to show how it could have looked or demonstrate the process of getting there.

And so even though my org requires us to conduct code reviews (and I find them helpful, at a hefty cost), I will still ask people to pair up with me regularly.

This to get to know the person better, to see what the problem is (and sometimes I am the problem), to help work out better design, etc. All of which is difficult with regular review.

For a while I worked on a team where we wrote all code in pairs. The team room had 4 computers, each of which had 2 pairs of 2 monitors and 2 mice and 2 keyboards. During the day, every hour or so, each dev would switch between driver and passenger roles.

It was a great way to work, because junior people could get up to speed quickly, and it was much less likely that would would get stuck on any problem. As a bonus, we didn't need code reviews because every line of code had already been seen (and if necessary, critiqued) by another person.

One drawback was that you could not start working until your partner for the day arrived, and you'd have to stop working when the first person of a pair left for the day.

I'm not sure how this way of working pays off during the annual performance reviews, I didn't stick around long enough to find out.

I am all for pairing up and interacting with people directly and in a human manner. It's good for building rapport and trust so that things run smoother in every facet.

That being said, I think impersonal reviews are still needed. You don't always have time for the soft human touch, especially at a smaller startup. The defensiveness some people have should really only ever be a short term problem. I've worked with people like that and found that being persistent, objective (follow standards that you make everyone aware of) and consistent with reviews fixed that over time. Rapport building is good , but you can't require it every time, so you need the other end of the spectrum too.

Rich Hickey nails this hilarious in his (amazing) "simple made easy" talk from 2011 where he calls this mentality guard rail driven development: we don't drive down the highway careening back and forth off the guard rails as we drive down the road, thinking "thank goodness these rails are here or i wouldn't be able to drive down this highway"-- why do we think this about our unit tests?

im hardly doing the bit justice because it's truly an all-timer-- highly recommend the whole thing, beyond just that bit of it (which i think comes early on iirc): https://www.youtube.com/watch?v=kGlVcSMgtV4

This boggles my mind. I checkout and manually test every PR that I review, and the vast majority of the time (> 80%) I do find a bug. In fact, I'm very suspicious whenever my PR's get the green checkmark without any other feedback.

If 80% of the code you're sent to review has bugs, your upstream process has to be horribly broken.

Or the PRs are just reasonable large with 1-25 bugs per 1000 loc.

https://stackoverflow.com/questions/2898571/basis-for-claim-...

> Expecting reviewers to checkout and run/test things is such a time sink

I agree. Is everybody writing and running their own bespoke testing on each PR, with no cross-person commonality, in which case why is your code so complicated that they have to do this? Or is everybody doing the same kind of thing every time, in which case why don't you automate that and run it in a CI process?

To be clear, there definitely should be unit tests run via a CI system. That's a minimum bar instead of a cure-all though. What checks that there are enough unit tests to cover appropriate cases, but not so many as to be noisy or excessively time-consuming? What checks that the tests actually assert the correct behavior? Code review does. What do you do when your test suite takes multiple hours to run, but still doesn't catch all of the bugs?

IMO, you can only really apply one ideological principle to your practices. I think that should be that your code works correctly while being no more complex than needed. Ideally many things shouldn't need to be done, but in the end, you do what you need to do to make sure that's true, including reviewers checking out code and working with it. If your ideological principle is instead that reviewers must never checkout code and try it, then you will eventually sacrifice the other in favor of it. I don't think that's a good result.

This is very much on the money. Code reviews often fall into the trap of people discussing and dissecting things that they are comfortable with, while ignoring the difficult aspects. For instance, reviewers often dissect the elegance and variable naming to death while totally ignoring how the change has been tested (unit tests can show some issues, but if the tests themselves are not covering the core aspects of the problem being solved, then they are not hugely useful, particularly in duck-typed languages like python).

Also, changesets that are large but can't be broken down into smaller ones because of practical constraints cause 'code review fatigue': people will review the first few commits/files with gusto and lose steam by the time they are reading the critical code which may arrive in a later changeset or file!)

On my current team, we use code reviews as more of an RFC mechanism. There’s a lot of trust in placed in PR authors to decide, based on delivery constraints, whether comments raised on a review are blocking or not. For large diffs that aren’t practical to review line by line, we tend to use narrative-based architecture/design reviews instead.

Yeah context plays a big part here. We wanted to make the experience of looking at "interleaved diff[s] on Github" a lot better. So we made https://www.codestream.com/ which lets reviewers look at pull requests (github) and soon merge requests (gitlab) right in their editor, leveraging the entire context of the file and all the things they love about their IDEs.

Why do you think in-person code walkthroughs are effective but tool-mediated reviews are not?

Most of my code review comments are along the lines of "what does this do" -- the primary question I'm asking in an in-person review. My goal for a review is to pre-emptively answer the questions I'd ask if I had to fix bugs in the code. This frequently exposes bugs because it requires the author to consider their code from a different perspective.

Maybe it's the code under review: I work in games and was reviewing a lot of code from juniors, so I'd often be asking questions intended to get them thinking about their technical design and we had very little rigor.

In my eyes, the biggest upside of in-person is how it reduces mental drain from back-and-forth. A conversation is comfortable instead of the digital equivalent of repeated red ink all over your work.

> Over a decade of experience with merging enthusiastically approved, hopelessly broken code

Or maybe you're commenting on the code reviewers you've worked with rather than your own experience as a reviewer?

When a sample is large enough, it becomes representative of the whole and you can start drawing inferences from it.

Code review approaches are discussed in this book:

https://www.amazon.com/Making-Software-Really-Works-Believe/...

IIRC the evidence was that formal code reviews were no more effective than async peer review (but both were effective at uncovering bugs).

I always thought the primary value of code reviews was informational, not bug finding. That is by reviewing code you have a better understanding of the changes that went into it, give you a chance to ask why something was done and will generally lead to better future interactions with it.

it's often a good opportunity to point out things that aren't clear when first seeing the new code. Like:

* naming this variable `accumulator` instead of `x` more clearly communicates intentn * add a quick comment explaining the reason this workaround exists

etc.

A major focus of code review for me is the changes are maintainable in the sense that they empower people to make changes in the future effectively.

I can agree with the post though that correctness and security are more important technically.

> I've come to believe that code review won't, and often can't, and isn't really designed to find bugs

> What actually uncovers bugs: > - thorough tests

How do you know that the tests don't have bugs?

> - in-person code walkthroughs

How is this different from reading the code in a code review, or checking it out on your own machine and reading it there?

Yeah, I would argue that in-person code walkthroughs are worse than reviewing someone's code in isolation. Once it's merged in, the author isn't going to be there to explain what the code is doing here and there.

> Over a decade of experience with merging enthusiastically approved, hopelessly broken code, I've come to believe that code review won't, and often can't, and isn't really designed to find bugs, only nits.

Nits are, both literally and figuratively, bugs that just haven't hatched yet.

(Relevantly, the things you’ve pointed out that code review can identify are all issues that impact maintenance of the code, including the ability to find bugs and avoid creating them.)

Thorough tests can also be in the form of good static analysis tools. eg: in Java codebases, errorprone+NullAway can block some really obvious issues leading to NPEs.

In my experience, the issue is often getting the codebase to a point where the tools are useful. The simpler (more nit-picky) rules in such system are usually easier to enforce while the more useful ones build on the simpler rules and flag design issues.

NPEs are a red herring. My immediate read on people who complain about NPEs is that they haven't done any enterprise Java development.

Let's say you have the potential for some reference to be null. And you cleverly wrap it in an Optional or add some @NotNull annotation to it. Great, now how do you handle the null? Maybe it's some query result set that unexpectedly contains zero results or an empty list of live service shards. What do you do to recover? Print an error to the application log and continue running in a corrupted state? Is that really an improvement?

Bugs don't get fixed by adding Optional everywhere or "null coalescing" or whatever, they get fixed by actually understanding how the bug arises during operation. It's a bug in the mental model of how the application works, not at the syntactic level of null references.

I agree, but I think the benefit of things like Optional is a bit different: it’s not really about eliminating NPE-type bugs, but limiting the scope in which they can occur. If you use them consistently, you only have to check the behavior of places where the optional is unwrapped, and you push those places upwards towards the entry-point rather than weaving null-checks into your business logic.

All that being said, the complaints about NPEs have always felt to me like focus on symptoms (null references) rather than the underlying causes (badly-designed application code)

Yup, NPEs are not caused in the location that they occur. They are caused by the part of the code that inserts a null where it doesn't belong/isn't expected. This means that getting rid of NPEs requires detailed enforcement across module/method/interface/etc boundaries.

If something is labelled @Nonnull (or @NotNull, depending on the annotations used) then it should never be allowed to be null. The issue arises when there is no enforcement of this paradigm at the compiler level. Similar languages (Kotlin/Swift/...) with the benefit of Java in hindsight, can track nullability as a core feature. Java requires a plugin to track+enforce nullability for backward compatibility reasons.

errorprone is a Java compiler plugin to add extra sanity checks that the base compiler doesn't/can't provide. NullAway is an errorprone plugin and has been shown [1] to get rid of NPEs.

In my experience, anyone who thinks NPEs are unavoidable or even a wanted feature has not sufficiently learned about nullability as a language/type system feature for type systems that allow null values.

NullAway isn't the only game in town (for Java) but it's the simplest and easiest model to adopt for a large body of developers that don't necessarily want to think hard about null values and complex type interactions.

Finally, I'm speaking from first hand experience of wrangling a large Java codebase with many developers. The mental model of the code is far too complex to hold in any one head. Even just the mental model of nullability is too complex to hold in a single head. The good news is that this is the kind of problem a compiler can excel at... and that a PR check can leverage.

[1] https://arxiv.org/pdf/1907.02127.pdf

One quick clarification, NullAway inverts the check to enforce, "if something can be null, it needs to be labelled @Nullable" ... it doesn't care at all about @Nonnull annotations. This means that potentially null values are traced accurately through your codebase with @Nullable annotations.

Thanks, this narrative makes sense to me! I'm a little sad never to have worked on a commercial Java codebase that showed this degree of discipline, though.

Reply on news.ycombinator.com