Code reviews aren’t just for catching bugs • Hacker News Board

Re: Code reviews aren’t just for catching bugs

#81

Post by chris_va » Thu, May 12, 2016, 9:34 PM UTC

millstone wrote: post #77

chris_va wrote: post #17

Earlier quoted context omitted.

Why do you find them expensive? I've done thousands in my career. They don't take much time compared to actually writing the code, and adding an extra 5% of engineering time pays major dividends later without drastically reducing throughput.

Code review makes you vulnerable to priority inversion. I need this change in now, to meet some deadline, but the reviewer has other priorities. Reviews pile up. When this happens, a common response is to go work on something else, but this only exacerbates the problem: now you have a bunch of commits awaiting review which are dependent in various ways. If I merge this commit I have to go and fix up that one, and tha…

This can happen if code reviews are not done well, I agree.

Things can be just as bad if code is not written well.

Doing either well requires good engineering practices and skill. It is part of the profession that you acquire over time, like anything else that can improve development.

Now, sometimes you are at the mercy of a system not filled with really experienced engineers. In such a system, though, I think skipping code reviews for expediency will probably be even worse in the long run.

Re: Code reviews aren’t just for catching bugs

#82

Post by aczerepinski » Thu, May 12, 2016, 9:41 PM UTC

I'm a dev with one year's experience, and recently moved to a team that reviews every PR. The benefits to me personally are super clear. #1, more experienced engineers are giving me frequent feedback, and that's obviously worth a ton. #2, it's part of my job to read other developers' code. I get exposed to patterns and design choices that I may not have in my repertoire.

Maybe the feedback I give the other direction isn't yet as valuable as what I receive on my PRs, but I trust that eventually it will be.

I totally get the posts pointing out that during crunch time, folks do pretend reviews, and the process becomes busy work. I think that's a symptom of other problems though (staffing model, etc), and not necessarily a shortcoming of peer review in general.

Re: Code reviews aren’t just for catching bugs

#83

Post by gurkendoktor » Thu, May 12, 2016, 9:43 PM UTC

frandroid wrote: post #55

ben_jones wrote: post #32

Earlier quoted context omitted.

I think it will always depend on the team. Process X might be a savior to team Y, but completely screw team A even if team A == team Y (javascript truthiness here). On the flip side I think you can also say the potential downsides of excessive code review are significantly less then the potential downsides of zero code review. I dunno.

> the potential downsides of excessive code review What would those be?

Large commits getting stuck in review forever, or until they have too many conflicts to be merged cleanly; small commits being over-reviewed for trivial issues because the reviewer wants to prove that they have actually looked at the code.

Re: Code reviews aren’t just for catching bugs

#84

Post by jquast » Thu, May 12, 2016, 9:43 PM UTC

zhemao wrote: post #4

I think a benefit not mentioned in the article is that it makes sure that at least one person besides the original author understands what the code does.

absolutely agree -- situational awareness of changes in the codebase, this is the number one priority. Even if a reviewer does not understand much of it, when shit happens after merge, they know why. A new issue might be related "to that thing I read in so-and-so's review the other day"

Re: Code reviews aren’t just for catching bugs

#85

Post by millstone » Thu, May 12, 2016, 9:51 PM UTC

chris_va wrote: post #81

millstone wrote: post #77

Earlier quoted context omitted.

Code review makes you vulnerable to priority inversion. I need this change in now, to meet some deadline, but the reviewer has other priorities. Reviews pile up. When this happens, a common response is to go work on something else, but this only exacerbates the problem: now you have a bunch of commits awaiting review which are dependent in various ways. If I merge this commit I have to go and fix up that one, and tha…

This can happen if code reviews are not done well, I agree. Things can be just as bad if code is not written well. Doing either well requires good engineering practices and skill. It is part of the profession that you acquire over time, like anything else that can improve development. Now, sometimes you are at the mercy of a system not filled with really experienced engineers. In such a system, though, I think skippi…

I was speaking more about the team organization. Code reviews increase in cost as the team becomes more heterogenous and more distributed. If your team consists of a kernel engineer in Paris and an Angular developer in San Francisco, code reviews will be high cost and low value no matter how experienced the devs are.

I agree with you, even in the ideal case of a co-located homogeneous team, it's possible to screw up the process.

Re: Code reviews aren’t just for catching bugs

#86

Post by gurkendoktor » Thu, May 12, 2016, 9:52 PM UTC

zhemao wrote: post #4

I think a benefit not mentioned in the article is that it makes sure that at least one person besides the original author understands what the code does.

It all depends on the reviewers, some will spend expensive hours on finding minor code style issues and not understand what actually happened in the commit.

Re: Code reviews aren’t just for catching bugs

#87

Post by wandernotlost » Thu, May 12, 2016, 10:02 PM UTC

While I value many of the same things as the author, I've found code reviews to be far inferior in every respect to pairing (especially promiscuous pairing (google it)), and to have negative effects in several important ways:

  * They delay integration.
  * They tend to encourage focus on abstract code polishing without proportionality or relation to the value of the code.
  * They favor superficial improvements, while increasing the costs sunk into paths that may be deeply flawed. (They facilitate late code-structural feedback, but not early directional/problem-analytical feedback.)
  * They often discourage more collaborative work and therefore quicker and richer feedback, by their presence as a substitute for pairing.
  * They create impediments to work moving quickly to completion.

Of these, the most egregious is the distraction from value. Teams that are spending a lot of time and energy talking about code quality (which is absolutely important, but not primary...the best teams I've seen maintain a high level of quality and talk about tradeoffs involved in delivering value at high quality) are often neglecting communication about where value lies and how to deliver it most efficiently.

[edited for formatting]

Re: Code reviews aren’t just for catching bugs

#88

Post by ArkyBeagle » Thu, May 12, 2016, 11:04 PM UTC

wellpast wrote: post #11

What often goes unmentioned in praise for code review processes is their insanely exorbitant costs -- measured in engineer hours but perhaps more costly is all of the blocking and impedance [1]. Most of the "problems" that code reviews claim to address can be solved by much more direct and optimal measures. Code reviews are damn expensive. This post concedes that code reviews are better for the more fluffy ends -- te…

1) Commits don't have to be blocked. The action items from a review can be a separate commit.

2) Reviews can be done privately, with comments entered electronically. There doesn't have to be a four-day meeting.

2a) Organizations that use four-day meetings are likely full of people who need something to charge that time to. It may be worth delegating a representative from the engineering team to attend those.

3) Action items must be triaged. Ideally, each action item would be assigned a number and numbers grouped together as a statement of work for a commit. This is much less painful than it sounds. Test provenance would also be nice per commit.

Re: Code reviews aren’t just for catching bugs

#89

Post by ArkyBeagle » Thu, May 12, 2016, 11:05 PM UTC

fishnchips wrote: post #5

buro9 wrote: post #2

I'd still rather earlier code reviews... design reviews about 1/3 of the way into writing the code. Enough time to have passed to have discovered the dragons in the whiteboard design, but not enough to have written code that could only undergo minor fixes in a code review. I prefer the idea of a code review happening at a time that it could still steer the ship... too many code reviews catch bugs, but don't correct p…

This is the role of design documents which get heavily reviewed by the team before even the first line of code is written.

Well, except for the elephant-in-the-room of the dread lord Requirements Traceability, which will frequently eat the entire design cycle.

Re: Code reviews aren’t just for catching bugs

#90

Post by mirekrusin » Fri, May 13, 2016, 12:09 AM UTC

In small/medium teams, especially remote ones, code review helps you keep understanding of the system - you already know what your code does and it's great way to learn the rest of the system.

In some sense, it can be looked at, as asynchronous pair programming.