Skip to main content

To code review or not to code review?

There are many articles on the web about how to do good code reviews, with mostly all of them either discussing their merits or how to obtain maximum value from them.

In a new role I've recently helped introduce and roll out the agile process and to start with as part of each story we always added a task to "code review" all work. We made sure that differences from source control were used to make sure only the changes made were reviewed, made sure that everyone had chance to partake in reviews so it wasn't just one or two people providing all the feedback on the other team members work. It worked well, the reviews were productive and provided really good feedback for everyone.

But there was something about them that didn't feel right for the entire team, but it was the sprint planning process of tasking the stories that gave us the answer to what was bothering us. For every story we added the code review task to the END of the story, which of course by it's very nature was when all the development had been done! We analysed most of the feedback that came out of the review process and realised that, similar to the wedding singer, that information would have been useful to us "yesterday".

So as a little experiment we added a task right at the beginning of the story that involved at least 2 developers, sometimes a tester and for more complex stories, the entire team. Outside of the task planning session the developer(s) assigned the task spend 30-60 minutes detailing the approach they intend to take to solve the story - the other participants in the process get to question the approach and by the end of the discussion we've normally ironed out all the bumps. During these experimental stories we kept the code review task, but the initial discussion task along with taking a TDD approach coding against Gherkin provided by the test team we quickly identified that the code reviews had become redundant.

We've now taken the decision to remove the code review task completely and haven't noticed any decrease in code quality or maintainability. The number of issues reported in the produced code has stayed pretty constant, but we're no longer have to spend any time at the end of the story reworking any code as a result of review feedback!

Comments

Popular posts from this blog

Mocking HttpCookieCollection in HttpRequestBase

When unit testing ASP.NET MVC2 projects the issue of injecting HttpContext is quickly encountered.  There seem to be many different ways / recommendations for mocking HttpContextBase to improve the testability of controllers and their actions.  My investigations into that will probably be a separate blog post in the near future but for now I want to cover something that had me stuck for longer than it probably should have.  That is how to mock non abstract/interfaced classes within HttpRequestBase and HttpResponseBase – namely the HttpCookieCollection class.   The code sample below illustrates how it can be used within a mocked instance of HttpRequestBase.  Cookies can be added / modified within the unit test code prior to being passed into the code being tested.   After it’s been called, using a combination of MOQ’s Verify and NUnit’s Assert it is possible to check how many times the collection is accessed (but you have to include the set up calls) and that the relevant cookies have …

Injecting HttpContextBase into an MVC Controller

It is a shame that when the ASP.NET MVC framework was released they did not think to build IoC support into the infrastructure. All the major components of the MVC engine appear to magically inherit instances of HttpContext and it’s related objects – which can cause no end of problems if you are trying to utilise Unit Testing and IoC. Reading around various articles on the subject just to get around this one problem requires the implementation of several different concepts and you are still left with a work around. The code below, along with the other links referenced in this article is my stab at resolving the issue. There’s probably nothing new here, but it does attempt to relate all the information needed to do this for Castle Windsor. The overview is that all controllers will need to inherit from a base controller, which takes an instance of HttpContext into it’s constructor. It then overrides the property HttpContext in the main controller class, supplying it’s own version…

Unit Testing Workflow Code Activities - Part 1

When I first started looking into Windows Workflow one of the first things that I liked about it was how it separated responsibilities. The workflow was responsible for handling the procedural logic with all it's conditional statements, etc. Whilst individual code activities could be written to handle the business logic processing; created in small easily re-usable components. To try and realise my original perception this series of blog posts will cover the unit testing of bespoke code activities; broken down into: Part One: Unit testing a code activity with a (generic) cast return type (this post)Part Two: Unit testing a code activity that assigns it's (multiple) output to "OutArguments" (Not yet written)So to make a start consider the following really basic code activity; it expects an InArgument<string> of "Input" and returns a string containing the processed output; in this case a reverse copy of the value held in "Input".namespace Ex…