Confessions of a programmer: I hate code review (2010)

Usually the submitter takes my advice, but sometimes I am educated in the process (I usually ask for better comments in this case).

I’m a bit of a code review evangelist, so I’m surprised at the amount of negativity toward code reviews. Some of it sounds a bit self centered, upset that it slows down “thier” productivity or ability to self express through code rather than the real purpose of the reviews- to improve the product and organization as a whole.

The consequence is that nested em specifications are cumulative; 1.2em of 1.2 em of 1.2em gives 1.2^3, while 1.2 rem is fixed, antwhere it is used on the page.

This is not of itself good or bad, it is behaviour. Use it to desired effect.

As examples, I prefer to specify font size within elements (header, nav, aside, article, footer) in rem, font size of contained text (h1-h6, code, blockquote, pre, generally) in em, and width of text elements in em.

This allows for less CSS (most of my styles are a few dozen selectors, often less), as relative text doesn't require respecification, and content blocks are defined in terms of sensible, reader-focused, preferable widths, padding, and margins.

Much of that is personal preference: I design around article text, mostly, not wireframes or pixel-perfect layouts. The designs also tend to be robust.

Examples:

    html, body {font-size: medium; line-height: 1.4;}
    article { font-size: 1rem, width: 40em, padding: 2em 4em }
    aside, nav { font-size: 0.9rem;}
    header, footer {font-size: 0.8rem}
    h1, h2, h3, h4, h5, h6 {margin-top: 3em; margin-bottom: 1em; font-weight: bold;}
    h1 {font-size: 2em}
    h2 {font-size: 1.6em}
    h3 {font-size: 1.4em}
    h4 {font-size: 1.2em}
    h5, h6 {font-size: 1.1em}
    .callout {font-size: 2em; width: 50%; padding: 1em; margin: 1em: margin-left:0; float: left: clear both;}

With simple DOMs, this can be useful. Complex DOMs and stylesheets may benefit from direct specification via rem.

Proper test coverage and human QA to me is the most effective approach to not shipping bugs, because it really doesn't matter to the customer what the code does or doesn't look like if it runs. And I just don't believe that code review is an efficient means to catch that if user is in a state of X and tries to do Y to Z that the app blows up. That becomes most apparent when that code is running in the full context of the application, not when viewed as a git diff. A bug is so often a perfect storm of seemingly innocuous but conflicting/insufficient behavior than it is some block of code that is clearly broken at a glance.

There is something to be said about stylistic and architectural issues and code reviews do give an opportunity for asking questions and driving "better" code. But this is actually just another reason why I don't like code review. "Better" is subjective and there's always another level of "better" to be reached. If I held them to my own personal standards sometimes I'd be asking for a rewrite. So what do I do, suggest that they improve their code, but not to the full extent that I feel it could be improved?

I hate code review.

But why delay the shipment of features/fixes to the customer for any of these reasons?

If code quality and human understanding is the actual rationale, it could make more sense to review code after it has shipped. In that way, it would cease to be a bottleneck for the developers or customers. Additionally, any bugs that appear in production could be accounted for during the refactoring process.

Saying that there is value to a practice is the easy part. We're forgetting to ask when to code review and whether or not code review should ever be a blocker to other processes.