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

Ask HN: Do you do code review?

news.ycombinator.com

Post reply on HN
Print view View original

21–30 of 50 posts

Re: Ask HN: Do you do code review?

#21

Post by dmlittle »

xendo wrote: post #10
thanatos_dem wrote: post #5

I’m at one of the big 5, so everything goes through code review now, but since we’re a small team, we do monitoring ourselves, and as a result we have a lot of ways to bypass the process in case we need to get a high priority hot fix out. When I was doing a small startup, we had a small phabricator installation which I loved using. It’s very engineer-focused in a lot of its tooling and features, but has an integrated…

Why bypass the code review for high priority fixes? That's when you need it the most.

Depending on what the issue is you might want a quick fix right away which can sometimes be hacky and then put in a long-term fix that is code reviewed by others to make sure it’s the right solution.

Re: Ask HN: Do you do code review?

#22

Post by madmax108 »

Of course we do. Code reviews help us in multiple ways:

- You have 2 sets of eyes on every piece of code committed to master (You may argue pair programming does the same, but in our experience, pair programming requires a very different mindset, esp. hands-off pair programming)

- Multiple people know what changes have gone into master recently. We have an alias (code-review@) which gets org wide code reviews and is added by default and every eng has an option of tracking/commenting on code reviews.

- For cross team efforts, it's helpful since a team which might have context on a change can see the change before the actual commit. "Coding to interfaces" is great, but seeing under the hood offers a different view as well

For the actual process, we use ReviewBoard (https://www.reviewboard.org/). It has decent integration with command line so you can post reviews from the command line itself. We also have a git hook set up which looks for "R=" in each code commit, and rejects the commit otherwise. The hook is (intentionally) super simplistic so you can get past it using something like "R=Self", but with a strong engineering culture, we see this happening less and less with non-minor code commits.

Re: Ask HN: Do you do code review?

#23

Post by joshuamorton »

xendo wrote: post #10
thanatos_dem wrote: post #5

I’m at one of the big 5, so everything goes through code review now, but since we’re a small team, we do monitoring ourselves, and as a result we have a lot of ways to bypass the process in case we need to get a high priority hot fix out. When I was doing a small startup, we had a small phabricator installation which I loved using. It’s very engineer-focused in a lot of its tooling and features, but has an integrated…

Why bypass the code review for high priority fixes? That's when you need it the most.

The big example is config changes during ongoing incidents. You may need to flip a flag or update a config right now, but waiting for review, even if it takes just a few minutes, can blow slas.

There's ways to handle that somewhat though.

Re: Ask HN: Do you do code review?

#24

Post by joshuamorton »

matfil wrote: post #14

I suspect this may not be too well received, but I've thus far managed to avoid regular formal reviews, and I'd advise others to think twice before imposing them. They seem like a big step towards treating programmers like cogs in a machine rather than competent individuals, and for me that's a direction I don't want to be headed.

Seconding Geoff, I find code review to be one of the best forms of mentorship possible for a junior to mid engineer.

Re: Ask HN: Do you do code review?

#25

Post by matfil »

geofft wrote: post #20
matfil wrote: post #14

I suspect this may not be too well received, but I've thus far managed to avoid regular formal reviews, and I'd advise others to think twice before imposing them. They seem like a big step towards treating programmers like cogs in a machine rather than competent individuals, and for me that's a direction I don't want to be headed.

I find that good programmers will voluntarily request reviews whether or not they are required by process, and bad programmers will attempt to minimize them even when required by process. Having a policy is a good way to weed out the bad people, and won't affect the good people.

I might agree with you in the sense that sometimes there's a worthwhile discussion to be had and of course in such cases it's good to talk to someone who's opinion you trust.

That's different from mandatory code review, which almost inevitably will end up covering style and process considerations.

It's certainly a filter, but depending on your perspective I'd be a little cautious about "won't affect the good people".

Re: Ask HN: Do you do code review?

#26

Post by matfil »

ccharles wrote: post #17
matfil wrote: post #14

I suspect this may not be too well received, but I've thus far managed to avoid regular formal reviews, and I'd advise others to think twice before imposing them. They seem like a big step towards treating programmers like cogs in a machine rather than competent individuals, and for me that's a direction I don't want to be headed.

Have you tried doing them? Other users have already described healthy code review processes that improve code quality while helping all participants to grow individually and as a team. How does review treat programmers like cogs in a machine?

Yes, I've encountered them. It seems really hard to do them in a way that doesn't touch heavily on style. To the extent that the preferred solution to trying to shift the focus back to deeper issues is to go one step further and mandate auto-formatting tools. So individuality-of-style has been dismissed out of hand.

Can they be helpful? In some cases, perhaps yes. But they're definitely pushing a collective-ownership, try-to-smooth-away-individual-differences agenda.

Re: Ask HN: Do you do code review?

#28

Post by ccharles »

matfil wrote: post #26
ccharles wrote: post #17

Earlier quoted context omitted.

Have you tried doing them? Other users have already described healthy code review processes that improve code quality while helping all participants to grow individually and as a team. How does review treat programmers like cogs in a machine?

Yes, I've encountered them. It seems really hard to do them in a way that doesn't touch heavily on style. To the extent that the preferred solution to trying to shift the focus back to deeper issues is to go one step further and mandate auto-formatting tools. So individuality-of-style has been dismissed out of hand. Can they be helpful? In some cases, perhaps yes. But they're definitely pushing a collective-ownership…

It is much easier to read code that is written consistently, and it's much easier to contribute to code when a style guide is available. This isn't "pushing an agenda". And what's wrong with collective ownership?

Code review shouldn't involve bikeshedding about whether braces should go on the next line or not. It should be about design decisions, clarity of expression, algorithm complexity, etc.

Re: Ask HN: Do you do code review?

#29

Post by kevan »

joshuamorton wrote: post #23
xendo wrote: post #10

Earlier quoted context omitted.

Why bypass the code review for high priority fixes? That's when you need it the most.

The big example is config changes during ongoing incidents. You may need to flip a flag or update a config right now, but waiting for review, even if it takes just a few minutes, can blow slas. There's ways to handle that somewhat though.

Also at a big 5. We've found that skipping reviews causes more problems than it solves in emergency situations. It's really tempting to bypass the process because you're sure you can fix it fast, but this mindset results in more missteps in aggregate.

If a couple minutes really makes that much of a difference you should probably page multiple people in from the start of an issue.

Re: Ask HN: Do you do code review?

#30

Post by [deleted] »

dmlittle wrote: post #21
xendo wrote: post #10

Earlier quoted context omitted.

Why bypass the code review for high priority fixes? That's when you need it the most.

Depending on what the issue is you might want a quick fix right away which can sometimes be hacky and then put in a long-term fix that is code reviewed by others to make sure it’s the right solution.

[deleted]
Post reply on HN