
Code Review Best Practices - Kaedon
http://kevinlondon.com/2015/05/05/code-review-best-practices.html
======
trustfundbaby
> If the reviewer makes a suggestion, and I don’t have a clear answer as to
> why the suggestion should not be implemented, I’ll usually make the change

This I feel is bad. Code reviews are usually between peers so you shouldn't be
afraid to seek out clarification where possible. You shouldn't be making edits
to code that goes in production without clearly understanding why.

The other thing that wasn't mentioned, that I think is important, is to not
act as a blocker for code reviews unless its absolutely necessary. Lots of
engineers take on the attitude that they're going to "gate" code they don't
agree with by with holding their +1 and bogging down the review with questions
and all sorts of runarounds till its what they want. this is a bad attitude to
have, even when you're dealing with Junior engineers.

I'm generally going to +1 something unless I fundamentally disagree with it or
think its going to break things in production. What I do, though, is leave
lots of comments with questions/suggestions and mention it in the +1 with (see
comments).

This builds trust on teams, and stops things getting personal, especially with
people who aren't very good at dealing with criticism, even in something as
banal as a CR. On a team that works well together, teammates will see those
comments, think about them and make thoughtful responses, especially once they
understand that you're not trying to get in their way. Giving the +1 gives
them the freedom to consider your suggestions without being irritated that
their PR is being blocked. They feel like they're in control not you.

In rare exceptions, someone will brush off my questions and merge ... which
means that next time, I get to be tougher on the review and specifically ask
for responses before the code can be merged, because they've degraded the
implicit team trust. Usually repeat offenders are assholes, and assholes
generally don't last on healthy teams.

~~~
Cymen
I agree 100% with the "don't be a blocker." I personally look for about 80-90%
yes and if it meets that criteria with no errors, I thumb it up. If I can't
thumb it up, I try to put a "thumbs up with commented items fixed". I try to
not block -- comment for bad issues, sometimes sigh but agree when it's a 10%
disagreement and go forward. Life is a series of iterations. That 10% will be
addressed in a future iteration.

And I never close another persons PR.

------
pablobaz
While I agree with all the points listed in the article, it highlights for me
a major problem of a lot of code reviews.

Most code reviews seem to focus on:

1\. Examining what the change does 2\. Finding ways to make the change in a
nicer way. E.g. Refactoring etc.

This leaves out the key step 0 - what is actually trying to be achieved, does
it need to be done and is there a better (maybe completely different) way to
do it.

This leads to a focus on relatively trivial matters such as naming conventions
and method lengths.

I think that the underlying reason for this is laziness. Talking about clever
refactoring is an easier/faster process than understanding the 'why'.

~~~
MichaelGG
I'm guilty of getting stuck up on trivial formatting issues. When someone
pushes a commit that has random whitespace (trailing or arbitrary newlines all
over, or just inconsistent spacing), it feels sloppy. Same for many other
simple things. If the code is unnecessarily superficially ugly, it sets up a
block in my mind that makes it harder to focus on the real issues.

Is it wrong to kick this stuff back and tell devs to make it pretty first?

~~~
dkubb
I believe that ugly code is usually buggy code. If someone didn't take the
time to deal with trivial issues like formatting then it makes me wonder if
they spent time thinking hard about the actual problem they were trying to
solve.

I usually explicitly do code reviews in 2 passes. I look for low level
problems first, and also try to identify ways that they can be tested using
tools so that next time they are caught before the code is sent for review. My
second pass looks at higher level problems with the overall design and checks
the functionality to make sure it matches the objectives.

------
enqk
setting thresholds on method / class sizes seems quite arbitrary and
potentially harmful. Splitting a method into n different ones, none of which
is called more than once is setting up the code for opportunistic reuse and
obscures it's true function. It's especially wrong if the code that was split
is mutating / non pure functional.

see [http://number-
none.com/blow/john_carmack_on_inlined_code.htm...](http://number-
none.com/blow/john_carmack_on_inlined_code.html)

~~~
hueving
I disagree. Breaking down a function into several one time functions that all
sit at the same mental model of abstraction make code much easier to reason
about.

~~~
Fr0styMatt8
I agree with you in principle but find that code editors let me down in that
regard.

Say I split a function up into a few sub-functions because it's getting big.
Now I have the problem that I'm jumping forwards and backwards through the
code when I want to explore what that function does:

SomeMassiveFunction() { SubfunctionA(); SubfunctionB(); SubfunctionC();
SubfunctionD(); }

SubfunctionA() { } ....

In this case, SubfunctionC() might end up a few pages down in source code. So
now it's a context switch to go there and then go back.

Now this can be somewhat avoided with good function names (so you don't HAVE
to jump backwards and forwards) and keyboard shortcuts (to make the process
quicker), but it's still a trade-off that I think needs to be kept in mind.

~~~
swsieber
I think it's a trade off we could get away from if we improved our tools. We
should make tools that reduce our cognitive over head. I am regularly diving
through code at my new job trying to figure out how everything fits together.
Reading code split between various functions and figuring out program flow is
a necessary skill, even if you do keep monolithic functions.

I would love an IDE that showed inline in some sane manner function calls,
letting me recourse arbitrarily deep - some thing ala code collapsing and
expansion. Extra points if you can show a for chart to side of function calls,
class interactions and general code flow, highlighting where your cursor is.
More points for a pane that shows what variables have to be in order for a the
branch of code you're working in to be reached. A changeable granularity
setting would be great for all of these.

I think often times we discuss pros and cons without discussing what they
could be if we put some effort into changing the current situation.

~~~
avn2109
You have suggested so many wins here. Gallons of win.

------
USNetizen
The one thing that is missing, which ALWAYS seems to fall by the wayside, is
security. If people incorporated more iterative security testing (static AND
dynamic, automated AND manual) and threat modeling into their SDLC reviews
there would be a plummeting number of vulnerabilities.

But, because it doesn't fit in with the whole "Lean" approach to software
(deliver features yesterday), all but the most established enterprises don't
seem to care much unfortunately. Once more people experience a breach because
of their desire to deliver first and remediate vulnerabilities later then
perhaps more awareness will be raised. By then it's too late though.

------
maguirre
I have an interesting problem. A co-worker of mine appears extremely sensitive
to his code being reviewed and I honestly don't know how to deal with it. He
feels attacked (and becomes defensive during code reviews) because the
reviewers focus on the "bad things and mistakes" of his code instead of the
accomplishments.

Has anyone here dealt with similar issues during reviews?

~~~
shakeel_mohamed
+1 I'm currently dealing with this. The individual doesn't offer feedback on
my code when under review, so it appears like a personal attack.

~~~
halostatue
Go have coffee (or whatever) with your colleague and have a 1-to-1
conversation with them saying you _want_ their feedback on code. You want
their expertise to make you a better software developer. Even if you are
better at this than they are, you _can_ learn something from the sharing.

The team’s lead should make sure that the individual in question knows that
software development is a cooperative process and that code review helps build
the team.

~~~
MaulingMonkey
This. Although I'm less civil about it - I'll start giving people shit for
rubber stamping my code reviews.

"Why are you letting me ship this terrible code I wrote? Do you want us to end
up in a death spiral of technical debt and crunch? Don't be an asshole,
critique my shit! We all need a second set of eyes - I'm no exception!"

...okay, I'm maybe a little more civil than that. But I'm not above leaving
some mistakes unfixed to call them out on.

~~~
garthk
Ha! I'm like that with my QA people, begging them to come after me with a
blunt weapon.

------
cmpb
Nice list. This is more or less what I look for. It's nice to see your rules
of thumb.

Anyone have any suggestions for time-estimating code review? That's the
biggest issue we've faced trying to implement code review into our workflow.

~~~
Kaedon
SmartBear has their own set of best practices and they recommend about 300-400
loc per hour[1]. I think that's probably about right. It's something I think
I've gotten faster at over time, partially because we're improving from
previous reviews and partially because it's a skill that develops.

[1]: [http://smartbear.com/smartbear/media/pdfs/wp-cc-11-best-
prac...](http://smartbear.com/smartbear/media/pdfs/wp-cc-11-best-practices-of-
peer-code-review.pdf)

------
BurningFrog
This is not a bad list, and can be useful as a checklist.

But note that it basically tries to define good programming practice in
general. That's a very big topic with a lot of room for debate and
disagreement.

------
mikehaggard
>Variable names: foo or bar are probably not useful method names for data
structures. e is similarly not useful when compared to exception. Be as
verbose as you need (depending on the language). Expressive variable names
make it easier to understand code when we have to revisit it later.

I so agree with this!

Properly named variables is perhaps THE first line of defense against bad
code. Too many developers think they are concise and having little code if
they only abbreviate their variable names enough.

Honestly, "em", "erg", "fc" and "sc" may make perfect sense to use, but it's a
form of obfuscation to future developers (including your future self).

Other pet peeve; adding things to variable names that don't add anything
meaningful.

E.g.

"usersList"

Does your code really care that it's a list? Should the reader be pointed at
this each and every time. I much prefer just using:

"users"

Clear, to the point, and readable.

~~~
MichaelGG
It's not a form of obfuscation if there is clear and simple context. Catch(ex)
is perfectly fine, and adding 7 letters does nothing but add noise. Similarly
"var us = getUsers()" is clear - it's not one u, it's multiple, and makes
sense to have a loop like "for u in us".

A better alternative if you find the code is still confusing is to add context
by cutting a function up into inner functions. This is why I really hate
working in languages that make it difficult to define little closures. In F#,
I'll often end up with a couple of 1 or 2 line inner functions and it makes
things much easier to read. This simply isn't practical in e.g. C#.

In exported function names and certain class names or modules, sure, a bit of
verbosity might help. But inside a function it just make it visually harder to
understand and I've rarely found it to be beneficial. There's going to be
enough context to load into my brain inside a function accurate, and
unfortunately I still subvocalize when reading code and all those extra
syllables add up.

Additionally, removing letters means you need to split lines less based on
length and focus more on when it makes sense.

------
bottled_poe
Some of this is frustrating to read. Architectural and detailed design
decisions should be made and approved almost entirely before the coding of
those features is started. (Obviously this is the ideal and not always
possible).

This means that the code reviews should involve little more that a checklist
of those approved design decisions against their implementation, perhaps a
code style is verified as well.

Coding without a design is just hacking, which I believe is the primary cause
of burn-out and should be avoided as much as possible.

So, the question is, do you have a design document? I know it doesn't sound
very agile, but traditional engineering procedures, when managed well, have a
lot of merit in controlling product quality and cost.

------
shakeel_mohamed
Another thing to check for that I didn't see addressed is debug statements.
There's nothing quite like seeing console.log("shit"); during a code review :)

------
zatkin
Awesome. I'm joining Cisco for the summer, so I think this would help me get a
head start since they do code review. Thank you!

~~~
azatris
Are you the person who took my place? :) Got to the last stage, twice, in
London.

To me, the Code Review Best Practices seem awfully like very general
knowledge, just gathered together. Not sure if it's HN-worthy per se.

However, the John Carmack link is quite enlightening.

~~~
zatkin
I sure hope not -- I'm working at the headquarters in San Jose this summer.

------
Too
This list is very very basic, most of the things like style shouldn't have to
be discussed and design should preferably be done before the code is written.

Just adding "error handling and potential bugs" as a generic bullet on the
list just doesn't cut it, these should basically be the only items on the list
but specified in much greater detail. A serious code review checklist should
contain concrete scenarios of these, preferably tailored for your specific
application.

Examples of this are: What happens during system startup, before all other
modules are ready to answer to requests? What happens if the user enters
malformed data (sql injections etc)? Does this function shut down gracefully
(transaction safe)? How will the algorithms scale? Race conditions and
deadlocks? What happens if the network to the database goes down? Is
localization handled properly? Backwards compatibility?

------
justinfreitag
Style, complexity and coverage checks should be left to automated tooling.
Code reviews should focus on whatever remains.

------
bozoUser
While I agree with all the points in the blog, I wonder how many programmers
do really follow them perfectly(even the author of the blog) because doing
code review to such great detail requires plenty of time which is often not
the case when you work for corporations.

~~~
chunkstuntman
One company I worked for relied heavily on code reviews after every feature.
At least two co-workers (one of whom had to be a supervisor) read, ran, and
gave feedback on every piece of code. Reading others' code and providing
feedback allowed me to improve my sight-linting ability, and it felt like each
day my group's code as a whole was improving.

Having some accountability for writing sloppy code is very sobering.

~~~
clebio
> supervisor ... ran ... code

This and this again. Engineering managers, and all that. But also, don't just
review, 'sight-lint', and reason about code. Rather, run the tests! It's the
shared accountability and knowledge. Does the baseline capability exist (tests
pass)? Can you, the reviewer, spot fallacies that the tests don't capture (if
not, criticisms feed back to you later)?

------
Kiro
> If we have to use “and” to finish describing what a method is capable of
> doing, it might be at the wrong level of abstraction.

When I coee, somewhere a method needs to initiate this execution flow and will
therefore contain "and". Even if all it does is call two other methods where
this principle is followed. How do I avoid this? I mean, somewhere in the code
it makes sense to execute the methods together.

------
a3voices
Having worked for businesses that use code reviews and those that don't, I
personally favor not having code reviews. The reason is that they hinder
development speed quite a lot, since you have to try to predict what other
engineers will say on your reviews, which takes a lot of brainpower.

~~~
jlarocco
Also having experienced both, I wouldn't want to work in a place that didn't
have code reviews.

I ask myself, "WTF was this guy thinking when he wrote this?" far less often
when I'm working on code that's been code reviewed. Most of the time, the
reviewer catches code like that, and requests that it be fixed and/or
commented, before I end up debugging through it 6 months later.

IMO, open office plans, reading HN and nerf guns hinder development speed far
more than code reviews.

~~~
MaulingMonkey
I experienced both within the same company - they started without code
reviews, and the later introduced them. The result was overwhelmingly
positive, greatly improving code quality and helping spread knowledge of new
systems and utilities.

I certainly believe it's possible to do code reviews sufficiently "wrong" that
they're a net hindrance. Drape them in too much ceremony, put too much
emphasis on style (Bob doesn't like your whitespace preferences, and he's the
code owner, so your changelist is vetoed!) and too little emphasis on
subsistence (these corner cases and possible bugs concern me).

.

We went with a relatively light touch approach. Nothing was technologically
enforced - our lead dev simply told us he wanted everyone to start having
everything reviewed by anyone - both for code quality and to spread knowledge
- and that he'd be pissed if he found a bug in one of our changelists and
found out it hadn't been reviewed, going forward.

Whitespace / naming stuff I tended to submit without review.

Quick and obvious fixes, small changes, etc., I was fairly willing to start a
review request, submit, and then fix anything caught in the review in a
followup changelist.

For larger changes where I had more reservations, I'd usually hold off on
submitting until after a review. Maybe threaten to submit if they were
dragging their feet too long :P

