I'd still rather earlier code reviews... design reviews about 1/3 of the way into writing the code. Enough time to have passed to have discovered the dragons in the whiteboard design, but not enough to have written code that could only undergo minor fixes in a code review.<p>I prefer the idea of a code review happening at a time that it could still steer the ship... too many code reviews catch bugs, but don't correct poor system design. That's also where the greatest benefit/education exists for the author... not minor changes, but "what about approaching it like this".
What often goes unmentioned in praise for code review processes is their insanely exorbitant costs -- measured in engineer hours but perhaps more costly is all of the blocking and impedance [1]. Most of the "problems" that code reviews claim to address can be solved by much more direct and optimal measures. Code reviews are damn expensive.<p>This post concedes that code reviews are better for the more fluffy ends -- teamwork, openness, social recognition, but given their high costs, I'd rather achieve even these soft goals in other ways than to impede my team's delivery potential.<p>While mission critical systems deserve the whole kitchen sink thrown at them, expensive verifications, code reviews, etc etc., most business applications would do much better optimizing for better software architectures and domain conceptualization than spend so much time dwelling on the minutiae of lines of code.<p>[1] Continuous integration and refactoring, pillars of agility, go out the window in typical code review environments where commits are blocked until peer review.
While I value many of the same things as the author, I've found code reviews to be far inferior in every respect to pairing (especially promiscuous pairing (google it)), and to have negative effects in several important ways:<p><pre><code> * They delay integration.
* They tend to encourage focus on abstract code polishing without proportionality or relation to the value of the code.
* They favor superficial improvements, while increasing the costs sunk into paths that may be deeply flawed. (They facilitate late code-structural feedback, but not early directional/problem-analytical feedback.)
* They often discourage more collaborative work and therefore quicker and richer feedback, by their presence as a substitute for pairing.
* They create impediments to work moving quickly to completion.
</code></pre>
Of these, the most egregious is the distraction from value. Teams that are spending a lot of time and energy talking about code quality (which is absolutely important, but not primary...the best teams I've seen maintain a high level of quality and talk about tradeoffs involved in delivering value at high quality) are often neglecting communication about where value lies and how to deliver it most efficiently.<p>[edited for formatting]
Code reviews can easily become a tool for people with huge egos to prove their smartness. I get code review comments for grammar of my comments or very small code style preferences that Google's anal style guide can't enforce (yet).<p>I like code reviews, don't get me wrong. But there should be a way to respond with "you just shut up, you're only trying to make yourself look smart".<p>It's all because higher up people mostly look at code review conversations this is happening. The other day I asked my peer how do I write this code? A or B? He said I don't care, A seems fine. Then in code review he commented it should be done in B way.<p>It's all politics.
I would go further and say that you should never trust code reviews to catch bugs. They're great for all the reasons in the article and what "zhemao" said but they're horrible for catching any bugs beyond the most trivial ones. Humans simply aren't good at running code in their head like that. Code review is no substitute for good code coverage and automated testing, preferably pre-commit.
I think a benefit not mentioned in the article is that it makes sure that at least one person besides the original author understands what the code does.
I'm a dev with one year's experience, and recently moved to a team that reviews every PR. The benefits to me personally are super clear. #1, more experienced engineers are giving me frequent feedback, and that's obviously worth a ton. #2, it's part of my job to read other developers' code. I get exposed to patterns and design choices that I may not have in my repertoire.<p>Maybe the feedback I give the other direction isn't yet as valuable as what I receive on my PRs, but I trust that eventually it will be.<p>I totally get the posts pointing out that during crunch time, folks do pretend reviews, and the process becomes busy work. I think that's a symptom of other problems though (staffing model, etc), and not necessarily a shortcoming of peer review in general.
As several other commenters have pointed out, code reviews are not a silver bullet. It is widely known that there is no silver bullet.<p>That said, if your goal is to make a quality product, you shouldn't have to choose between code reviews and continuous integration. You shouldn't have to choose between code reviews and code coverage, or manual QA processes. These are all widely regarded as best practices and if implemented "correctly" and appropriately to the team their combination forms a virtuous cycle for code health and team culture.
Code reviews : good programming habits :: sexual reproduction : good evolutionary traits<p>They make them spread much much faster than they otherwise would.
Working at a Gov't IT contractor, I really wish we had the organizational skills/incentive to do code reviews. Very rarely does the code I write get glanced at, much less examined.<p>My instinct, of course, is to solicit code reviews from my peers, but the organizational structure and support tooling are all woefully inadequate. Plenty of projects, even greenfield ones, aren't checked into source control. And of course if I spent time contacting those in my org that have similar skills and could thus review, not only would I have to justify it to my 3-4 managers, but so would the person I solicited for review to their own.<p>Nobody cares about the code quality, and thus it has been a nightmare for me trying to improve my skills right out of school. Here's to hoping I get out soon.
At a software shop about ten years ago I loved code reviews. They were a major way of learning and teaching things. I stopped doing some silly (albiet harmless) habits when other people pointed them out.
How do people handle reviews of highly specialized stuff? We have people who do stuff nobody else on the team understands or at least it would take them a long time of learning to do a real review. I look at a lot of stuff and check if it makes halfways sense. I can look at the coding style but I can't judge the overall design without spending many hours on it (which I don't have. Nobody else on the team has it either).<p>I am starting to think that pair programming may use up less engineering time than doing thorough reviews.
This is a topic close to my heart. I’m a software engineer for Canonical (company behind Ubuntu). Last year I did a study of the total review wait time over a two month period, which came out to be 8683hrs ( <a href="http://lingo.reviews/d3/juju_review_wait_times.html" rel="nofollow">http://lingo.reviews/d3/juju_review_wait_times.html</a>).<p>I wrote up my thoughts around scalable engineering here: bit.ly/1P0YgNo and released an MVP solution here: www.lingo.reviews<p>Since then I’ve been refining privately with a handful of engineers from different companies. Two days ago I put in my notice and took on solving the problem of scalable engineering as a full-time mission: <a href="http://codelingo.io" rel="nofollow">http://codelingo.io</a><p>I'd love to connect with anyone that is passionate about this problem (it's been a 2 year obsession for me): jesse@codelingo.io
My employer develops safety critical software with decades of legacy code, long term support for multiple versions, and big customers. In our attempts to transition(in a mock way for the time being) to rolling releases we've "simplified" to a process with 3-5 people reviewing designs, 2-3 developers doing code review/light testing, 2-x(depending on affected modules) quality assurance people doing heavy testing and occasionally we pair program. Documentation/logistics is almost always more time consuming than design and development. I've had 4 character code changes take weeks to get through the process as several people have to find the time to look at it even for just an hour. It's cumbersome but we do catch a lot of bugs, and we're slowly getting better at design.
Code reviews:<p>* signal distrust by default - the work I do is not to be trusted to be merged in and by extension, I'm not to be trusted<p>* can create a culture of passive aggressiveness - you do something that I don't like, I'll get back at you when I'll review your code<p>* not guaranty code quality - having a junior review a junior's code will not yield expert level code<p>Just because Google does code reviews doesn't mean you should, for Google a bug could cost millions, for your project a bug might cost 10$, however the overhead of code reviews might cost more than 100$. It's important to do numbers and think rationally.
What about the stress of continuous evaluation? How does the company view that? Standards are great to maintain, but people aren't robots. I question the value of the word 'review' in this context and its blocking nature.<p>Pair programming is a far more effective tool, but it's not always practical. How about using the terminology of 'collaboration' instead of the test based culture - which I feel turns people into machines - it's just the wrong control structure for people to be happy.
In small/medium teams, especially remote ones, code review helps you keep understanding of the system - you already know what your code does and it's great way to learn the rest of the system.<p>In some sense, it can be looked at, as asynchronous pair programming.
Requiring code reviews before the commit is a bureaucratic waste of time and resources. If they non-mandatory and after the commit, then they can be a good idea.