Code Review Best Practices

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

[allsystemsgo]

I agree. I have had review comments that have made no sense at all. If I didn't ask why, I wouldn't learn. Also, to be honest, there have been times where the review comment didn't understand the context of my code, so the review comment ended up being incorrect.

[nazbot]

I'm going to differ on the 'don't be a blocker' thing.

It varies by team. If your whole team agrees that the code reviewer is the 'gate' then people understand that they just need to fix the thing and move on. I personally love that style of working because you tend not to write trivial suggestions and everyone knows that what people have written isn't meant as nit-picky and just has to be changed.

If you don't think it's the right change then the other person can just ask why it's important and hopefully get educated. I also tend to just trust my colleagues enough that what they suggest is a good change. I often find I may miss small things through like lack of documentation or whatever (get tired after a while) and those reminders in the CR are the equivalent of a spotter getting you to do one last rep at the gym.

[nimnio]

From a certain perspective, you just paraphrased the quote you disagreed with:

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

You: "[Do] not act as a blocker for code reviews unless it's absolutely necessary."

I think these express essentially the same philosophy, with some slight differences in the particulars.

[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?

[yoran]

I agree with this. But I don't think code reviews are suited for this step 0 as you say. Just the way a pull request is formatted, it's very hard for the reviewer to deduce from the changeset what the high-level design of the code is.

That's why we discuss these high-level design and architectural decisions before-hand so that they are known to the reviewer at the time of the review. We're a small team so it works well. But I'm not sure how this scales up as the team gets bigger. I would like to know how bigger teams approach this problem!

[jeremiep]

Same here, I've seen too many code reviews where people complain about a badly named variables and nobody saw the design was faulty leading to costly bugs to fix in production.

One thing we did on the current project is pre-commit reviews in pairs. This ensures at least two people in the team knows about the changes, let us talk about the why and how of the changes, and possibly teach a coworker new things in the process.

What it ended up doing is that every programmer now self-reviews their own changes prior to the actual review knowing they'll soon share it all face to face with a coworker. Turns out the talks are now about the design of the code, not how it looks.

[dsuth]

You should not really be arguing about design and architecture in a code review - that should be done in design review.

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

[userbinator]

Agreed, I find it difficult to read code that's been split up into many small pieces because it gets difficult to keep track of the overall flow. Even with an IDE, the jumping around (and maintaining a mental call-stack) can be quite distracting.

My guideline is basically "as big as it needs to be, without having large pieces of duplicated functionality." If an algorithm requires 100 lines, none of which are repeated or very similar to the others, then so be it.

I wonder what the distribution of function lengths is like in the Linux kernel... there will be longer ones and shorter ones, but a 20-line limit seems absurdly short.

Also vaguely related: http://www.satisfice.com/blog/archives/27

[mikehaggard]

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

Sometimes, but in general I don't agree.

If the functions are made private and giving appropriate names than it's really a way of self-documenting code, which helps.

Suppose you have:

    some_function() {
        // Calculate interest
        // 100 lines of code

        // Apply tax
        // 120 lines of code

        // Store results
        // 30 lines of code
    }

Then the need to have those "banner comments" there already indicate it might be a good idea to decompose the function into:

    some_function() {
        calculate_interest(...);
        apply_tax(...);
        store_results(...);
    }

It's not just about reuse, but about documenting what each section of code does, and about limiting interactions between arbitrary sets of variables. Functions are create boundaries (assuming they don't intensively use global variables or class instance variables when part of a class)

[flipperkid]

Where I work we refer to guidelines like this as code smell. These are cues that refactoring should be considered but not strict rules which would prevent a commit.

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

[krzyk]

If you name the methods correctly (what they do) splitting actually helps readability of the code. It is quite hard to read and remember what was in the beginning of 500 line method.

[mhomde]

It's always a balance between "The Blob" and the "Poltergeist" anti-pattern, when in doubt go more towards borderline "The Blob".

I just say though it's very seldom that a huge class/method is justified. Usually there are plenty self-enclosed logic or reusable methods that can be broken out either to utility libraries or a separate class.

My rule of thumb is that you should be able to look at a method/class and understand what it does, and that diminishes quickly over a certain size.

Sure some logic might be in other classes but that's another layer of abstraction

[reipahb]

I agree that arbitrary limits can be a bit restrictive at times, but that is mostly an issue when dealing with automated code checking tools.

However, in this case it is more about something to keep an eye for for the reviewer -- if the function is large and complicated it may be a good idea to take a closer look at it. Presumably the reviewer won't force the function to be broken into smaller pieces unless it actually improves readability and maintainability of the code.

[bliti]

I mean, some functions/methods will be longer than usual. Even if you split it into multiple ones the end product will be the same. Due to some of those derivative functions/methods being exclusive children to the caller. Sometimes it's best to leave the bigger one alone, and some times it helps to break it down. You do end up with basically the same size codebase. Dunno if it's more readable. Though this somehow shines some light into other issues. Such as language verbosity. Some languages are just syntax factories. All the logic is accompanied by a truck load of verbosity.

I agree with your point. It seems silly to be strict about such things. More so if the codebase is written in Java or any other verbose language.

[Kaedon]

Yeah, thank you for the link! I use this list as a set of rough guidelines, basically, so it may or may not apply in all cases.

Sometimes it can be helpful to split up a method to be called once if it helps readability, for example if it helps split up a loop or abstracts the specific way we get a value.

[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?

[MaulingMonkey]

Balancing the critique with some positive feedback - parts you liked, appreciation for the features shipped, etc. - is one idea I've seen floated around, for a particularly touchy coworker.

Also, generally managing tone - "Looks good! Don't forget to add the doc comments on functions X Y and Z" vs "Why are X Y and Z missing doc comments?". Both have the same fundamental information (X Y and Z could use doc comments), but:

With the former, you're acknowledging progress, a "good" first draft, and pointing out what remains to be done in an ego friendly way ("I know you're good enough to make this even better") that doesn't demand a response.

Meanwhile, the latter demands a response, all of them negative. Either "it doesn't need them", or "I forgot them (because I suck)", or silently ignoring the question. None of these are ego friendly.

.

If the code just plain sucks, however... I'm not really sure what to do.

And if the coworker gets defensive even with positively slanted feedback... maybe there needs to be a conversation with them, to try and shift their mindset. Show them you get things pointed out in your own code reviews, that it's a team game where everyone is just looking out for each other. That bad code doesn't mean they're a bad programmer, that critique of their code isn't a critique of their skill, that everyone gets blind to the problems in their own code and benefits from a second set of eyes.

Maybe get them in your shoes - helping critique someone else's code, and help explain that they're not being an asshole when they point out edge cases and ugly code, but that they're being an asshole to future them when they have to go back and add a feature, fix a bug, or handle a corner case they'd missed.

[mmcconnell1618]

Try taking power away from the reviewer to disarm the defense reaction. In Ed Catmul's Creativity, Inc.

(http://www.amazon.com/gp/aw/d/0812993012/ref=mp_s_a_1_1?qid=...)

he describes the Pixar "brain trust" which reviews early progress on movies with the director. They found directors would get defensive if they felt someone more powerful was telling them to make specific changes. Once they set some rules like "the director decides what, if anything gets changed" and "talk about what's not working but don't solve the problem for the director" things improved. Now instead of film veterans telling you your movie (code) stinks it becomes about opportunities to make things awesome.

[halostatue]

If you know that your colleague is sensitive to what appears to be the overwhelmingly “negative” comments, make sure that there are positive statements. I had a code review recently for one of my guys that had a lot of comments that were style and logic problems.

When I finished the comments on the code, I made sure I added a comment to the top of the pull request (we use BitBucket) indicating that it was clear he understood the point of the code and had written a good first pass at it, but there were a few things that needed to be dealt with for idiomatic code.

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

[dewiz]

Try with design sessions upfront, i.e. discuss how the problem should be solved in terms of patterns and architecture. Once that is agreed, with 2-3 people, then the code review becomes simply a matter of style and there you can only invite for consistency with the rest of the code base. Often the problem is in the tools, face to face conversations, pair programming, human interaction help with that. You can also have your colleague help with a task of yours and show that you appreciate his input.

[supercanuck]

Try a compliment sandwich... Sandwich the criticism between two positive statements.

e.g. I like the way you did this, but I think this would work better but you're on the right track.

[Cymen]

Have you considered going out to lunch with them or grabbing a coffee and bringing it up? If you're in an organization where you wouldn't be comfortable with that, maybe talk to your manager and ask them about it? Basically, we all have to toughen up a little bit when getting reviewed and it is for the good of the project/team. So getting someone who is uncomfortable past that is a win-win for everyone.

[Blackthorn]

Sounds like he told you exactly how you should deal with it. Stop focusing solely on on the bad things and mistakes: make sure to mention "hey, neat idea" or "this is pretty clever!" on parts where, you know, it is exactly that. Diplomacy is a useful skill.

[tomjen3]

Sounds like he is too attached to his code - maybe make it abundantly clear that you are critiquing his code not him. If you have the ability to do so, try to have him fix bugs in other peoples code (that way it might feel less like his code).

[cpitman]

I'm assuming you are performing asynchronous code reviews (like pull request comments). If you have the time, one of my teams did occasional "Code Inspections", where a team of 5 people would review a larger chunk of functionality together.

The key is that each person has an assigned responsibility, and that one of those is the "Reader". The reader presents the code for review, and the reader is NOT the author. The author is there, but I think it helps when someone else is presenting the code to reduce the feeling that the review is of the author.

[Too]

Ask your manager to make the review a formal and required part of the process, for everyone. That way everybody is treated equally. If you go ask your colleague out of the blue why his code looks the way it does he might get defensive and wonder why you were suspicious to review his code behind his back.

[meejah]

I have, and it was more an attitude/cultural thing: the reviewee took any criticism very personally, and was hence super defensive and unlikely to change anything anyway.

Honestly, I think it's actually really, really hard not to react that way (to varying degrees) especially as most code-reviews basically come at the wrong time: you've already written the code, it works, it has glorious unit-tests -- who cares if it's a for vs while loop or whatever. It's actually even worse if there are serious design or maintainability issues, as that's even more code to re-write/"fix".

Personally, I really like having code-reviews -- it's better to get rid of serious problems if they're there. However, I've found a lot of reviews to be rather worthless "rubber-stamp" exercises -- especially if the dev team has just been told Thou Shalt Do Code Reviews Because Reasons! (I've also found the opposite, for sure: getting nice solid advice or missed cases; but these are invariably with other developers who want reviews no matter what management said).

...but all that said, I think so-called "code reviews" really work best if they're spread out over the coding (unless it's really small chunks at a time) -- a bit of pair-programming (or "hey, can you help me with ...") and design (or whatever you call "early on in the cycle") reviews make it a hell of a lot easier to take a different approach.

I think they used to call this "collaboration"?

Companies that just blast out a "new rule: everything gets code reviews!" memo without helping people understand the benefits, and how to do more "along-the-way" validation of design, refactorings, etcetc are always the very worst at having code reviews that are in any way effective. At such places, I find "code reviews" devolve into either useless "rubber stamp" affairs or something like what you describe. Or worse.

Also, if you have "Junior Developers" blasting out code for a couple days (or more!) with no adult supervision with a hope that some last-second "code review" procedure is going to equal amazing code, you're doing mentoring wrong. It's a lot nicer for everyone involved if there is someone sitting down with (or chatting or watching-the-commits-of or whatever works) the less experienced developers. These mentors should be providing hands-on, practical advice as they go. Some devs might need more hand-holding, some will need "a plan", maybe you need to sketch out classes or method-signatures for them, perhaps they need a bunch of half-day tasks written down, etc...Such mentoring should, IMO, be what your Senior Developers are spending a decent chunk of their not-coding time on.

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

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

[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!

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

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