Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Code reviews aren’t just for catching bugs

blog.fullstory.com

Post reply on HN
Print view View original

91–100 of 150 posts

Re: Code reviews aren’t just for catching bugs

#91

Post by thrownaway2424 »

wellpast wrote: post #29
eduren wrote: post #18

Earlier quoted context omitted.

>This post concedes that code reviews are better for the more fluffy ends -- teamwork, openness, social recognition, but given their high costs, I'd rather achieve even these soft goals in other ways than to impede my team's delivery potential. What techniques have you found effective for improving the soft goals in the context of software (genuinely curious)? Are we talking more conventional management/business conc…

Not sure how to formalize but the team's I've worked on have generally negotiated mutual respect, openness, and teamwork by collaborating on the things of greater import -- the architecture, domain conceptualization, etc. And peer code reviews in some cases tend to work against these goals--because you tend to be down in the weeds of LOC, bike-shedding, arguing over the equivalents of tabbing and spacing or whether a…

Well perhaps another part of Google's code review culture is important, then: proposed changes must be style compliant, must compile, must have tests, and all the tests must pass before anyone will bother looking at them. If I got a code review (at Google) that was incorrectly indented, there would be a little red chip in the review UI that indicated such style violation, and I would just reply "Please fix" and not look at it again until the next revision.

In the absence of a strong style guide and testing culture then I agree code reviews can get bogged down in bikeshedding. At Google that doesn't seem to happen because the style guide for each language is quite prescriptive. For Go, no change will be reviewed that hasn't passed through gofmt, and so forth. And on the flip side you can have confidence that if your change has gone through gofmt, there's not going to be any discussion about the formatting.

Re: Code reviews aren’t just for catching bugs

#92

Post by thrownaway2424 »

msoad wrote: post #20

Code reviews can easily become a tool for people with huge egos to prove their smartness. I get code review comments for grammar of my comments or very small code style preferences that Google's anal style guide can't enforce (yet). I like code reviews, don't get me wrong. But there should be a way to respond with "you just shut up, you're only trying to make yourself look smart". It's all because higher up people mo…

I always complain about code comments that aren't complete sentences. Why should comments be misspelled or grammatically incorrect?

Re: Code reviews aren’t just for catching bugs

#93

Post by thedufer »

millstone wrote: post #71
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.

Unfortunately code reviews don't ensure this. Reviewers, when faced with code in a domain they don't understand, don't go out of their way to learn it; instead they just look for surface-level / nitpicky stuff.

That's not a problem with code reviews; it's a problem with your company's implementation. "Please explain this" should be considered a very reasonable response to a code change the reviewer doesn't understand. In my experience, this ends in the code being cleaned up with readability in mind and the reviewer learning something new in more or less equal proportions.

Re: Code reviews aren’t just for catching bugs

#94

Post by phasmantistes »

geophile wrote: post #80
devishard wrote: post #54

Earlier quoted context omitted.

> Another huge cost of code reviews is distraction. We've all seen the Paul Graham essay on maker's schedules vs. manager's schedules. We've all read the statistics on how much time is lost to interruptions. Code reviews are a massive interruption, done on a manager's schedule. Each code review is a distraction, and can take a significant time commitment, if it is to be a meaningful review. Wait why are we doing code…

What? And keep the other developer waiting?

Code reviews should be asynchronous. Everyone on your team should be able to be working on multiple small changes/commits/whatever in parallel. While one is out for review, they're working on the others. Different people keep different schedules: I do all my reviews first thing in the morning, to settle in, and then often do another round after lunch. Other people do them at the end of the day, or don't mind the interruptions (e.g. follow a pomodoro schedule anyway) and do them as they come in.

Re: Code reviews aren’t just for catching bugs

#95

Post by neandrake »

maxxxxx wrote: post #62

How do people handle reviews of highly specialized stuff? We have people who do stuff nobody else on the team understands or at least it would take them a long time of learning to do a real review. I look at a lot of stuff and check if it makes halfways sense. I can look at the coding style but I can't judge the overall design without spending many hours on it (which I don't have. Nobody else on the team has it eithe…

Where I work the same situation often comes up -- a developer may spend weeks doing research for a specialized function building prototypes, etc.

Ensure the specialized developer is doing due-diligence in defining and verifying the module works as intended and have others analyze its interactions in the larger system to prevent cascading failure, conforms to application norms, __is documented__, etc. Even if most others wouldn't understand the internals during review you will be taking steps to keep the risk localized. If the bus arrives and you lose the specialized developer then it's reasonable to assume another developer would need to spend nearly as much time to catch up on the research.

Otherwise you're responsible for making the decision to invest more developer time now in understanding the problem/implementation vs. later - a gamble at how much time until that developer leaves.

Taking over specialized/legacy code is part of the job. It can be painful but there are steps you can take to minimize that pain later.

Re: Code reviews aren’t just for catching bugs

#96

Post by harwoodleon »

What about the stress of continuous evaluation? How does the company view that? Standards are great to maintain, but people aren't robots. I question the value of the word 'review' in this context and its blocking nature.

Pair programming is a far more effective tool, but it's not always practical. How about using the terminology of 'collaboration' instead of the test based culture - which I feel turns people into machines - it's just the wrong control structure for people to be happy.

Re: Code reviews aren’t just for catching bugs

#97

Post by simplify »

wellpast wrote: post #49
CorvusCrypto wrote: post #35

Earlier quoted context omitted.

I feel like this can be argued so many ways. For instance, while code review might block some changes from being introduced into the system, it also does have more benefits than the "fluffy" ones you mentioned. Maybe an engineer that has more experience with the codebase can point out a way of doing something that is more in line with what the team's style is (earlier move toward standardized code vs letting people d…

So the problem you're talking about is how to properly manage (and coach) less experienced devs or devs who write messy code. While code reviews can tangentially deal with this, I think there are much more direct approaches to these problems than an umbrella code review policy.

What direct approaches do you have in mind?

Re: Code reviews aren’t just for catching bugs

#98

Post by jupp0r »

gurkendoktor wrote: post #83
frandroid wrote: post #55

Earlier quoted context omitted.

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

You can also refuse changes for being too big to review. Creating incentives for small changes seem like a good thing to me, so I don't see where you complaint is.

Re: Code reviews aren’t just for catching bugs

#99

Post by jupp0r »

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…

If anyone is ever blocked, you are doing it wrong. When performed on git branches, you can easily branch off you current feature branch, keep going and rebase to master when the original branch has been merged. Continuous integration is with you all the time and helps the reviewer by annotating the diff with code coverage, cyclomatic complexity and other metrics. A good review culture emphasizes design feedback over low level issues. In addition, plenty of positive feedback is as important as finding issues.

Re: Code reviews aren’t just for catching bugs

#100

Post by yxhuvud »

jakub_g wrote: post #68
hangonhn wrote: post #7

I would go further and say that you should never trust code reviews to catch bugs. They're great for all the reasons in the article and what "zhemao" said but they're horrible for catching any bugs beyond the most trivial ones. Humans simply aren't good at running code in their head like that. Code review is no substitute for good code coverage and automated testing, preferably pre-commit.

Honest question: what strategy would you recommend to deal with a (senior) developer who specializes in opening gigantic pull request with significant number of bugs? (I invest a lot of time to read through the code and I catch lot of stuff, but it's draining huge amounts of my energy). Declining anything with coverage below 100% is not a viable option unfortunately, I think, and preaching about best practices gives…

"This commit does too much different things. Please break it up into smaller units that do one thing only."

And then continue to reject it until you get nicely chunked up stuff.

Post reply on HN