Code review, before or after check-in?

Recently I’ve been thinking about our code review process, and how it could be improved. We’ve recently moved from a simple VSS based source control to a more feature rich TFS based approach.

We now have the majority of feature development occurring in several development branches, at the end of each development iteration we get all the changes reviewed and then merge the changes into our Main (or trunk); a build from Main will then be passed to test.

Problems we’re finding with this approach are:

Development Code Freeze

Whilst code is being reviewed (and review actions being actioned) we pretty much freeze feature development in the branch, the code review can often take a long time, so people often start doing further development off-line, ready to check-in once the freeze is lifted.

How can we make sure that what is eventually merged into Main is what was reviewed? 

If the review is done against the changes between Dev and Main then you miss reviewing any merge conflict resolutions (unless you do a 2nd review). If you review the ‘pending merge’ (where someone does all the merge process apart from the final check-in to Main), you risk the merge no longer being valid (someone else checks into Main between pending merge and review complete).

Seeing very little benefit from the review

Our process of reviewing all code going into Main is giving us a false sense of confidence, as we think Main has top-quality code in it; in fact this is not the case, often it seems the only review comments that are actioned are fairly trivial changes that wont take very long. Any fairly major changes are often deferred to a future ‘Change Request’, with the merge going ahead anyway, as the feature is required.

We do want to prevent breaking changes going into production or even to test, but this should probably be caught by testing (automated) and Continuous Integration.

Code review is inconsistent

Code reviews are processed in a fairly inconsistent out-of-band way, with very little formal process around it. Review comments can sometimes be lost, and tracking whether the review was fully actioned etc is not 100% possible.

Suggestion: Do the code review after the check-in to main

So the suggestion I’ve been making is that instead of holding up the check-in to main with the code review we do the code review afterwards. The process would be something like this:

  • Feature development is done in a development branch
  • Towards the end of the iteration the code is prepared for check-in to main
  • The code is merged into main
  • The check-in to main causes a code review task to be created for the changeset
  • Code review actions are done directly against main.
  • Once the code review has all been resolved (actioned, further tasks raised etc). It is marked as closed.
  • Before main is pushed to production all feature development code reviews must have been completed.
So if we adopted this process would it resolve our issues?
  • Development Freeze – by decoupling review from development the two activities could run in parallel, as soon as the merge is complete development could resume. If merging up to a label you wouldn’t even have to freeze whilst the merge is done.
  • Review exactly what was merged – by definition we would be reviewing what was merged. We could even say that review actions should be reviewed so no changes to main go unreviewed.
  • Review benefit – by decoupling review from development and making the review process more traceable I think we will be able to add review actions to the product back-log and then even the big changes will be done. Review actions should be treated in a very similar way as feature development with priority dictating what is actually done.
  • Review inconsistency – having a proper review process that is tracked with tasks will means a more consistent approach and a reliable process. Any unresolved review comments will be searchable/reportable just like any other tasks.
So what would the cons be?
  • I think we would risk checking in bad code to main, and although we could use our source control system to roll these back etc, we might not catch them in time – I don’t think, however, that our current process really stops this happening.
  • Reviewing could be neglected in favour of feature development, but this just means that project management needs to see the importance of the review process and prioritise the tasks appropriately.

Accepted Best Practice

Before writing this post I tried to search for the best practice in this area and couldn’t really find any. I guess one answer is that instead of code reviewing paired programming could be more effective. Does any one have any suggestions as to what others are doing?

Advertisements

About Alex McMahon

I am a software developer, interested in .net, agile, alt.net. I've previously specialised with .net 3.0 technologies like WCF, whereas now I am trying to specialise in agile development and best practice and patterns. I am obsessed with looking at the latest technologies, tools, and methods, and trying them out. I am currently employed by Rockwell Collins in the UK.
This entry was posted in agile, development, guidelines, methodology, techniques. Bookmark the permalink.

5 Responses to Code review, before or after check-in?

  1. ouelcum says:

    I used to develop in a dedicated branch, and merge my code at the end of the dev. This is good/efficient when you are alone to develop, in order to keep your main ‘safe’, or when you have a lot of developer, and you don’t want to see unfinished code in your main.

    Today, i will advise to do differently.

    1- Commit in the main ASAP, as it’s the only way to detect inconsistency among branches, and it’s a efficient way to find integration bugs.
    2- Insure all your dev are finished, even if your task/feature is not finished, you can commit on a file/class bases finished
    work.

    So, the code review process is present only on the main.

  2. Alex McMahon says:

    @ouelcum: So do you do a code review before checking in to main or do you do it afterwards?

  3. Pingback: Why do our code reviews take such a long time? « ICoder

  4. This is a difficult topic and there are a lot of compounding issues here. Disclaimer: since I haven’t directly coded in a few years, I can only dispel advice based on my observations as a coach working with a team, or on a philosophical level. That being said-

    – Check in frequently, check in constantly, merge code maniacally. Using a system like SVN instead of CVS or Clearcase forces developers to do this since there are no lockouts and last one in has to merge code and get the tests green again. This is a team culture thing and you can nudge it into a more positive direction (but it will be painful at first).

    – Your code review priority, scope, and energy level seems to be a problem for your team. Code reviews can’t be done to fulfill a checkbox on a checklist. If you are regulated (HIPAA, Military, etc), then there is some level of ritual you have to complete, but the code review should be something the team owns. Sounds like it is time to get the team together and do a retrospective on code reviews. Figure out what they would want to value them for. What do they want them to do. Create a checklist that the team agrees are items that help them build and maintain better code as a team. This is your code review scope and it is a living/evolving checklist.

    – The main branch topic. My last team had many branches. Main was constantly checked into and smoke tested with CI. The nightly build tested it with the full test suite. We had another branch for the test server to run against. This was the server used for Sprint Review and customer demo’s. The CI only pushed to this branch if the nightly build passed all tests. This helps with your “Check-in to main has always been seen as the be-all-and-end-all, meaning that no further work needs to be done.” because the business no longer looks to main for the “DONE” delivery. They look at the test branch which has a lot less fluid evolution to it (once per day instead of once per hour). This creates a stable environment for the business to pound against anytime they want.

    That’s another point- is your business using the system daily to review and provide feedback on work as it evolves every day or so? If not, then it becomes too easy to code for a week without checking in. Giving them access to your work as it is ready creates a checkpoint for both dev and the business to watch progress and make progress daily/weekly.

  5. Alex McMahon says:

    @Kevin:
    Some very good points, I agree with pretty much all that. My previous role was in a fairly agile, new team on a greenfield project; My current role is with a fairly established team with a great big legacy application suite.

    So my (seemingly continuous) challenge is to try and introduce some of my agile experiences here. It’s always an interesting challenge to find the key points and migration paths and we’re certainly not there yet. As you pointed out it can be a painful process, but we’re certainly seeing some benefits already.

    I found the code review topic an interesting one as it wasn’t something we did at all at my old place (I did suggest dropping it, but people weren’t very keen).

    We don’t currently have the daily deployment and usage, as we still haven’t fully automated the deployment of the entire suite (and there is actually some resistance to continous deployment). I’m part way there and most of the new products are ready for that approach, but some of the old ones aren’t really there yet.

    Thanks again for your replies!

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s