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?

Improve this question
8
  • 17
    - 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
    Commented Sep 10, 2014 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
    Commented Sep 10, 2014 at 21:09
  • 7
    Seems Code Review 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
    Commented Sep 10, 2014 at 23:09
  • 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?
    – user95851
    Commented Sep 11, 2014 at 15:00
  • 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.
    – user95851
    Commented Sep 11, 2014 at 20:06

4 Answers 4

Reset to default
81

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.

Improve this answer
3
  • 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
    Commented Sep 10, 2014 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
    Commented Sep 11, 2014 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
    Commented Sep 12, 2014 at 6: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.

Improve this answer
12
  • 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
    Commented Sep 11, 2014 at 1:22
  • 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
    Commented Sep 11, 2014 at 8: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
    Commented Sep 11, 2014 at 9:23
  • 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
    Commented Sep 11, 2014 at 22:52
  • 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
    Commented Sep 12, 2014 at 7:11
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?

Improve this answer
5
  • 2
    @gnat: The other answers address knowledge transfer and spotting bugs. These are important, but there's more to a code review.
    – user25946
    Commented Sep 11, 2014 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
    Commented Sep 11, 2014 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
    Commented Sep 11, 2014 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
    Commented Sep 12, 2014 at 7: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.
    – user251748
    Commented May 24, 2017 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.

Improve this answer

Not the answer you're looking for? Browse other questions tagged or ask your own question.