Ask HN: How do you do code review, if at all? | Hacker News Reader

Ask HN: How do you do code review, if at all?

7 pointsgoralph6 comments

If you do, what's your process? Pre-commit, post-commit? How many devs need to review a change set? Which service do you use?

If you don't, why not? Have you thought about starting? What's blocking your team if so?

View on HN

In company I work, we've developed through years quite complex code review. First of all - we're programming in Perl 5.21 and because of this we made coding standards to ensure people will code in simmilar manner. Second - we have mailing list where summary of all commits is send - code, reason, project description etc. Every developer is allowed to read those commits and if he find anything suspicious - he can rand code check (or wtf) message and authour of questioned code has to respond (either by correcting his code or explaining why he did what he did). Third - we do partial reviews during project. Whenever sprint is finished - there goes minireview. Fourth - after project is finished - there is inner team project code review, where developers take step back and look themselves on their and theirs teammates code. Fifth - when inner-team review is finished and all issues are fixed - there is outer-team review. Senior Engineer or Software Architect is appointed to overview whole code, but at this point any developer from any team is allowed to join outer-team review.

Leading reviewer after finishing his work decide about eventual after-fix review (if something was really messed up despite all precautions).

And - just for sure - before releasing code to production releasing architect take quick eye look on git diff. If nothing sting him on his eye - code is like Dobby - free!

Only one reviewer is enough for us. Once a PR is submitted to GitHub, it should be merged by the reviewer, who usually only has to click that GitHub merge button. This means we favor a rebase-workflow, which has worked great for us.

There are two cases in which we bypass this mechanism. One is hot-fixes that can't afford to go through this process, and the other is changes too trivial to wait for a reviewer. This later case is rare, since there is always a reviewer, but we do have some remote colleagues (Australia, SF) who should use their judgment in deciding whether a PR is worth being reviewed. This is slightly subjective but so far all decisions in this regard were excellent. Once the team is bigger perhaps we'd do away with this and keep hot-fixes as the only review-by-passers.

Exactly one developer should be responsible for a review. Otherwise, you get diffusion of responsibility, and both reviewers assume they can half-ass things. Also, even if they were diligent, you're needlessly creating work for yourself, and the marginal benefit of the second reviewer is very small. Having two reviewers is okay if you give them non-overlapping parts of the code to review.

You should definitely do code reviews. Their main benefit is in the long-term improvement in coding quality and coding ability of your team members.

Pre-commit code reviews are usually ad hoc for us. Simpler changes may undergo no review while larger refactors are almost always reviewed with multiple people.

At the very least, about 2/3 of the way through the coding process, we again go through the code and make sure to identify ways we're deviating from the intended design. This gives us a bit of time to correct any issues and retest everything before moving on.

We just use TFS.

Our code review process:

- starts as a pull request (we use git-flow strategy)

- Pull request is announced in our team slack channel via integration

- 2 devs must sign off before merge; 3 if branch is from our outsourced team in India.

- Github project wiki contains an outline of the type of criteria to keep an eye out for, mostly syntax & patterns not caught by our linter config

- All feedback is considered optional

- Devs are encouraged to take conversations offline if a comment thread takes off on a particular item

- Team leader handles the merge after devs signoff on PR

At Nylas we use Phabricator (which I heartily recommend) for code reviews. Generally we have two reviewers, one is responsible for the review and the other one tries to notice things the other reviewer hasn't found.

Reply on news.ycombinator.com