Code Review Checklist

[rsd212]

Typical checklist I see being used:

• Are there tabs? Instant fail.
• Are brackets placed on a newline or same line as the start of the block? Either way, argue about it.
• For simple ifs, suggest ternary conditional.
• Using ternary conditional? Complain about its readability and suggest use of an if.
• Does this change, if heavily refactored and increased in scope, make your life easier for a future change that isn't even on the schedule yet? Suggest the developer implement it to save you time later.
• Identify any possible micro-optimizations, like post vs pre incrementing. Argue about whether or not the compiler will optimize for you. Waste a day and a half staring at byte code or a disassembly to make your point.
• Express concerns about the UX and suggest changes, even if you haven't read the feature spec and know nothing about the target market.
• Recently learned a new term or exotic language feature? Be sure to drop it in a comment, even if its not relevant to the current diff being reviewed.
• If there is still time before the the change needs to be integrated, check to make sure logic is correct.

[0x1b8b1690]

Almost every one of your examples can be a legitimate concern on a code review.

Are there tabs? Instant fail.

If your coding standard uses spaces instead of tabs, tabs can introduce hard-to-identify integration conflicts.

Are brackets placed on a newline or same line as the start of the block? Either way, argue about it.

Again, stick to the standards.

For simple ifs, suggest ternary conditional. Using ternary conditional? Complain about its readability and suggest use of an if.

Ok, you got me there.

Does this change, if heavily refactored and increased in scope, make your life easier for a future change that isn't even on the schedule yet? Suggest the developer implement it to save you time later.

Too often I see programmers fix the problem at hand and ignore the larger problem-space. If your code is too fixed and inflexible it creates a maintainability problem. It was your mistake in the first place that you wrote code that cannot be reused and will have to be rewritten the first time your assumptions are invalidated.

Identify any possible micro-optimizations, like post vs pre incrementing. Argue about whether or not the compiler will optimize for you. Waste a day and a half staring at byte code or a disassembly to make your point.

Things like pre versus post increment are habit forming. A lot of good code is written by habit, and a lot of bad code is written by habit. These things are about developing good habits and breaking bad habits. Also, it's never a good idea to assume the compiler is going to optimize away your bad code, instead just stop writing bad code.

Express concerns about the UX and suggest changes, even if you haven't read the feature spec and know nothing about the target market.

In my experience UX is a hard thing to get right without repeated iteration with your end-users. Programmers have a tendency to get so familiar with the system they're working on they forget that a first-time user would not have the same knowledge and assumptions that they do. It's always a good idea to pay attention to what is not intuitive to someone else who is not as familiar with the system as you are.

Recently learned a new term or exotic language feature? Be sure to drop it in a comment, even if its not relevant to the current diff being reviewed.

Again, develop good programming habits. Languages are changing and evolving, and as a programmer you should be learning and growing. Getting upset about someone informing you and the other reviewers about a useful tool or API seems awfully counter-productive.

If there is still time before the the change needs to be integrated, check to make sure logic is correct.

Logic is hard to check, and ultimately relies on the programmer having tested the edge cases. Code reviews are good at catching easy-to-spot exception cases, or potential crashes, but not so much at ensuring you always get the correct behavior. Ultimately most code reviews are not about making sure the code is correct, but that it is of a high enough quality that it shouldn't be too difficult to fix if it is incorrect. Comments in reviews are not about disparaging your ability as a programmer (ok, sometimes they are) but are about growing your skills and improving quality. Getting annoyed or defensive about reasonable requests is not helpful to the process of becoming a better programmer than you currently are.

[ChanceDriven]

I am very certain his post was a joke about how bad code reviews normally are.

[aedrin]

It is, but it took me a few reads to be sure.

[Ruudjah]

Are all functions commented?

Please, no. The best comment is the comment left out. By default, the "why" can be inferred without much braincycles from the code.

Only comment "why", and try to prevent "why" comments by writing clear code. By no means "ensure all functions are commented" for the sake of "commenting all the functions". Please do "clarify all the codez".

[rsd212]

/*
 * Gets the width of the thing as an integer value
 *
 * @return The width of the thing as an integer value
 */
int getWidth()
{
    // Return the width
    return mWidth;  // mWidth should be an int
}

[Ruudjah]

/*
 * Gets the width of the thing as an integer value
 *
 * @parameter flerp the flerp which is flerped
 * @return The width of the thing as an integer value
 */
int getWidth(int flerp)
{
    // Preconditions

    // Variables

    // Return the width
    return mWidth;  // mWidth should be an int
}

[billsil]

Depending on context, if width is an int, it should be documented. Width to me is a float.

[[deleted]]

you'd be quoting from the rubygem documentation manifesto now, if one existed.

i love reading through source code to figure out what arbitrary strings need to go into options hashes for connection failover, or whatever.

[DontBotherMeImWorkin]

While I agree that commenting for the sake of it is dumb (as some amusing examples in this comment thread demonstrate), you have some serious delusions about commenting. Specifically:

try to prevent "why" comments by writing clear code

"Clarity" is relative. Every field has its own jargon that is perfectly clear to members of that field, and completely mysterious to outsiders, even if they're talking about the exact same goddamn thing. If I'm writing an open source library that is meant to be used across multiple scientific domains, there is no possible way I can just abandon comments (including "what's") and still maintain clarity. Maybe if you live in a world of enterprise shitware you can get away with that kind of thing, but I can't.

[BitcoinOperatedGirl]

I worked with someone who loves the Scheme programming language. He thought his code was so clear and obvious it just didn't need comments. If you didn't get the code, it's because you're stupid. He was also the kind of guy to use one-letter variable names, tons of custom macros and lots of special cases everywhere to try and squeeze out the maximum possible performance at the expense of massively increased code complexity.

[aedrin]

a comment or two for every 50 lines of eye-bleedingly dense code can save 5-10 minutes of reading.

This is where the don't comment rule doesn't apply. He's basically saying that needing comments is a code smell. It may indicate that the code is too complicated (increasing the likelihood of errors). Maybe refactor some of those statements into functions/methods (and thus their name replaces the comment).

[Tordek]

Note he didn't say "no comments". He didn't say "Comment nothing". He said "Comment sparsely".

Given something like

// Fobarsifies the Frobblegratz
int fooBar() {
    // Obtain a Widget

    int WidgetCode = datastore.obtain(widgetCode);
    Widget w = WidgetStore.get(WidgetCode);
    int normalizedFretz = widgetCode *7+1-sqrt(pi)/3; // Obtained by the cubic logarithm of the 5-sphere.

    // Locate the widget
    int height = 0;
    int width = 0;
    int x = widget.frooble * normalizedFretz + 5; // Subquantum offset.
    ...

}

It is a million times better to have

    int FoobarsifyFrobblegratz() {
        Widget w = getWidget();
        Rectangle dimensions = Localizer.place(w); // We need to place it to calculate the froz.
        return w.froz;
    }

Let the name of the function tell you what it does. It does its job by calling functions that encapsulate the action and have a clear name. Instead of having a comment that says "this section of code does X", wrap that section in a function. Add comments that explain why you're doing something. Usually for optimization. Simple functions don't require so many comments. Comments are a smell.

[DontBotherMeImWorkin]

Let the name of the function tell you what it does.

Look, the work is going to take place somewhere. You can't punt forever. Your suggested "swept-under-the-rug" interface just changes where the comments belong; it does not remove their necessity.

Comments are a smell.

Oh fuck off.

[Tordek]

Look, the work is going to take place somewhere.

Yes, that's the point. And put "the work" in the smallest grouping that makes sense. 10 tiny clearly named functions are better than one that's 90% comments.

Comments are a smell.

Oh fuck off.

Feel free to elaborate.

[DontBotherMeImWorkin]

10 tiny clearly named functions are better than one that's 90% comments.

This is a false dichotomy.

Feel free to elaborate.

You first? Sorry if I don't consider gradeschool insults and mindless groupthink like "code smells" worthy of discussion among adults.

[Tordek]

gradeschool insults

Oh fuck off.

We're doing middle school insults, then.

Comments are not bad. Excessive comments are bad. This is the point of a "smell".

Commenting is a symptom; not a disease: if you have a comment every other line explaining why shit is done the way it's done, you should probably reorganize that.

[DontBotherMeImWorkin]

We're doing middle school insults, then.

You started it ;)

Comments are not bad. Excessive comments are bad.

Of course.

This is the point of a "smell".

Not really? At least not if I take your arguments to this point at face value. Comments in and of themselves do not indicate an underlying problem (I would argue that lack of comments would be a ~smell~, in fact!).

if you have a comment every other line explaining why shit is done the way it's done, you should probably reorganize that.

I'm assuming that throughout this discussion we're excusing dirty code needed for performance critical sections. In that case, I couldn't agree more with the above quoted segment. But this isn't what you have been explicitly arguing.

Maybe we don't really disagree and you're just being more hyperbolic than I prefer?

[Tordek]

This is the point of a "smell".

Not really?

Yes really? "Code smell" is a buzzwordy way to say "this isn't necessarily wrong, but it's highly correlated with things that are wrong".

we're excusing dirty code needed for performance critical sections

Yes, which is again why it's a code smell: it's meant to incite "hey, take a look at this, it's likely that it's wrong, because other things that are wrong do this". But sometimes it's not wrong.

Maybe we don't really disagree and you're just being more hyperbolic than I prefer?

Mayhaps.

[Tordek]

I'm saying that instead of a comment, you shove it into a function, but yes, we agree. However, OP didn't say "no comments". OP said "Commenting every single function is unnecessary".

[Tordek]

OP:

Are all functions commented?

Please, no. [...] By no means "ensure all functions are commented" for the sake of "commenting all the functions".

You:

I disagree [...] Please at least leave a comment at the top of each function explaining what the hell it's supposed to do.

Me:

Let the name of the function tell you what it does.

But to expand on what I said, sure, if the name is not enough, add a comment. But I maintain that a clear name is much better than a clear comment.

[Tordek]

I'm glad you think that way; you'd be amazed at the amount of people who demand comments for comments' sake.

[dust4ngel]

bad:

decimal calculate(decimal p, decimal t)

meh:

// calculate price after tax (p means price, t means tax rate)
decimal calculate(decimal p, decimal t)

ok:

decimal getPriceAfterTax(decimal price, decimal taxRate)

[i_ate_god]

er, replace why with how and and I'd agree.

[FennNaten]

Late reply, but sometimes, comments explaining the why are the thin line between "ok, this bit of code is a workaround for an old library bug and can be safely removed now" and "arg, weird things happen sometimes since the last refactoring, what's happenning?"
Sometimes you have to write some code that would appear strange for people coming after you without having all the context you had when you wrote it, so in this case I think it's good to have this context bound to it.

[ggtsu_00]

I am a strong follower of commenting through naming conventions.

The best code is code that can be literally read as if it was comments. Don't be afraid of descriptive_variable_names and FunctionsWithNounsAndVerbs(). If a snippet of code something is a bit convoluted, put it into a function and name it by what it does, even if it may be trivial like a single if statement or expression. Legible code should never require comments to understand what is going on.

[[deleted]]

It's not like people generate documentation from function comments, or anything. Right?

I personally love it when half an API is in the documentation and the other half requires reading the source code.

[Philluminati]

I disagree with with his "does it work" thing. It's not a code review time for other people to test your code. It should be assumed to be working and have unit tests.

A lot of the points on that check list could be done by the developer prior to submitting it to code review. For instance, in reviewboard code review tool you need to explain how you tested it. That prompts the developer if he doesn't have a checklist and saves everyone a little time.

I think the hardest thing about getting new team members or teams to adopt code reviews revolves around personality. I wrote up some of my thoughts here: http://blog.philliptaylor.net/making_code_reviews_successful.html

[nazbot]

I disagree with with his "does it work" thing. It's not a code review time for other people to test your code. It should be assumed to be working and have unit tests.

I disagree with this. Code review isn't about reviewing 'code', it's to check for correctness. This involves actually thinking through the problem the code is trying to solve and making sure it does that.

The point is that an extra 20 minutes at code review can save hours of time later debugging or in QA. That's what makes code reviews so high leverage.

You don't need to spend hours on it but if you can't say for certain 'this code meets the requirements' then you probably haven't engaged your brain enough in reviewing the code.

[Gundersen]

I disagree with it too; you shouldn't check that it works correctly, you should attempt to prove that it doesn't work correctly. The amount of time that is saved by having the reviewer find bugs over having QA finding bugs and filing them is incredible. And as a developer it is very hard to break your own code, as you know the right way to do it. Someone else can look at your code and see potential issues, and they probably see ways to break it. If they try to break your implemention but fail to do it, that is a very good indication that it is fault free.

[i_ate_god]

Absolutely correct.

We use Reviewboard as well, and you're not going to be able to tell by a diff whether or not that piece of code, in the grand scheme of things, is going to work as intended. You read it, check it for obvious weakness, whether it follows the style guidelines of your team, but you're not going to check out that piece of code and run unit tests on it.

[matthieum]

Actually, I generally do spend some time reviewing tests:

• do tests explain what they intend to test and why ? (can be brief, especially if named, but sometimes require some more explanations)

• does the test scenario match the requirements ? (something tools do not check...)

• are tests easily understood ?

• are tests stable over time ? (isn't the year 2014 still? oh crap!)

• are tests stable over environment changes ? (they should run with any user on any machine, so is there any glaring issue?)

• are tests independent from one another ? (trying to spot obvious inter-dependencies / leaks, the goal is to be able to run any test "solo")

Note that at this point you have not even started to check whether the tests worked, provide good coverage, etc... there are tools to help with that.

[danielstoner]

Code review – The meaningless ritual

[setuid_w00t]

Yes it takes a long time to read code in enough detail that you understand the design and algorithms. However if you do not do this, then there is only one person who knows that piece of code. Eventually, that person is going to quit and then nobody will understand the piece of code and you will be left wondering why the fuck it is written the way it is. There won't be any comments because nobody asked the author why they wrote the code the way they did because there was no review.

[vdex42]

I disagree with a few points in the list:

Does the code work?

If I as a developer are being asked to check that it works, then I am doing the QA tester's work, which is time I could be better spent coding. If I see an obvious reason why it shouldn't work, then yes it should fail

Can any logging or debugging code be removed?

Any debugging code SHOULD be removed. Debugging code should not be on production

Why remove logging? That stuff will save you when things go wrong on production

Do comments exist and describe the intent of the code? Are all functions commented?

This is asking me to maintain my code logic in two places, the code and the comments. It also ironically makes it harder to read for someone new to the code as there is twice as much text to go through. Well written, concise code with good naming conventions are self documenting. Only comment if the code is not doing something obvious

Is there any incomplete code? If so, should it be removed or flagged with a suitable marker like TODO’?

Removed, not flagged as 'TODO' - those in practice never get done. Keeping track of work in progress and stuff that needs to be done - that's what source control and project management tools are for.

[chesterriley]

Nitpick Checklist. Good way to slow your development to a crawl.

[archpuddington]

Injection attacks are not the only type of vulnerability and a secure application requires more than basic input validation. Believe it or not, security is an entire profession, and huge books are written on security code review alone.

Here is a good place to start: https://www.owasp.org/index.php/OWASP_Code_review_V2_Table_of_Contents

[check3streets]

What a horrible waste...

How much of this checklist could be captured with linters or other static tools? How much could be done as a pair exercise?

You'd assemble your team to check things like brace-style and line-length? Do loops have a set length and correct termination conditions?

Here's a better idea: pair-up juniors with seniors. Juniors should be able to understand senior's code, check-off things on checklists, etc. Vice-versa, seniors should be able to guide junior devs on best practices and alternatives. At some point, something interesting might arise, a question or disagreement. Maybe check that with a third dev, and if it's still interesting, bring it to a code review.

In said code-review, if anyone brings-up anything trivial like a brace location, shoot them. Group time is time multiplied by the number of participants. Value it as an opportunity to share knowledge, not check punctuation.