CodeReviews

From Epowiki

Jump to: navigation, search

The benefit of code reviews is undeniable. Take a look at Code Review References for research showing the effectiveness of reviews.

Yet, code reviews don't happen in most software development organizations. And for good reason. The way code reviews are typically handled uses an enormous amount of people and time resources. Even with system quality improvements developers may not feel the effort is worth the cost.

It turns out you don't need a high overhead code review process to get results. At one company I worked at I created a very low overhead automated code review system that enabled every line of code to be reviewed before it was checked-in.

Here are some ideas of how code reviews can be made practical in your organization.


Contents

Ineresting Code Review "Facts"

My initial impression of code reviews is that they were heavy and bloated and a waste of time. I was wrong on all counts. Here are some interesting facts I found in the research:

  1. Using two inspectors is as effective as using four or more.
  2. Using an asyncronous process is as effective as a meeting based process for code reviews. For design reviews a meeting based approach may be more effective.
  3. One review session is as effective as multiple inspection sessions.
  4. Scenario or perspective based reviews were more effective than add-hoc and check-list based reviews.
  5. Inspections are effective.
  6. Inspecting upstream process like requirements and designs is very effective.

The implication is that the complexity and overhead of Fagan like code inspections are not needed and a semi-automated method will yield improved system quality. Requirements and HLDs seem to benefit from a more structured inspection process, which makes a certain amount of sense. <P>

Code Review Standard

This is the list of some policies I would use to create a code review standard for my organization. Take what's here, read the review on code research, and figure out what will work best for your organization.

The key point to keep in mind is that the heavy weight nature of code reviews is not true. You can create a code review system for your group that is light enough that you can review every change in your entire system.


Every Line of Code Must Be Reviewed Before it is Checked-In

Do not allow code to be checked-in unless it has been reviewed, fixed, and rereviewed. If you can't do this then your review process isn't fast or light enough. It can be done.

Reviewing code after it has checked-in is next to useless as everyone is exposed to the unreviewed code.


Code Must Be Integrated, Compiled, Unit Tested, and System Tested Before Review

Spending time on code that is just going to change or doesn't work is a massive waste of time. The entrance requirements for a code review are:

  • Code should compile without errors or warnings (according to coding standard).
  • Code should be already be integrated with its parent branch.
  • Code should be fully unit tested.
  • Code should already pass the system tests where possible.


Develop Perspective Based Reviews

For the issues that are important in your software consider developing a Perspective Review for each issue. Perhaps special hotspots for you are internationalization, memory corruption, memory usage, threads, semaphores, etc. An example can be found in Semaphore Perspective Code Review.

You can then use the perspectives to assign roles to review team members.


Use Meetingless Asyncronous Reviews

You don't need to hold a big meeting for a review. Everyone can review the code when they can find time. Simpy have a done-by date when the review must be completed.

Everyone on the review must review the code. If a person can't perform the review then the team needs to elect someone else to perform a similar role in the review.

Meetingless aysncronous reviews are fast and light, yet they are still effective. With the right tool support you can easily review every line of code in your system.


Use Between 2 and 5 Relevant Reviewers

Too many reviewers wastes everyones time. Keep the number of reviewers small and relevant to the code being reviewed.


Assign Reviewers Roles and Perspectives

It's almost impossible to review a lot of different issues in a lot of code. People get tired and they stop noticings things.

The way around the getting tired issue is to use perspective reviews. Create a perspective for each important issue category your are concerned about. Assign the perspectives to people on the review team. Because they are only reviewing for issues in their perspective they will do a better job because they can stay more concentrated. This doesn't mean they can't find other issues as well, but they responsible for their perspective.

For example, if using semaphores correctly is important in your software and the code has semaphores, then assign someone the role of reviewing semaphore usage. An example can be found in Semaphore Perspective Code Review.


All Review Communication Should Go to the Review List and be Logged

Part of the benefit of a review is that people learn about the system being reviewed. This learning feature is facilitated by broadcasting email communication between the review team and saving all communication so it can be read by other people later.


Do Not Redesign in the Review

Make a note and schedule design issues for for a later time.

Developers always think they can do stuff better. Take these issues off line unless the issue is that requirements are not being met. Requirements not being met is not the same as you could have done it better.


Do Not Cover Coding Standards Violations in the Review.

Send violations via email or in person.

Talking about violations only gets everyone angry and is a waste of time.


Code Is Rereviewed Until it Passes

Code isn't reviewed once and then forgetten. Any changes made have to be rereviewed. If you think this is too slow then your process isn't light enough.

No reviewing all changes makes the process useless as people will just ignore suggestions or introduce new bugs in any changes.


All Issues Must be Fixed, Marked as Not an Issue, or Marked as Bug

Any issue brought up to a developer must be handled. A developer just can't ignore issues because they think it's stupid. Every issue must:

  • Fixed.
  • Marked as Not an Issue. If the developer and the reviewer can't decide between themselves if an issue should be fixed or not, then the review team gets to decide. If there is only one reviewer then bring a manager in or another team memember.


Automate Your Code Review System

You can make your process light enough by building it into your build system. If your process isn't light enough work on until it is.


Review All Code on Private Branch Before a Merge

Code developed on a private branch doesn't need to reviewed during development. But before the code is merged into a parent branch all code changes must go through the complete review process. For this reason, development of a large scale feature, may still want to perform reviews on the private branch because that can speed up the merge process.

Of course, try not to have branches separate from the mainline, but for large features that take a long time to develop you will often need separate branches.


Review the Right Scope of Changes

You don't have to review every line of code in every module that has changed. Certainly if a module is new it must be completely reviewed. Other than that you may be able to just review the changed code. Though just reviewing changed code isn't always possible. If you are performing a semaphore perspective review, for example, then you will need to go look at the code within in the scope of the sempahore as well.


Stick to Reviewable Issues

Develop your list of what issues can be reviewed and how they are to be reviewed. Usually this is in the form of check lists and perspective reviews. Don't allow reviews on other items without changing what can be reviewed. Otherwise people spend endless time on off-topic arguments.


Review Team Reponsible for Deciding Issues

If there is a conflict on any part of the review then the review team is responsible for handling it. That's the only way the review process will be light enough to work.


Keep it Cool

Nobody is perfect. The attitude of the review should never be personal, it should always be professional, with the goal of improving the system and the people building the system. Keep your tempers.

Don't blame people for bugs. Work together to make things better. No finger pointing! Not ever!

Meetingless reviews can help keep the anger down, but it can make it worse too. When people are in the same room anger can ramp up really quick. And we know in email it's very easy to say something that can be take wrong. Raise the awareness of these issues in your team.

A good rule is to Never Assume An Attack. If you find yourself getting angry, assume it's a misunderstanding, not an attack.


No Managers

Unless a manager has something to add to the reviews then they shouldn't be involved. Issues should be decided by the review team. Managers always have to run to a different meeting, they don't have time slots open for meetings, and they generally don't add technical input. So you don't need managers as part of the review process.


If a Bug Was Not Caught in a Review Figure Out Why

If a bug happens after a review then track down why each bug wasn't found and then change your development process somehow to try and prevent that bug in the future.

This is not always possible as running full tests are often impossible, but it should be mostly possible.

I would create a bug for each bug to track down why it wasn't caught. Because this issue is more serious than just the review. It means the unit test, the system test, and the review did not work.


Review Upstream Documents Too

Reviewing product requirement documents, specs, standards, etc can provide excellent return on value. Make sure those products are reviewed as well.


Feed Leasons Learned Into the Team and Documentation

If issues come up during the review that everyone in the team would benefit from, then have a way to make wisdom public.

I would recommend a development wiki where you can write documentation on anything useful that people come up with.


Plug Reviews into Your Source Code Control System

I have done this through change check-in comments. Each change has to be reviewed before it is checked-in. The submit comment must contain a review ID that points to some document containing the review status for the change that is about to be submitted. The code is prevented from being submitted without a valid passed review. If you are able to automate your code review system all this works quite quickly and painlessly.


Perspective Based Reviews

Before my reseach into code reviews I had never before heard about perspective based reviews . Perspective based reviews are done from the point-of-view of various project stakeholders. This is as apposed to everyone doing an add hoc or checklist based review. A perspective would be a user, requirements person, hardware interface, exception handling, mutex usage, memory usage, etc. The thought is they are more effective because reviewers are concentrating on a particular issue instead of trying to review everything. Do one thing well i guess. <P>

Reviews are driven by scenarios. A scenario tells the insepector how to go about reading the documentation from particular perspective and what to look for. Clearly there's a lot to the scenarios. <P>

Perhaps people could be assigned in various review roles. Or maybe some people can perform reviews only from a certain perspective. <P>

Deciding If Code Reviews Work

How do you know if code reviews are working for you? Look for

  • Increased in test execution rate. The tests suites runs through its test suite. Unstable software can't do this.
  • Increased pass rate.
  • Reduction of critical bugs in QA.
  • Reduction of field issues.
  • Subjective improvements in quality.

Try to measure the issues to verify if code reviews are working for you.


Are Code Reviews Necessary With Pair Programming?

An issue that comes up is the relationship between code reviews and pair programming. Do you still need code reviews if you pair program?

If you don't pair program then you definitely should perform code reviews. That one is easy. Unit tests and system tests simply aren't good enough to replace code reviews.

After that it depends.

It depends on the product you are developing, it's importance, it's complexity, and how critical it is that there be no errors.

Pair programming is an excellent form of active code review, but there are many products where I don't think two eyes are enough. Some systems are complex enough, some domains are complex enough, and some conseuqneces are severe enough that I want more eyeballs on them before the code is let loose in a real code running system.

When pair programming is used you could decide what code should be reviewed by subsystem or by the type of change involved.

For example, code in a key algorithm may need to be reviewed by the impacted parties. Perhaps a drive change needs to be reviewed. Perhaps when a piece of code is written new or completely rewritten, it should be reviewed. Perhaps if semaphores are added to code that would trigger a review because it changes the system a great deal.

Your mileage may vary of course.


Code Review Resources

You'll find many of the code review papers interesting. I have rounded up some of the most interesting here.

  1. Code Review References
  2. Perspective Reviews
  3. Perspective Based Inspection Approach
  4. comparing detection methods for software requirements inspections
  5. practical code inspection for object-oriented systems
  6. what's so great about inspections?
  7. an experimental comparison of checklist-based reading and perspective-based readingfor UML design document inspection
  8. identifying the mechanisms to improve code inspection costs and benefits
  9. an experiment to assess the cost-benefits of code inspections in large scale software development
  10. Semaphore Perspective Code Review

Personal tools