80

I am in the process of trying to sell my organisation on the value of code reviews. I have worked at several places where they were employed. I have seen them used to nitpick styling choices, and functional decisions, and I have seen them used as nothing more than a gut check to make sure nothing dangerous is being implemented. My gut feeling is that the most effective purpose is somewhere between the two options.

So, what is the purpose of a Code Review?

SoylentGray
  • 3,063
  • 16
    - How would you know if you've written readable and easily maintainable code? - Your peer tells you after reviewing the code. _'Rationale:

    You cannot determine this yourself because you know more as the author than the code says by itself. A computer cannot tell you, for the same reasons that it cannot tell if a painting is art or not. Hence, you need another human - capable of maintaining the software - to look at what you have written and give his or her opinion. The formal name of said process is "Peer Review".'_

     – gnat Sep 10 '14 at 20:16
  • 3
    "What is the purpose of a code review?" to prevent developers from writing terribad code, and to steer them in the right direction. – zzzzBov Sep 10 '14 at 21:09
  • 7
    Seems [codereview.se] might have an indirect answer to this question.. just browse at the questions and answers there, the purpose of a code review becomes pretty apparent :) – Mathieu Guindon Sep 10 '14 at 23:09
  • You may find that the organization may see the value of code reviews already, but they're going to be more concerned with how it will be: implemented, training, tracking, and amount of time spent on it. – JeffO Sep 11 '14 at 13:30
  • 1
    side note observing some of the recent answers made me recall an old MSE discussion: Lots of not-always-useful but well-intentioned answers – gnat Sep 11 '14 at 14:41
  • 3
    I wonder how many times programmers have discovered bugs in their own code just through the simple process of explaining their code during a review? –  Sep 11 '14 at 15:00
  • 1
    @hatchet Rubber Duck debugging is vastly helpful indeed. – Eben Sep 11 '14 at 19:33
  • 3
    @Eben - true. But I was thinking about cases where the programmer thinks the code works great, and discovers a previously unknown problem while describing the code. I know it's happened to me. And if you don't have a person handy for the review, then Rubber Duck Code Reviews. –  Sep 11 '14 at 20:06

4 Answers4

80

There are multiple reasons why you would want to conduct a code review:

  • Education of other developers. Ensure that everyone sees the modification associated with a defect fix or enhancement so that they can understand the rest of the software. This is especially useful when people are working on components that need to be integrated or on complex systems where one person may go for long periods without looking at certain modules.
  • Finding defects or opportunities for improvement. Both the deliverable code as well as test code and data can be examined to find weaknesses. This ensures that the test code is robust and valid and that the design and implementation is consistent across the application. If there needs to be additional changes made, it catches the opportunity closer to the point of entry.

There are several business cases for conducting reviews:

  • Finding defects or issues that would need to be reworked closer to their injection. This is cheaper.
  • Shared understanding of the system and cross-training. Less time for a developer to come up to speed to make changes.
  • Identification of possible enhancements to the system.
  • Opening up the implementation to ensure that testers are providing adequate coverage. Turning a black box into a grey box or white box from a testing perspective.

If you're looking for a comprehensive discussion on the benefits of and implementation strategies for peer reviews, I'd recommend checking out Peer Reviews in Software: A Practical Guide by Karl Wiegers.

Thomas Owens
  • 82,739
  • 7
    +1, I think you spotted the main points well, with the correct priorities. Keeping the design consistent when dealing with coworkers who constantly find very creative "WTF" solutions can only be achieved by regular code reviews. – Doc Brown Sep 10 '14 at 20:41
  • We do code reviews in our JavaScript code, mainly to ensure the developers stick to the outlined standards, using the set out pattern when designing modules, using the components provided and not start doing ninja coding (intentionally or not) around problems we already have solutions for. They also are great to spot someone accidentally over-writing this context, not using .hasOwnProperty in places it should be in, etc, etc. -- So, mainly for standards. In a managed language like C# you off course have several less reasons then for dynamic languages. – Nope Sep 11 '14 at 16:42
  • 2
    Thus far your answer only alludes to improvements to correctness of code. It fails to address readability/maintainability of the code, which can rarely be accurately quantified by the developing dev. – ArTs Sep 12 '14 at 06:15
53

Code Reviews are a tool for knowledge transfer.

  • When developers review each other's code, they gain familiarity across all areas of the system. This reduces a project's bus factor, and makes developers more efficient when having to do maintenance on a part of the system they didn't write.

  • When a junior programmer reviews a senior's code, the junior programmer can pick up tricks otherwise only learned through experience. This can also work as a corrective against overly complicated code.

    A thorough code review will require frequent checks against various documentation. It's a great way to learn a language or API.

  • When a senior programmer reviews a junior's code, this is an opportunity to iron out issues before they translate into technical debt. A code review can be a good setting for mentoring junior programmers.

Code reviews are not about:

  • … finding bugs. That's what tests are for. It will still frequently happen that a code review finds some problem.

  • … nitpicking on style issues – settle for one style, and use automated formatters to enforce it. But there are many things which an automated tool cannot check. Code reviews are a good place to make sure the code is sufficiently documented or self-documenting.

amon
  • 134,135
  • 2
    your last point on nitpicking style issues I don't entirely agree with - we've just had a harrowing experience reviewing code of a junior developer & the most glaring complaint was actually on style, but not the kinds of style problems that are easily programmatically enforced.... waaaaaay too many if statements for edgecases etc; issues that yes you could make a computer find in some cases, but most were not issues worth generically finding by script. Takes 30 seconds of reading for us to start seeing it, another 30 to explain to the dev & hopefully ammend the issue. Still in shock at it :/ – pacifist Sep 11 '14 at 01:22
  • 1
    I have several colleagues who frequently cite significant numbers of functional defects being located in code reviews. At very least it is a happy side effect. – Gusdor Sep 11 '14 at 08:25
  • 7
    @pacifist That's not the kind of style the answerer is describing. Style is about locations of braces, indentation and so on. If your junior developer is using too many if statements you have an entirely different problem from style; a general attribute of coding STYLE is that it doesn't impact performance. And I think that a significant amount of if-statements will impact performance. – Pimgd Sep 11 '14 at 08:47
  • 18
    When doing a review, I often spot things where I think "this looks like a bug", then I write a specific test case to prove it is a bug. So one of the many goals of code reviews is finding bugs. That's why I think the viewpoint "either code review or tests" is a little bit too single-edged. – Doc Brown Sep 11 '14 at 09:23
  • @Pimgd: In addition, when following SOLID I would imagine to many if statements could possibly mean a class has to many responsibilities and/or abstraction might be needed to deal with that. Though I'm not sure if that logic applies across all languages evenly or to some at all. – Nope Sep 11 '14 at 16:49
  • 1
    I think code reviews have more of a QA role than this answer admits. Yes, tests should catch everything but they don't always - I've seen too many tests that can't fail because they're checking the wrong thing. Until things like coverage tools get much smarter, having a fresh set of eyes inspect code and tests before you ship is a good idea. – nobody Sep 11 '14 at 17:34
  • +1 for knowledge transfer; IMO this is the greatest benefit. – Russell Borogove Sep 11 '14 at 17:49
  • 2
    In addition to @DocBrown there are cases which cannot be easily testable - data races, some types of deadlocks, livelocks, undefined behaviours/values (mostly in C/C++ but the order of elements in hash tables in undefined as well) or resource leek (opening file in a loop might be a bad idea even with GC). Some of those things can be detected by sufficiently smart -compiler- static analysis. – Maja Piechotka Sep 11 '14 at 22:52
  • @Pimgd it's disingenuous to claim if statements are not a style problem, just a performance one. In my case the cascading if statements will have actually been faster (quick-fail-early) but the readability is horrible since the reader has to mentally cache parts as they scroll through. Damien's PBP (the perl style guide) is quite clear about 'Avoid cascading if'. Heck try and read the code here http://www.beenishkhan.net/2012/05/27/if-statements-are-bad-for-code-avoid-them/ ... and yes I'm talking about Perl. I can hear the chatter of keyboards starting jokes already. – pacifist Sep 12 '14 at 00:57
  • @pacifist You're still stuck on the different kind of "style". There's syntactical style and semantic style. Syntactical style is the position of braces and such. Semantic style is how you translate a solution into code; such as the use of a large amount of if-statements. A Code Review is not for determining issues in syntactical style. It is intended for determining semantic style. Both these points are present in the answer: you'll find semantic style matches DO point 3. You'll find syntactical style matches DONT point 2. – Pimgd Sep 12 '14 at 07:00
  • 2
    @pacifist your specific example would get totally blasted in a code review. It would also be a red flag for any static code analyzer (Cyclomatic complexity 17!). A code review would quickly identify this function as a problem in semantic style (or even the algorithm!). However. This kind of issue is not just a "style" issue. If you treat it as such then you'll have some really nasty code in your repository real soon; it's just "style", after all. – Pimgd Sep 12 '14 at 07:11
  • 1
    @DocBrown - I do not read this as saying that if you find a bug or problem dont say anything, rather that the focus of the review should not be finding bugs. – SoylentGray Sep 12 '14 at 14:09
14

The most valuable thing I personally get from a code review is confidence that the code is clear to another person. Are variables clearly named? Is the purpose of each chunk of code reasonably obvious? Is anything ambiguous clarified with a comment? Are edge cases and valid values for parameters outlined in comments and checked-for in code?

  • 2
    @gnat: The other answers address knowledge transfer and spotting bugs. These are important, but there's more to a code review. –  Sep 11 '14 at 15:03
  • 1
    as far as I can tell, there's-more part is reasonably addressed in this answer: "If you're looking for a comprehensive discussion on the benefits of and implementation strategies for peer reviews, I'd recommend checking out Peer Reviews in Software: A Practical Guide by Karl Wiegers." This answer also covers it: "a corrective against overly complicated code" – gnat Sep 11 '14 at 15:11
  • 2
    @gnat It emphasizes a different aspect of the points raised in other answers. Have you ever been puzzled by your own code that you wrote six months ago? Code review lets you accelerate that process. If your colleague is puzzled by your code, you can still clarify it while the problem is still fresh in your memory. – 200_success Sep 11 '14 at 18:46
  • 1
    @200_success maybe. But this point, if it's there indeed, looks really poorly presented. Even your comment manages to communicate it better than this "answer". Not to mention that it just repeats what was pointed in a prior comment that refers to canonical answer explaining this in a related question – gnat Sep 12 '14 at 07:06
  • @gnat This answer offers something that none of the other answers or comments include at all (AFACT)... "confidence that the code is clear to another person." This is the most important virtue of good code, and the most important information for a dev to receive. It can only be reached by having someone else review it. And, for the dev to know this, it must be communicated using a positive statement - a lack of negative statements is not sufficient. No news is not good news. –  May 24 '17 at 14:03
11

I'd like to add two areas not covered by the other great answers:

One great reason for code reviews is the Hawthorne effect which in our case translates to: If you know someone is going to look at your code afterwards then you are far more likely to write it better in the first place.

Another great reason is for better secure development practices. One only needs to look at Apple's goto fail (an accidental duplicated line of code) or the Heartbleed bug (a basic failure in input validation) to understand the importance of proper code reviews in a secure development lifecycle.

Chris Knight
  • 712