Wednesday, July 31, 2013

Code Reviews: A Sane Approach

Do you feel code reviews are excessively time consuming, distracting, and generally just more work than they're worth?  Perhaps your process is to blame.

I've worked on a lot of projects that have done code reviews poorly.  Perhaps you have too.  I've experienced first-hand having my productivity destroyed with constant peer review interruptions.  I've felt the discomfort of trying, as diplomatically as possible, to tell a defensive co-worker their code sucks, only to have them check it in anyway.  I've been in the position of avoiding that argumentative team member to perform reviews so my quick check-in doesn't turn into an all-day discussion.  Sound familiar?

I've also worked on projects that have done code reviews right, and that have reaped the benefits.  I've seen junior team members get up to speed fast while learning advanced techniques they wouldn't have otherwise learned for years.  I've seen every member of a team learn to actually use the same conventions (e.g. fewer foreach's more LINQ).  Most importantly I've seen teams with a variety of skill levels consistently and methodically increase the quality of large code bases as a team.  It is possible.

A Process That Works

The best process I've seen so far works like this:

After a team has completed development on an iteration they must complete a code review on all new code developed during that iteration.  This step is a part of the team's iteration closeout procedures.  The entire team is given time to review changes individually (typically comparing against a tag from the last code review) and then compile a list of issues which they bring to a group review.  

In the group review everyone goes through the individual issues and compiles a finalized list of code review issues, clarifying, removing, or adding as necessary.  Team members sign up for code review items, items are estimated (often aggregated by person because they tend to be so small), and for the most part are put in the next iteration.
 
Performing the code review in this manner has the following benefits:
  • Interruptions, a developer’s primary productivity killer, are kept to an absolute minimum
  • Architectural and cross cutting issues are easier to spot than with per-feature code reviews
  • Placing code reviews as part of iteration closeout procedures ensures they are performed and maintains quality as a top priority
  • New team members (in particular) benefit from consistently reading code, and seeing first-hand conventions and techniques employed by more senior members
  • Pre compiling a list of issues eliminates all-day marathon type code reviews
  • Unlike with a per-feature review it is not immediately apparent who is to blame for any bad code, reducing negative feelings and maintaining a focus on what’s important: the code
  • Expensive refactorings can be reviewed by the Scrum Master (or customer or whoever) and postponed as priorities dictate

Summary

Code reviews don't have to be awful.  It really is possible to increase quality and bring everyone's game up a level.  If you've had negative feelings with code reviews in the past, maybe it was the process.  Maybe it's time to try again.

Those are my code review experiences.  What are yours?  Please feel free to post in the comments.

Friday, July 5, 2013

Cannot await in the body of a catch clause

I stumbled onto this error recently: "Cannot await in the body of a catch clause". Or as ReSharper says it "The 'await' operator cannot occur inside a catch or finally block of a try-statement, inside the block of a lock statement, or in an unsafe context".

The limitations as described by ReSharper all seem to be for different reasons.  I'm only describing the bit about the catch block, but if you're interested in why you can't await inside of a lock statement or for some interesting reading check out this StackOverflow post.

It happened in code that looked like this:

try
{
   
return await GetMinutesOnce(show);
}
catch
{
   
await RandomDelay();
   
return await GetMinutesOnce(show);
}



The idea is simple: try to do something, if it fails for any reason whatsoever, then wait for a random amount of time and try again.

After a little digging I was surprised to learn that awaiting inside of a catch block is disallowed because of a limitation in the CLR.  Normally when you're inside of a catch block in C# you can use the throw; statement, all by itself without an exception, and the CLR will re-throw the exception from where it originated with it's previous stack trace and all meta-data.

The throw; statement is a nice feature, and it works because the CLR automatically keeps track of the "ambient exception".  The problem with an await in a catch is that when control is yielded to some other place in code, another exception could be thrown which would overwrite the current ambient exception and cause your throw; statement to throw the wrong exception.

In my case I was able to fake out the compiler by doing this:

try
{
   
return await GetMinutesOnce(show);
}
catch
{
   
// await can't exist in a catch because the CLR would lose the ambient
    //  exception. We don’t need the ambient exception (i.e. we don't "throw;", 
    //  so we need to trick it out by putting retry logic after the catch
}
await RandomDelay();
return await GetMinutesOnce(show);



Not very pretty, but it seems to solve the problem.

It seems to me as if the compiler should only error when both an await and a throw statement occur together in a catch, but I guess compilers don't really work like that.  After all, which line number would it list?

Anyway, I thought it was interesting.  Feel free to post a comment if you can think of a better solution.