Software Engineering Stack Exchange is a question and answer site for professionals, academics, and students working within the systems development life cycle. It only takes a minute to sign up.
Sign up to join this community
We're currently modifying out development process and I'm wondering if we should try to keep a 100% of our commits peer reviewed.
What is your experience regarding code reviews?
We have a 'Code Review' task in each story. Someone ideally not involved in the development of that story will review all code changes associated with that story. It works well.
A lot of time? Not very much, depends on how much code - we're looking for obvious errors, typos, basic logic sanity checking, uncaught exceptions, etc.
It's a quality step that does find bugs, therefore it has some value. Allocating time may not be the best way of doing it - how about if something is fairly complex, it should be code-reviewed?
By the way, it's important that someone else does the code review..
An issue more important that how much over your code is covered by reviews, is how effective are the reviews. If your reviews discover few or no issues, then reaching full coverage will be useless.
First work on making your reviews more effect, then decide on the coverage.
Reviews should be performed not only on code, but also on design.
Also, reviews are no replacement for tests and tools:
Try dedicating a preset amount of time per month (or per sprint) for reviews.
Select the code you want to review at the next dedicated slot using a heuristic such as:
And remember, you are reviewing code (or design or tests) and not authors.
I recommend the following reading materials:
Selective Homeworkless Reviews
Best Kept Secrets of Peer Code Review
It depends.
It depends on what your software is doing:
If it controls an electronic pacemaker or a space shuttle, then definitely yes.
If it is a throwaway prototype, then probably no.
It also depends on how well resourced you are, how experienced your developers are, and what you are looking for in code reviews. (Bear in mind that the average developer reviewing someone else's code is probably going to notice style issues and miss subtle algorithmic bugs ... especially given that code reviewing is something of a chore.)
My advice would be to save your code-review effort for code where correctness is critical and the cost of undetected errors is high.
First, you need to answer this question: Why do you review code?
With that answer in hand, you can figure out which code needs to be reviewed.
Some code reviews accomplishes exactly what testing does or would have done. If that is the goal of your reviews, then getting closer to 100% is a good idea if you have little testing. However, letting test tools do this would reduce the need for all of the code to be reviewed.
Most good reviews seem to be focused on sharing knowledge and increasing the capabilities of the developers in the review (either the one who wrote the code or the ones reviewing the code). With this as a primary reason for reviews, making sure to review 100% of the code is probably overkill.
In a perfect world, everything would be explicitly read by the author and peer reviewed by at least one other person, from requirements specs to user manuals to the test cases. But reviews, even simple desk checks, take time and cost money. This means that you need to choose what you should review and when you should review it.
I recommend prioritizing things to review, choosing how you want to review them, and trying to review as much as you can with the appropriate level of detail. Prioritiziation could be based on the type of artifact, such as stating that requirements must be reviewed, design and production code should be reviewed, and test cases can be reviewed. Within that, you can also specify that high risk or high value components receive a priority in review, or perhaps a more formal review.
As far as time, it all goes back to how high of a priority the component is. There have been times where I've spent 10-15 minutes reviewing, and other times when multiple people have read the code individually then went into a room to do a more formal inspection process that lasts 30-45 minutes (depending on the size of the module).
In the end, it's a balance between time, cost, scope, and quality. You can't have them all, so you need to optimize where you can.
As a suggestion, if you plan to do any reviews at all, have some shared guidelines about the review scope and goal to make sure that reviews don't cause needless frictions between team members.
Coherent teams build better projects. People might loose relationships over nonsense or over perfection requests. There is always that one person who would complain about this or that and bug others just because he is like that...
I reserve an hour per day for doing peer reviews, but don't always require it. Our code base is shared among a couple dozen products. Our policy is that a trivial change in code unique to one product is okay to check in without review. More complex one-product changes require a review, but it might be as informal as calling a colleague to your desk to give it a once over. Changes in shared code require a more formal review, including developers on other products. I think our policy strikes a fairly good balance compared to other companies I've worked for.
I spend more time per day on reviews than some of my colleagues with less central roles, but I don't consider it an unreasonable amount of time, because before the review policy I could easily waste more time than that tracking down bugs that a developer on another product introduced.
We've done 100% reviews for code. It's far cheaper than testing, especially 100% code coverage testing. We don't spend too much time on them, reviewing for more than an hour per day becomes less productive. (30 minutes is not a lot).
As you're zeroing in the process, keep notes. What did you find? What did QA find later? What did your customers find? Why did those bugs escape you?
Have regular code reviews mostly for team building and sharing ideas on implementation. You can learn a lot from your coworkers this way.
We require a peer code review for every check-in. If no peer is available we arrange for a post check-in review. The reviewer is noted in the source control check-in comment.
These don't take a lot of time, and since they are done between peers there is no toxic adult-child aspect to them.
Code Review is, IMO, needed. You're 99.999...% of the time not always going to be right, so you need to make sure it is correct. Do I have a set time? No. But I do take the time to check over my code. Usually I have a colleague do the same.
Code reviews may seem daunting, but they are a valuable tool when conducted properly. They will be your first line of defense against design and implementation mistakes. If you aren't conducting code reviews on every feature you put into place, you should start ASAP.
As for how much time to spend on peer reviews, a good practice is to leave 5-10% of your total estimated development time for conducting and responding to the code review.
We have a whitepaper on conducting effective code reviews that can help get you started on the right foot. It is a step by step guide and discusses common pitfalls you might face and how to resolve them. You can download it from our website.
