Usefulness of Code Reviews

When I chat with developers about code reviews, generally I get a positive response in terms of their usefulness 'in theory', although most are not exactly forthcoming when it comes to actually offering to take part in them. I think some see them as a 'threat' or an 'opportunity to receive criticism of their precious code'! This shouldn't be the case and those that believe this I think have missed the point. I see code reviews (or more often in my case, 'system reviews') as opportunities to pause for thought, take a step back, look at what you've achieved and do a critique of how 'well' it has been designed and developed. Every review I have ever done has always had positive outputs.

I've just recently completed a code review of a medium sized project written in ASP.Net/Oracle. I did it 'paired' with another developer and was a joint effort. We concentrated on the following points:

• The Solution and how it was organised into projects and subdirectory structures.


• Shared libraries and their usage


• Consistency in 'style of development'


• Source control/versions. How Visual Source Safe (VSS) is used for (e.g. source code only or some docs too?)


• Coding style and conformance to coding standards (and are the standards we have in place up to date/suitable?)


• Any areas where it would benefit from refactoring


• Upgrade comments (recently undertaken) from .Net 1.1 to .Net 2/VS 2005. Where could the project benefit from refactoring/rewritten using .Net 2 features


• Use of the N-tier approach, coupling and possible compromising of layers (e.g. SQL in code behind files?!)


• Stored procs (Oracle) and adopted standards


• Pass thru SQL vs Stored procs


• General concerns and feedback on development so far

Here's the outputs from our session:

• the project could have benefited from being broken down into more sub-projects


• our coding standards need updating to bring in new .Net 2 features, new controls, app_themes etc, and JavaScript coding guidelines.


• N-Tier approach used very well in most places by developers although some SQL present in 'higher layers' from earlier coding (!). 


• There were some areas where standards were not followed but mostly affecting 'early code'. Most of latest code followed guidelines and best practise


• Would benefit from the use of Web Services in the shared 'site list' library: 


• Source control working well. Oracle stored procs, views etc also in VSS.. but would be better if integrated with TOAD. To investigate. 


• Better versioning needed of shared libs. 


• Some of the system would benefit from some .Net 2 enhancements (eg master pages, themes, new .net 2 menu tree) but decided to only bring in new .Net 2 features on either brand new modules or where a significant change is needed on an existing part of the system. Consistency important though.

All of this I think is good positive feedback and result in actions where necessary. It was a joint effort, which is important. Even if you are reviewing a new starter's code, there is still opportunity to learn from their past experiences.

If anyone else has experience of doing such reviews, do let me know.