
Ask HN: Do you do code review? - jadeydi
Do you do Code Review? Any good tool or suggestion?
======
mengledowl
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 purpose of a code review is to make sure the code
is good, but also to help the team grow together. If growing isn't the focus,
then code reviews can quickly become harmful.

2\. Get code reviewed early, often, and in small chunks. No one wants to
review a 1,000 line change spanning several sub-systems, and this is the
quickest way to get people to just blindly approve or to discourage them from
taking the time to review. Smaller changes make it easier to justify taking 10
minutes to look over the code and make some good, constructive suggestions. It
also makes it easier for the person writing the code to implement the
suggestions.

3\. Don't take it personally. It can be easy to let our ego get in the way
when reading someone's comment about our code - after all, we might be proud
of it after having spent 45 minutes on the 10 lines that someone just said
"could be cleaned up"! Keep in mind that code reviews are about growing and
learning, building better systems, etc.

4\. The review is about the code, not the coder.

5\. Honesty is key. Don't be a jerk, but don't avoid making suggestions just
because you're worried it might offend someone. That just makes the review
pointless!

6\. Be willing spend time with the reviewer if you wrote the code, and vice-
versa if you reviewed the code, to go over the suggestions made. Sometimes a
comment isn't enough to adequately explain something.

I could probably write more suggestions, but I think these cover the basics.

As for tools, I just use PRs. Create one, either add certain people as
reviewers or put a link to the PR somewhere that you can ask for reviewers,
and then await their approval/rejection/comments. Once you have approval from
the reviewers, merge it. From there you can start to build out different
processes/rules around it as you see fit, but it doesn't have to be
complicated and doesn't require anything fancy.

I would recommend doing them more async as opposed to scheduling "code review
meetings" however. These tend to be more wasteful and can introduce a lot more
stress.

~~~
http-teapot
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 linting would help.

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

------
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/](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=<someuser>" 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.

~~~
bo0tzz
Could you elaborate on your experience with pair programming?

------
alangpierce
In every professional team that I've been on, nearly every change has gone
through code review. Skipping code review is only done with a very good
reason, like if there's a time-sensitive change that you need to make and
nobody is around to review it. Another reason to maybe skip code review is for
prototype projects or internal tools where low-quality code is ok.

Code review is not only a good way to spot bugs, it spreads knowledge and
context throughout the team and makes sure everyone is consistent about both
high-level and low-level coding practices. This is especially important when
two people are writing code that will integrate with each other; normally each
will look over the other's code. It also ensures that if something breaks,
there are at least two people who might know how to fix it. Every software
engineering context is different, but it seems like in most cases, it's
irresponsible to ship code to users that has only been seen by one person.

GitHub, Phabricator, and Gerrit are all good code review tools. I would
recommend _both_ using a tool like this and having a habit of talking through
code changes in person.

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

~~~
matt_the_bass
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.

~~~
jchung
short sighted, but that's sometimes the right way to prioritize. sounds like
kenning is in prototyping mode, and could be intentionally prioritizing time
to first test, acknowledging that s/he might have to rebuild things later. in
the early stages of a startup, speed to insight can easily be more important
than long term coding efficiency. so short sighted thinking can make sense.

~~~
matt_the_bass
I agree with “sometimes”. However from Kennings post it sounded like “always”
and that code review is of no value.

~~~
kenning
> and that code review is of no value.

I don't agree with this at all. Code review is obviously a huge benefit once
you have enough money to pay more than one engineer per gigantic slice of the
codebase.

I'd say code review is also great if everyone is working on the same thing,
such as a library. In that case I'd do code review if there were only two
people working on the project. But when one guy is basically just doing react
boilerplate and implementing designs, it's not worth the time to check every
commit they make.

~~~
matt_the_bass
Thanks for your clarification.

------
hennsen
Yes i do whenever possible - that is, if the team i work with as a freelancer
already does or is willing to follow my advice to do it. That is -
disturbingly - not always the case.

I only worked with gitlab abd github pull/merge request code review tooling
and found it ok. There’s Gerrit, but i had no chance to work with it yet.
Obviously always depends on how they need to be done. When in doubt, just sit
next to each other and look at the part of code and changes for the current
task your working on that is about to be merged. Not always a special tool
necessary and if you don’t know which one to use better just start than let
the tool choice get in the way. That being said I’m not against tools!

Suggestions? As with any process or meeting, define what the scope is! Is it
about coding style? Which are your guidelines for that one then? Is it about
getting a second look if the story is implemented in a good, efficient way? Is
it to verify definition of done rules are met? About having (good) tests? It’s
probably not (counterexample i just had) to discuss the actual requirements
defined by the story the code should fulfill... that should have happened at
another time, probably with other persons, like the Product Owner. How often?
I think it’s best to do it with each merge request for the code it contains,
and maybe in defined tinespans for the whole project.

------
Spartan-S63
We don't do code reviews because we pair program 100% of the time. Our belief
is that pairing is like embedding a code review in the process.

~~~
seattle_spring
I would literally rather be unemployed than have to interact with someone for
the full 8 hours every day.

------
marpstar
Yes, fairly regularly. We do "agile" at my 9-5, and we have at least one code
review per user story that we complete, usually when the story is about 2/3 of
the way done.

How big is your team? Do you all work in the same physical location, or are
you remote?

We don't use any fancy tools. We just go back through the commit logs for the
branch and diff from before we started to present. For any major refactoring,
the lead on that user story will walk the rest of the team through the
architectural changes.

We recently moved to Visual Studio Team Services, which has a few more
niceties as far as code review goes. I'm looking forward to digging in, but in
no way do I think that tools like that are _necessary_ to perform and derive
value from code reviews.

------
avarsh
We use Crucible [1] for reviews and Jira [2] for task management. Each review
links to a Jira task (or handful of related Jira tasks). On each review, we
ideally:

\- Add 3-4 people as reviewers

\- Reviewers ask questions, make suggestions

\- Author follows up to these questions (online or offline), and if they make
code changes they link to the diff in a comment

\- Close reviews only if 2+ people have finished reviewing

\- It's okay to close reviews that not everyone has finished reviewing

[1]
[https://www.atlassian.com/software/crucible](https://www.atlassian.com/software/crucible)

[2]
[https://www.atlassian.com/software/jira](https://www.atlassian.com/software/jira)

------
thanatos_dem
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 task management systems and wikis, and tools
for pre-commit reviews, or post commit reviews/audits, and I found it much
easier to run and maintain than github enterprise or the Atlassian suite (and
it’s free).

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

~~~
joshuamorton
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.

~~~
kevan
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.

~~~
joshuamorton
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.

------
ruffrey
At a medium sized company. Everything gets code reviewed by one or two teams.
Only one or two approvals are required. Stash/bitbucket is used and we
leverage the Default Reviewers feature. The process works very well, is
integrated with our ticketing system (Jira) and build system (Bamboo). We are
nice in the code reviews but thorough and a little pedantic about style.

The only reason I can picture not doing code review is if there is only one
developer. It is an excellent way for everyone to learn, reduce bugs, reduce
spaghetti code, force you to write tests, etc.

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

~~~
geofft
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.

~~~
throwaway84742
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.

~~~
bostik
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.

~~~
throwaway84742
TBH I favor atomicity over small size. So we get a few gnarly PRs a week.
That’s also why we use Reviewable instead of GitHub reviews

------
davidjnelson
A workflow using feature branches off of master that show changes visually
using github, bitbucket, and similar tools works well.

------
ochronus
Yes. No code gets into the master branch without a code review unless it's a
well defined critical fix during an outage.

------
dmarcos
Code reviews keep me honest. I know I’m often sloppy and I wan’t to rush
things. Code reviews keep me in check. It’s when team work in software
development manifests at its best. You need good trust among peers though or
it can be destructive. Code reviews are not about looking the smartest or
putting others down.

------
tempuracat
How do you motivate people to actually review the code? Not just accept it.

~~~
throwaway84742
By requiring reviews for merging into master in GitHub and postulating that
unless something is in master it does not exist in the product.

~~~
PeterisP
What you propose is perfectly compatible with people merely blindly clicking
'Accept" instead of actually reviewing it, which is the problem of the parent
poster.

~~~
throwaway84742
Explain the value. Then spot check and separately talk to those who blindly
click accept, and if they continue to do so, get new engineers.

------
Whiteskin_Kanye
I would 100% recommend pair programming. Its baked right in!

