

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

If you do, what&#x27;s your process? Pre-commit, post-commit? How many devs need to review a change set? Which service do you use?<p>If you don&#x27;t, why not? Have you thought about starting? What&#x27;s blocking your team if so?
======
ukoms
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!

------
why-el
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.

------
SamReidHughes
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.

------
marpstar
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.

------
theaccordance
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

------
vorador
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.

