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

31–40 of 50 posts

Re: Ask HN: Do you do code review?

#31

Post by kenning »

I work for a tiny 2-man project and we need to save all the time we can, so we don't code review. We actually have an unofficial policy: all code that is directly user-facing (web design for example) is handled by me and all backend stuff is handled by the other guy.

My previous company worked the same way though, and it was a real company with funding and customers. It was another tiny startup (essentially 4 person engineering team) and the work was cleanly divided: one person did the entire iphone/android app (react native), one person did the website, one person did the backend and one person did the analytics. For an established company this would be a nightmare -- we had tons of bugs in production, usually reported by our users. However, having a really clean separation of duties made it extremely efficient in terms of management, because everyone knew that if they wanted something done on the mobile app they just asked the guy who did the whole thing, he knew the codebase thoroughly and could fix or add the change almost immediately, then would work on the next task on his trello board (which almost nobody else even looked at). We essentially had no management (CEO was constantly gone looking for more funding) so this organizational structure was a huge boon, and allowed us to push out huge features in 1-2 weeks. I think that if we had a full time QA person this really could have worked well, and by "well" i mean "we could have done the job of a ~15 person engineering and QA team with only five people."

Re: Ask HN: Do you do code review?

#32

Post by joshuamorton »

kevan wrote: post #29
joshuamorton wrote: post #23

Earlier quoted context omitted.

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.

Generally speaking, your un-reviewed change isn't a "fix", but something to stop the bleeding. It might move traffic out of a problematic area, or disable an experiment that's leading to unusual metrics.

In other words, its a rollback, not a fix-forward. (and I'll note that where I work, these changes still need to be reviewed soon after they're submitted). I agree that all fixes should be reviewed before submission.

Re: Ask HN: Do you do code review?

#33

Post by throwaway84742 »

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.

This is absolutely the wrong thing to do. When mitigating issues on call anything that goes live bypassing canary has to receive much more scrutiny, not less. It is best not to do such things, but if you have to do them, review the change extra hard.

Re: Ask HN: Do you do code review?

#34

Post by matt_the_bass »

http-teapot wrote: post #16
mengledowl wrote: post #3

100% would recommend doing code reviews. The most effective way I've found to do code reviews is to create a pull request for the code you would like to have reviewed. Send it out to someone or a few people that you would like to take a look, get them to approve or reject it, and then take a look at their comments and see what you can do to address them. Some guidelines for code reviews: 1. Build each other up. The p…

This is a great list! I'd like to suggest one if you don't mind 7. Review your own code before handing it over. Time is wasted when the submitter left out obvious issues such as typos, missing comments, unnecessary complexity, debug code, etc. Consider the reviewer's time precious because context switching is expensive and she/he might not get back to it as quick as you'd like to. Implementing clear guidelines and li…

Using a linter goes a long way for this. Maybe make linking part of the check in process.

Re: Ask HN: Do you do code review?

#35

Post by throwaway84742 »

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.

And moreover, I send my gnarliest PRs to the most vicious reviewer on the team, just because 3 months from now finding out what’s wrong is exponentially more difficult. I want to get it right the first time around, and I want other folks to be able to make changes anywhere in the codebase, even in the gnarly parts.

Re: Ask HN: Do you do code review?

#36

Post by matt_the_bass »

kenning wrote: post #31

I work for a tiny 2-man project and we need to save all the time we can, so we don't code review. We actually have an unofficial policy: all code that is directly user-facing (web design for example) is handled by me and all backend stuff is handled by the other guy. My previous company worked the same way though, and it was a real company with funding and customers. It was another tiny startup (essentially 4 person…

With all due respect, your process is likely short sighted. In the long term, you would probably benefit from code reviews. I find code review very effective even in very small teams.

Re: Ask HN: Do you do code review?

#37

Post by joshuamorton »

throwaway84742 wrote: post #33
joshuamorton wrote: post #23

Earlier quoted context omitted.

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.

This is absolutely the wrong thing to do. When mitigating issues on call anything that goes live bypassing canary has to receive much more scrutiny, not less. It is best not to do such things, but if you have to do them, review the change extra hard.

See my other comment to a sibling expressing a similar sentiment. Rollbacks and drains may require submissions to change configurations, but aren't bypassing canary, they're specifically reverting to known good systems.

Re: Ask HN: Do you do code review?

#39

Post by bo0tzz »

madmax108 wrote: post #22

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…

Could you elaborate on your experience with pair programming?

Re: Ask HN: Do you do code review?

#40

Post by bostik »

throwaway84742 wrote: post #35
geofft wrote: post #20

Earlier quoted context omitted.

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.

And moreover, I send my gnarliest PRs to the most vicious reviewer on the team, just because 3 months from now finding out what’s wrong is exponentially more difficult. I want to get it right the first time around, and I want other folks to be able to make changes anywhere in the codebase, even in the gnarly parts.

We have mandatory code reviews for everything, which encourages small, bite-size commits and frequent merges. Majority of the changes goes through with just one other person taking a look.

But we also specify (and highly encourage) that anyone trying to land a more invasive or complex change should require multiple review approvals. The most demanding "please pile on, we want eyes on this" request I remember was set to require 4 separate approvals.

Post reply on HN