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

Improving code review time

engineering.fb.com

Post reply on HN
Print view View original

101–110 of 233 posts

Re: Improving code review time

#101

Post by CogitoCogito »

nfRfqX5n wrote: post #99
Firmwarrior wrote: post #59

Earlier quoted context omitted.

I think most people would be able to handle any one of those terms smoothly on day 1 You: "I'm going to send a pull request" Other engineer: "OK. BTW, we call them change requests here" You: "OK"

The workflow is different enough where it does take some time to get used to

> The workflow is different enough where it does take some time to get used to reply

If the workflow is different, then it's _not_ just a question of terminology. In that case it's probably a good thing that there are different names for the various workflows.

Re: Improving code review time

#102

Post by Jason_Gibson »

gorgoiler wrote: post #76
andreygrehov wrote: post #25

Meta: > At Meta we call an individual set of changes made to the codebase a “diff.” GitHub: > Pull request Amazon: > Change Request GitLab: > Merge Request Google: > Changelist Nitpicking, but jesus christ, why can't we stick to a single term?

They aren’t all the same thing. A diff (or patch) is a change to the code that’s not tied to the history in any place. Linux kernel patches and Meta diffs are like this. Pull Requests and Merge Requests are more complicated, and the unit of change is a branch. Many people reject this workflow because ultimately a branch of commits can be summed to one single diff and that’s the only thing that matters in the wider pr…

A changelist is most generally a Perforce term (and as may the case for Google, a similar concept). In the Perforce Helix Core (what the "Perforce Server" is currently called), changelists are increasing integers that mark units of atomic change. Changelists can contain committed work, pending work, or shelved work (a shelf is un-committed work that resides server-side). Shelves are often how code review is done since the file content and metadata is available outside of the author's computer.

Re: Improving code review time

#103

Post by proc0 »

gthrowaway52 wrote: post #100
proc0 wrote: post #43

If you're going to add machines to the process why not add it with the purpose of eliminating the human from the process all together? Reviews are necessary because compilers and linters can't catch everything. Runtime bugs that are not caught by the pipeline tend to be edge cases that don't happen until there is enough data to test (in the general sense) the feature. ML could be used for smart testing and if it pass…

Well, yes, the menial parts of review can and should be automated. Big tech cos are leading the industry, and ML does find its way in there (with mixed success). But at its core, code review is putting two heads together to write some code instead of one. In that sense you can think of the code review problem as the "automating away all of programming" problem.

Fair point, and to that extent reviews can still be useful. However many code changes are small, bug fixes, or straight forward changes, so the main benefit would still be the same. There could be an option to wait for human reviews if needed.

Re: Improving code review time

#104

Post by lamontcg »

azinman2 wrote: post #68
bsaul wrote: post #7

there’s so many low hanging fruits for improving the quality of diff viewing. The worst code reviews are often the ones where code get refactored, leading to piles of delete / create lines that are just code being moved or slightly renamed. One very simple approach would be better git integration with the IDE, helping build commit that make sense, where a set of changes could easily be commented by the author as they…

This is a concept I almost pursed for my PhD over a decade ago. I’m still surprised no one has done this. The bigger picture: context is often missing for anything complicated, be it software or a new law. Yet many hands touch and retouch the underlying material over time. If you could capture _how_ something was built, and had enough insight into the larger process to sample some of the _why_, then you could both kn…

The PR history can answer the why, and if you can anticipate someone will ask why because you know it is edge-condition spaghetti then you can document it right in a comment.

Re: Improving code review time

#105

Post by mike_d »

underdeserver wrote: post #44

I'm a strong believer in fast reviews. I get into deep working mode for 3 hours a day total, on a good day. The rest is meetings, daily sync, coffee, lunch, "hey can you look at something", hallway conversations, emails, my own inability to concentrate when I'm not feeling it. I've been in this industry for coming up on ten years. None of this is going to change, unless I become an academic or a hermit. I don't get t…

> 2) People write good commit messages. If your commit message isn't in the following format: [snip] I'm sending it back to you.

Try reviewing commits without reading any associated messaging, or having your team submit complex changes with no message. You have to engage your brain to understand what the code is doing and what is being changed, you'll have a better understanding of if the comments are useful or insufficient, and most importantly you don't already have a bias that the code does what it says. You may find that when you really look at it foos are getting pushed in qux order, or that the benchmark was calling a different function.

Re: Improving code review time

#106

Post by yodsanklai »

comfypotato wrote: post #5

Am I reading this right? If the total median time in review is “a few hours”, that means that people are dropping what they’re doing to review the code. That can’t really be a good code review? A context switch alone would be half of that time for me.

Most reviews can be done quite quickly. Small changes, test plan is convincing, low potential to break things. Besides, if engineers are good, code is easier to review.

Re: Improving code review time

#107

Post by Jensson »

comfypotato wrote: post #5

Am I reading this right? If the total median time in review is “a few hours”, that means that people are dropping what they’re doing to review the code. That can’t really be a good code review? A context switch alone would be half of that time for me.

Slow reviews means that you need to context switch a lot to answer their review suggestions and resubmit the review, that is much worse for productivity than taking a few minutes to go and look through what a teammate is doing now and then.

Alternatively your code review culture doesn't give a lot of suggestions for changes before submission, and that leads to more technical debt which will produce bad results long term.

Re: Improving code review time

#108

Post by H8crilA »

comfypotato wrote: post #5

Am I reading this right? If the total median time in review is “a few hours”, that means that people are dropping what they’re doing to review the code. That can’t really be a good code review? A context switch alone would be half of that time for me.

If you're working 8 hours and do at least 4 breaks per day - that's a break every 2 hours. If you do a break you might as well skim through emails and do some reviews. And remember that everything* is Pareto distributed, the review request sizes too. It's really not hard at all.

[*] it's an exaggeration, but close to reality.

Re: Improving code review time

#109

Post by skirmish »

aftbit wrote: post #47
sgeisenh wrote: post #9

Earlier quoted context omitted.

Google places a pretty high emphasis on code quality and readability. It's not universally great, but it's a big part of the culture. You can catch a glimpse in their [style guides][1], [abseil totws][2] and [aips][3]. Almost every change is required to be reviewed for "readability" in addition to functionality. This can feel like a lot, but it leads to pretty consistent style across the codebase which makes it a lot…

Oh, Abseil is new to me. Thanks! Do you know where the missing tips are? For example: https://abseil.io/tips/110 The root page (parent's [2]) mentions this one as famous by name: >Often they are cited by number, and some have become known simply as “totw/110” or “totw/77”. Is totw/110 some Google secret sauce that we are forbidden from knowing or did they skip some weeks?

totw/110 was about safely initializing global and static objects. IIUC there was something missing in the open sourced version of Abseil so that it could not be published verbatim.

Re: Improving code review time

#110

Post by rlkf »

mike_d wrote: post #105
underdeserver wrote: post #44

I'm a strong believer in fast reviews. I get into deep working mode for 3 hours a day total, on a good day. The rest is meetings, daily sync, coffee, lunch, "hey can you look at something", hallway conversations, emails, my own inability to concentrate when I'm not feeling it. I've been in this industry for coming up on ten years. None of this is going to change, unless I become an academic or a hermit. I don't get t…

> 2) People write good commit messages. If your commit message isn't in the following format: [snip] I'm sending it back to you. Try reviewing commits without reading any associated messaging, or having your team submit complex changes with no message. You have to engage your brain to understand what the code is doing and what is being changed, you'll have a better understanding of if the comments are useful or insuf…

My takeaway from your comment is that one should read the code first, and then see if the comments match your understanding of it afterwards, NOT that one should not write any comments.
Post reply on HN