Esp. if there is a very large code base and if the team is also doing some level of test-driven development, it is not practical and also does not make sense to do review of entire code-base. In such a case, how do we decide which parts of code to review and whether it is effective at all? This question is also related to 527259.
+2
A:
The parts you're working on now.
Also, the parts with the highest bug frequency (which should be some of the parts you're working on now).
Greg Mattes
2009-11-12 14:12:22
A:
You can use a code metrics tool and run it against the code base and find the most complex pieces of your project that have been worked on most recently.
Greg Andora
2009-11-12 14:12:57
A:
I generally check the contracts of the various classes and methods. Do the methods make sense, are the pre and post conditions well defined,etc. And I check if there are any obvious code smells (http://c2.com/xp/CodeSmell.html). And if TDD is being practiced, it is good to review the tests first, it will give an idea of how the code is working and also if all the conditions are being tested.
Arvind
2009-11-12 14:13:16
A:
I would focus on those areas that are important to the business objectives first, then those areas that are performance sensitive. For example, if your application is crunching data, review the algorithms doing the data reduction for correctness.
Jeff Paquette
2009-11-12 14:13:17
A:
- Code that is used very often in code.
- Business critical code.
Edit With "Code that is used very often" I mean Code that is used often in code. not necessarily code that is used often by the user. There is a big difference since code that is used often is a target for change. Now I'm not saying that code should not be reused. What I'm trying to say is that when code is beeing reused. It deserves more attention and is therefore good to review.
Carl Bergquist
2009-11-12 14:16:08
Though I would estimate the code that is used very often to contain few to no bugs.
ziggystar
2009-11-12 14:20:54
+5
A:
Someone should review every single line of code that changes. If there's too much code in a single change to review, then you're changing too much, and you should consider breaking down the change into a series of smaller changes.
Not everyone needs to review every single change, but every single change should be reviewed by someone. No one writes perfect code.
Dominic Rodger
2009-11-12 14:16:37
Whether this is good or not depends on your environment and your application. If time and money and infinite or "getting it right" is vital (e.g. if financial transactions, or safety depend on it) then yes do this but be conscious that reviewing every change is exceptionally expensive (and can be more so than the odd bug).
Iain Collins
2009-11-12 14:35:20
@Iain, the problem is that you cannot know whether the "odd bug" is going to cause you serious issues until you understand it, and you can't understand it until you find it.
Dominic Rodger
2009-11-12 14:50:12
@Dominic That's why I say it nature of the application. As undesirable as it sounds we all know that there some very widely used applications are horribly buggy (some even crash regularly) but they are very successful, even wildly popular, regardless. Other systems, like billing platforms, are inherently much less fault tolerant.
Iain Collins
2009-11-12 16:12:52
+2
A:
I always start with the parts that are "ugly" in the sense that there's either little commentary, poor variable name choices like xx1 or such, or any place that decides it doesn't want to use encapsulation. Those almost always strike me as places that need to be reviewed. After that, I agree with the "most complex" comment from above.
wheaties
2009-11-12 14:17:50
+1 A simple and to-the-point answer. I think this is an excellent place to start. If none of it is like this, that's great you can maybe move on to the "most complex" / hairy cases.
Iain Collins
2009-11-12 14:43:02
A:
You should code review the areas of code that have changed in the current development iteration/relate to the specific dev case.
e.g. the way it works for me, in an environment where we use Fogbugz: - a fogbugz case is worked on by a developer - the developer does the code changes, checks in to subversion and the files changed get linked in to the fogbogz case - developer assigns the case for code review - code reviewer looks at the files changed for that case/the changes made (listed via the "Checkins" list associated with the case) and checks that the changes made make sense/are correct. - if all OK, code reviewer marks the case as resolved/completed, else assigns back to the developer with comments
AdaTheDev
2009-11-12 14:19:19
+1
A:
One motivation in code review is to find bugs. Another is to expose more developers to parts of your code base they don't normally see. This helps educate junior coders. It also helps to assure that you don't have big problems when a developer leaves - because they're the only person that knew how something worked.
So:
Review the parts of the application that are scary. Especially the parts that you would hesitate to work on, or tell a junior not to work on.
Old programs often have areas written by developers who no longer work at the company. Often this stuff gets really crufty because newer developers won't take the time to understand it before making changes. Review that.
Review all code that is only worked on by one person.
Review anything really complicated.
Scott Saunders
2009-11-12 14:22:48
The point about scary parts or things written by people no longer in the department is well made, as is the one about avoiding that in the first place. However, I don't think it's necessary to review all code written by one person unless you have people who can't be trusted to write reasonable code on their own in the first place (and IMHO it can be best they don't write anything at all or work in isolation from core products - but if you are stuck with people like that then yes, have everything reviewed).
Iain Collins
2009-11-12 15:32:48
My reason for reviewing code written by only one person is so that other people understand the code. Otherwise, as soon as that person leaves, you've got code that none of your developers are familiar with. It's not really a matter of how many bugs you might find, but how well knowledge is shared.
Scott Saunders
2009-11-12 15:49:38
+1
A:
If the team is using TDD, one code metric you can use is code coverage: a class with low test coverage is a solid candidate for review.
A tool like NDepend can help you also visualize both the complexity of various pieces of your codebase, as well as which pieces depend on which.
Mathias
2009-11-12 17:24:43