5 ms·

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 abili

• imoverclocked 6y ago
  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.
  • legerdemain 6y ago
    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.
    • fiddlerwoaroof 6y ago
      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)
    • imoverclocked 6y ago
      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 https://arxiv.org/pdf/1907.02127.pdf
      • imoverclocked 6y ago
• ufmace 6y ago
  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.
  • legerdemain 6y ago
    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.
  • BobbyJo 6y ago
    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.
    • JKCalhoun 6y ago
      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.
• dragonwriter 6y ago
  > 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.)
• bcanzanella 6y ago
  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/ 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.
• rmk 6y ago
  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!)
  • catlifeonmars 6y ago
    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.
• pydave 6y ago
  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?
  • legerdemain 6y ago
    When a sample is large enough, it becomes representative of the whole and you can start drawing inferences from it.
  • itsdrewmiller 6y ago
    Code review approaches are discussed in this book: https://www.amazon.com/Making-Software-Really-Works-Believe/dp/0596808321 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).
• eikenberry 6y ago
  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.
  • jrib 6y ago
    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.
• datagram 6y ago
  > 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?
  • elwell 6y ago
    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.