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

11–20 of 233 posts

Re: Improving code review time

#11

Post by shortstuffsushi »

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 I read it right, it described there being a "code review team." Whether they're responsible for reviewing all code, or just for coming up with these sorts of practices was not clear. ETA: with that said, the "reviewer recommender" to me would imply it's people who work on the same codebase.

Re: Improving code review time

#12

Post by Scubabear68 »

I came to the conclusion long ago that mandatory code reviews are a waste of time. For critical stuff, absolutely.

But PRs and review cycles over burden dev teams and don’t seem to move the quality needle higher one bit.

A better way is to ensure multiple hands touch a given area of the code, so that multiple eyes ultimately are seeing and manipulating those bits. If they are given a task to do in that area they will be motivated to understand it (and potentially improve it). By contrast, with code reviews often the reviewer does not have time to really deep dive into the code and will only have a superficial understanding of it.

Oh, also use code quality scanners to keep an eye on tactical code debt.

Re: Improving code review time

#14

Post by rbongers »

Scubabear68 wrote: post #12

I came to the conclusion long ago that mandatory code reviews are a waste of time. For critical stuff, absolutely. But PRs and review cycles over burden dev teams and don’t seem to move the quality needle higher one bit. A better way is to ensure multiple hands touch a given area of the code, so that multiple eyes ultimately are seeing and manipulating those bits. If they are given a task to do in that area they will…

I have been on teams that never review code and teams that always review code, and I can confidently say I never want to work on the former again. That was with a junior team, but your team is going to have new people, if not junior, at some point. People who are familiar with each other's code and have agreed on standards can review code pretty quick. I would rather have it and not need it than need it and not have it.

Re: Improving code review time

#15

Post by kentonv »

Scubabear68 wrote: post #12

I came to the conclusion long ago that mandatory code reviews are a waste of time. For critical stuff, absolutely. But PRs and review cycles over burden dev teams and don’t seem to move the quality needle higher one bit. A better way is to ensure multiple hands touch a given area of the code, so that multiple eyes ultimately are seeing and manipulating those bits. If they are given a task to do in that area they will…

The most important reason for code review is not to fix the code but rather to transfer knowledge between developers. I've learned a lot of techniques for writing better code from suggestions from my code reviewers, and from reviewing other people's code.

Without code review, when will a junior developer ever learn anything from a senior developer?

Re: Improving code review time

#16

Post by zadjii »

kweingar wrote: post #10
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.

I work at a company with a similar code review culture. I have about 2 blocks of meetings per day on average. I usually check my assigned code reviews in the morning and when I come back from the meetings. Once I’m done with those then I move on to my own work. If I have too much code review to the point that I can’t get my own work done, I take my name off the reviewer list and it’s reassigned. On most days I’m stil…

> I take my name off the reviewer list and it’s reassigned.

How big is your team? We've got 4 developers on our team (and one lead), and require two signoffs for code changes. There's just not enough of us for something like that to work... right?

Re: Improving code review time

#17

Post by ummonk »

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.

Working there I tended to conduct code reviews each afternoon at the end of work. Sometimes after coming back from lunch as well. There isn't any "drop what they're doing" involved.

Re: Improving code review time

#18

Post by partdavid »

kentonv wrote: post #15
Scubabear68 wrote: post #12

I came to the conclusion long ago that mandatory code reviews are a waste of time. For critical stuff, absolutely. But PRs and review cycles over burden dev teams and don’t seem to move the quality needle higher one bit. A better way is to ensure multiple hands touch a given area of the code, so that multiple eyes ultimately are seeing and manipulating those bits. If they are given a task to do in that area they will…

The most important reason for code review is not to fix the code but rather to transfer knowledge between developers. I've learned a lot of techniques for writing better code from suggestions from my code reviewers, and from reviewing other people's code. Without code review, when will a junior developer ever learn anything from a senior developer?

It works both ways, too--it makes more developers aware of the others' work, what changes are being made and so forth. So it's not just expertise that gets transferred, but also the current state of the codebase.

Re: Improving code review time

#19

Post by JamesBarney »

Scubabear68 wrote: post #12

I came to the conclusion long ago that mandatory code reviews are a waste of time. For critical stuff, absolutely. But PRs and review cycles over burden dev teams and don’t seem to move the quality needle higher one bit. A better way is to ensure multiple hands touch a given area of the code, so that multiple eyes ultimately are seeing and manipulating those bits. If they are given a task to do in that area they will…

I've found PRs are usually a waste of time because people focus on such trivial bullshit because they're easy to find.

98% of the review comments I receive hark back to some handwavy explanation about maintainability. But never in any way that could actually cause a bug.

when I run teams my rules with PRs are if it's not a bug, and it's not against the code review guidelines leave it. It's usually not worth the back and forth unless there is a clear mentor/mentee relationship.

Re: Improving code review time

#20

Post by jiggawatts »

cheriot wrote: post #6
posharma wrote: post #3

Disclaimer: these are anecdotal reports. I've heard from a lot of my friends how abysmal the quality of code is at Meta. Obviously, this may not be true in all teams/products, but that's the general sentiment. Why make it faster when you're already dealing with mess! This is abundantly evident from the constant fire fighting, duct tapes and a metric driven culture that incentivizes the number of diffs landed.

Are there companies with a reputation for code quality?

Yes. VMware's ESXi kernel was unbelievably rock-solid compared to other similar systems. Fantastic resilience against even hardware faults. I've heard that there was a culture of "doing things the right way" there.

Meanwhile in the same org, the group doing their GUI kept adding band-aids to a broken mess for years.

Post reply on HN