How do you review large blocks of new code

I'm a strong advocate of code reviews, whatever that means. I know it means different things to different individuals and groups and can be applied differently in different phases of development.

When I make a one-line change to a #define to fix a typo in a user-visible prompt string, "Hey, Joe, did I spell 'FooBar' right?" is easy. When you make a few related changes in several related functions, an over-the-shoulder sanity check is 10-15 minutes' work.

But what about a new project with tens of thousands of lines of brand spanking new code? It doesn't happen that often so I'm not as comfortable with it. How do you review that? One-on-one? Hand off and review "offline" with later feedback? In a group setting?

For the different approaches, are there any rules of thumb for lines/hour reviewed so I can estimate the time to allow for it?

From stackoverflow

• I think it is recommended to do code review before it reaches the thousand of lines of brand new code. After that it becomes the problem you're facing.

  David Thornley : That was the problem with the review process one place I worked. Suddenly, a lot of complicated stuff would be dumped on me or whoever, and it was either a matter of dropping other important things for a few days or rubber-stamping the review.

  Chris Nelson : I agree that early review is better. For various reasons, that's not the circumstance I'm in right now.

• Personally, when I'm responsible for another programmer; I review source code check in diffs as close to daily as possible

  This is providing everything is in a working state. If their changes are not in a working state, I will wait until they are.

  If they take more than 3 days to get a change to a working state ... I take it as a warning flag that needs to be investigated. This is usually something simple like making too many changes at the same time or a major refactoring with too many implications.

  Chris Nelson : I agree for incremental development. But if you're developing a new product from empty files, it may take a few thousand lines before there's anything really functional or reviewable. That's not always the case, you can have a stubby main() on the first day and start fleshing out functions but...

  John MacIntyre : @Chris Nelson-True. There are valid reasons for taking longer; complexity, boot strapping something new, complexity, etc ... The 'warning flag' is just an indicator to confirm there is a valid reason and nothing is wrong.

• If the code is pretty complex, then I like to look through it with the original developer one-on-one. That way I can ask them to explain how it works instead of taking the time necessary to figure it all out by myself. I use a diff tool to see what has changed in multiple files and we walk through the flow together paying close attention to the changes.

  If the code is pretty straight-forward, then I normally look through it by myself. I only call in the original developer if I have questions or concerns. Once again, I use a diff tool to see what has changed and walk through the flow.

  If there is a ton of new code/changes, then I normally only review the critical path and spot check other parts of the code. In this situation, I am not trying to find every thing that might be wrong with the code. My assumption is that testing will catch most of the bugs. My review is mainly to catch any major bugs, to make sure that the code follows basic good software practices and that it will be easy to maintain.

• Ideally you would do a code review when it is being built, so you can provide feedback and reduce issues significantly.

  That said, I would use static analysis tools. That can check for regular issues and even do general analysis on the dependencies in the code.

  And the very first thing would be: does it have unit tests? Start there, check they are well understood, it can reveal tight some coupling issues quick.

  If the code is for an existing system you are working on as a team, you should consider:

  • Continuous integration + commit small pieces of change (instead of doing a big commit after several days)
  • TDD - focus the team on doing unit tests, specially to avoid tigh coupling
  • Have not only unit tests, but other type of tests run periodically. Have different type of tests separated, you don't want load testing triggering on each build for example.
  • Have static analysis tools run with the build
  • Analyze the build info as a first step, before going further with the code review.

• My approach is to decompose the work into smaller chunks and review the changes as soon as possible. It's much easier to review 100-200 lines of code than it is to review several thousand and you can propagate any changes you need to make to subsequent code.

  Chris Nelson : The decomposition is probably the key to my problem.

  Tony : With new code this should be straight forward. I recently had to add several new classes to existing code as well as change the current code. Where possible the code was reviewed as the classes were completed. If the classes were large we reviewed methods as they were added.

  Dan : @Chris, have the author help with the decomposition. When I have to put out a huge review package like that (not by choice), I provide an overview, suggestions on where to start, which files should be reviewed as a group, etc. Reviewing 10,000 lines at a time is crazy though, that could take a week! If that's the situation you're in, maybe you can divide the work among a few reviewers.