A was talking to a project manager friend a couple of days ago and he was saying how the company he was working with at that moment was being your typical small development company. Little documentation, having to fix the same bugs over and over again and general code thrashing. I mentioned Code Reviews and he hadn’t used them as a means
Knowledge transfer (training):
No one developer knows everything or maybe only a couple of developers know how the system works inside and out. The code reviews can act as a way of spreading the knowledge of the system around the room. Making other developers aware of new updates to the system and how they work. So if something does go wrong they at least know roughtly where to look.
For example if you have an expert database guru or someone that is more knowledgeable on systems security, their input can increase the understanding of the other developers and help pull the no so knowledable developers up…
Even just developers talking and saying “Oh I would have fixed that problem but doing x y and z” increases understanding so that next time maybe they will try to tackle a problem in a more efficiant way.
Risk Management:
There have been very few companies that I have worked for where development documentation of a system has been “uptodate”. The only documentation that every seems to be the most correct is the user documentation, internal documentation? well thats another thing altogether. The moment documentation has been written it usually becomes out of date in no time at all. So in the end only the developer who developed the software unit/class etc knows really whats going on and therefore you get the all eggs in one basket problem. This was very real to one company I worked for when a developer went on holiday and wasn’t contactable. In a code review everyone see how things work.
… the list goes on and on. Code security, other developers seeing a potential security hole in anothers, more manageable readable code etc…
Ok now the nitty gritty, how to do an effective Code Review…
Firstly I don’t like the phrase “Code Review” it usually scares off developers…. So your going to look at my code and criticise it! I’ve called it a Weekly Project Review or Development Review in the past.
I’ve always done a review in the form of a meeting:
REVIEW MEETING STRUCTURE:
The reviews i’ve had in the past we have given specific roles to people in each review meeting:
Moderator – Their prime role is to keep the review moving fast enough to be productive but slow enough to find the most issues. They would also distribute design or code to be reviewed and an inspection checklist, setting up the meeting, reporting inspection results and following up on any actions.
Author – Quite obvious, the coder of the code 
Reviewer – Anyone who has an interest in the code. The reviewers take a look at the code before the meeting and discuss any bugs or questions during the meeting.
Documentor – This could be the role of the moderator if the group is small, but should not be the role of the author or reviewer (as its got a bit of a conflict of interest). The documenters role is to record the bugs/issues and the assignments of any actions during the review meeting.
Management – No way, this is purely a technical review so the focus is technical not political. Managers could receive a review report (list of actions from the review or notes on the checklist) very simple report which can be written during the meeting. But this shouldn’t be used to evaulate developers in any way.
REVIEW RULES:
I’ve followed a number of rules in the past for the review:
* To have checklists which focus the reviewers attention on areas which have been problems in the past.
* The emphasis is on bug/issue detection, not correction.
* Reviewers prepare for the review meeting beforehand and arrive with a list of issues/questions.
* Information is passed on to the next review to improve future reviews (eg: adding/removing questions on the checklist)
* The meeting is for technical people and not management.
* Under no circumstances should the review results be used for performance appraisals, its just like killing the goose that lays the golden eggs. It doesn’t make sense anyway performance should be based on final products/projects and not work that isn’t finished. Another reason why there is no management It just means everyone is a lot more free to say what they think.
* The meeting should have a minimum 3 people and maximum of 6 people; any bigger and the review gets too big to manage and ends up becoming a hindrance.
GENERAL PROCEDURE FOR THE REVIEW:
Ok in the past i’ve broken reviews down into 9 possible stages:
Planning: The author gives the code/design to the moderator. The moderator then distributes the code to the people they think should review it including the checklist which focuses the attention of the reviewers so they don’t waste too much time looking at the code.
Overview (optional): The overview is used only if the reviewers aren’t familiar with the project. The author will spend some time just describing the project. This is generally a dangerous practice because the reviewer could end up glossing over issues. The design or code should speak for itself, the overview shouldn’t speak for it as that’s what the whole exercise is all about.
Preparation: Each reviewer works alone to become familiar with the design or code. The reviewers use the checklist. For every 700 lines of code the reviewer “should” spend about an hour but its really down to what works best in the organisation.
Review Meeting: To start the review meeting usually the author reads the code/design. They will explain all the logic including each branch loop and each logical structure/object etc… while they are talking the documenter records any issues/bugs but all discussion of the issue stops as soon as they have been recognised. The documenter also notes the type and severity of the issue and the review moves on.
As I said above the meeting needs to keep moving otherwise you’ll loose concentration. So, depending on the project this shouldn’t take longer than 30mins/1hr. Everything needs to stay focused, I worked in one place where the group was banned from even discussing weather an issue really was an issue because they assumed that if one person is confused enough to think an issue existed then it was an issue and so the design, code or documentation needs to be clarified.
Inspection report: The moderator creates a review report usually during the meeting that lists each defect including its type and severity. At this point you could just put them straight into a bugtracking system while in the meeting as this will save time, and the report can be a list of these bugs. This therefore helps to ensure all the issues will be corrected and can also be used to develop the checklist which will in time highlight problems in coding etc and can help when creating risk registers… it can also give a good indication on the number of issues/bugs found. From the report you can find out the time spent on the issues over time which can be used to prove the usefulness of a review by giving hard data otherwise you are just left with saying reviews are good … because they are! This is also a good guide to see if they are working or not. Some people generally prefer end testing rather than reviews and it maybe that you will have to modify or even abandon the reviews. It depends on if it works in your company.
Rework: The moderator assigns issues/actions out to someone (usually the author unless its a larger project) for repair and the assignee resolves each defect on the list.
Follow-up: The moderator just checks to see how the issues/actions are being carried out.
Third-hour meeting: The code review or review is just that… a review so the people in the meeting shouldn’t be discussing solutions, but of course we will, so its better for those who wish to discuss different solutions to do this at another meeting either straight after the review meeting or at another time, this also allows it to be a bit less formal and get more people involved .
Fine-tuning the review: Hopefully over time the checklist will become more refined and things will be added and things will be dropped. We should find the same common issues appearing on the checklist and we can work on refining how we work to fix them. The checklist should be short and sweet so max of one page.
The Checklist:
As mentioned above this is used to track common issues so the developers use this as a guide when developing. It shouldn’t exceed more than one sheet of paper, an example of one is below, based on a PHP project:
Code Review Checklist
1. (Are there line numbers on all code printouts?)
2. Are the standard headers / footers being used?
3. Are functional javaDoc / phpDoc comments being used?
4. Is there enough documentation to work out what the system is doing?
5. Is there enough error handling, relating to the importance of the work?
6. Does the code follow naming conventions?
7. Does the code follow formatting conventions (bracketing)?
8. No Magic Numbers/Text
9. Any duplicate code should be put in a separate module for reusability.
10. Removal of code which is not being used.
An Example of the Review Notes:
This was of a unified login system. A simple system to allow a single point sign-on to login to an ASP and PHP based system:
Code Review
23/03/2004
Uni-Login Script Review
Moderator: Mr X
Author: James X (PHP), Peter Y (Architect)
Reviewer: Alistair X (Senior DB)
Reviewer: John X (Java Developer)
Reviewer: Simon X (Technical Documentation)
Reviewer: Tim X
Reviewers: Please make sure you prepare for the review meeting beforehand so you can become familiar with the code and arrive with a list of issues/questions and make use of the Checklist below.
Author: In the meeting you will be expected to read the code/design and explain all logic including each branch loop and each logical structure/object.
Code to be Reviewed:
http://internal-cvs/cvs/cvs.php/Common_SQL/LMSUniLogout.asp
http://internal-cvs/cvs/cvs.php/Common_SQL/LMSUnifier.asp
http://internal-cvs/cvs/cvs.php/Common_SQL/LMSkeepalive.asp
http://internal-cvs/cvs/cvs.php/forumsUnifier.php
http://internal-cvs/cvs/cvs.php/account.php
Related Files:
S:\web-sw development\Documentation\Code Reviews\Code Review Checklist.doc
S:\web-sw development\Documentation\Code Reviews\Uni-Login Script.doc
Results From Review:
ForumsUnifier.asp
ACTION: Missing Header/Footer
ACTION: ASP Comments (including related files) missing
ACTION: Use include to lmskeepalive.asp rather than static code
LMSUniLogout.asp
ACTION: Header/Footer missing
ACTION: Point to gatekeeper (update link)
ForumsUnifier.php
ACTION: Header/Footer Missing
ACTION: Not enough documentation
ACTION: Remove code which is not being used (Line 15,17,34 to 40, 41)
account.php
ACTION: Header/Footer needs to be updated with latest versions, with more information
ACTION: Line 247: Needs a header refresh to index.php and remove acountDetails() reference and function.
ACTION: PHPDoc comments missing.
ACTION: Line 97 – 99, 229 comment removal
ACTION: Lines: 127 to 171; Remove echo’s and drop out of php instead.
ACTION: Line 113; Remove magic numbers
ACTION: Line 114, 113; Remove rand functionality
ACTION: Line 212; replace “relevant” with “NCALT Systems”
ACTION: Line 218; change to “Back to NCALT.com”
CheckList Items Added:
9. Any duplicate code should be put in a separate module for reusability.
10. Removal of code which is not being used.
Duration: 1 Hour
