In my experience, most teams in the company review the code of team member with a small amount of code, always less than hundreds of lines. Is it appropriate to review large amount of code, for example a module, when the code is complete and ready to get reviewed once for all?
2
-
6Related (and possibly duplicates): What does a standard code review contain?, How do I choose what code to review? & Should we attempt to review all our code?– yannisJun 19, 2012 at 17:04
-
1Your question implies reviewing "all at once." There is nothing to say that a large module couldn't be broken up into smaller review sessions.– user53019Jun 19, 2012 at 17:34
Add a comment
|
3 Answers
The reason for the small code reviews is to maximize effectiveness.
Studies involving the Personal Software Process have found that reviewers are most effective, with respect to maximizing defects identified in the review, when they review no more than 150-200 source lines of code per hour. Given that empirical data, it becomes a matter of determining how long people can stay attentive. I don't have any empirical data, but I know my mind begins to wander after about 1 hour of reading something. To me, that indicates that code reviews should review less than 150 lines of code at a time.
answered Jun 19, 2012 at 17:13
-
It would take me probably more than a hour to properly review 200 new lines of code. Well, maybe Java code is less terse than, say, Ruby or Python, but I strongly prefer changelists with < 50 severely changed lines. These can be reviewed quite fast, and with less risk of missing a potential bug.– 9000Jun 19, 2012 at 17:37
-
@9000 It depends on your level of review. A formal inspection where each line is read is much slower than a desk check where an individual developer reads at his/her own pace. The familiarity with the programming language and system under development also have an impact. The 150-200 lines/hour is an upper bound. Personally, I find I read code at closer to 80-100 lines an hour when comprehending it and determining how it fits in with the system.– Thomas Owens ♦Jun 19, 2012 at 17:45
It's appropriate if it helps your team create a better product.
Some reasons that code reviews often focus on smaller chunks:
Properly reviewing a large amount of code requires each participant to spend quite a long time preparing, and that can be expensive.
The longer a review meeting goes (any meeting, really), the less people pay attention. Two hours is about as much as most people can stand, and keeping meetings shorter than that (say, an hour or 90 minutes tops) goes a long way toward making sure that everyone is at their best.
It's often not necessary to review every line. One reason to review code is to make sure that everyone is sticking to basically the same set of coding guidelines. A detailed review of a smaller amount of code works better for that purpose than a less detailed review of more code.
That said, if you find that reviewing more code is helpful, and if your team can stand it, then by all means give it a try. It'll help to make sure that there are some ground rules and someone acting as a facilitator to keep the meeting moving along. Also, consider having reviewers submit their comments beforehand so that you spend less time on nitpicky things like improper use of white space and more time discussing important and interesting issues.
-
1Something to note is that the first two bullet points only apply to formal inspections. A desk check at the leisure of another developer doesn't require formal preparation (if they are familiar with the system under development) nor does it require a meeting.– Thomas Owens ♦Jun 19, 2012 at 17:14
-
@ThomasOwens Agreed. Hopefully, an informal check will also be smaller than hundreds of lines.– CalebJun 19, 2012 at 17:58
If you review a large amount of code once it's already complete (and presumably functional), this is going to turn more into a session of "we should have done..." than "let's fix/change this to do..."
So, if the primary purpose of the activity is as a post-mortem learning exercise, then the large code review makes sense. If it's to try to catch defects, it probably won't work very well. Small defects are likely to be missed as everyone in the review gets bored and tired, and large, structural defects are likely to be skipped since, well, "too late now -- code's written and customer wants it yesterday."
answered Jun 19, 2012 at 17:44
