Originally posted by Kenny Birney: Well as usual with XP practices, ive found loads of stuff telling me how good the practice is. but very, very little on how to actually perform it.
I'm confused. As far as I know, formal code reviews aren't an XP practice.
The soul is dyed the color of its thoughts. Think only on those things that are in line with your principles and can bear the light of day. The content of your character is your choice. Day by day, what you do is who you become. Your integrity is your destiny - it is the light that guides your way. - Heraclitus
Shameer, have you used Fagan inspections yourself? If you have, how do you feel about them? Have you tried other, more informal code review techniques? If you have, how would you compare the effort and results between them?
Lasse, when I was working with Motorola they had a detailed Software Production Process and code reviews was one of the processes. Initially we had formal code reviews and informal code reviews. This was changed before we went for a CMM assessment and we only had formal code reviews.
Basically, informal code reviews were ad-hoc in that Tech lead or someone senior would review the code of a junior engineer. I had no issues with this and thought it was good as Tech lead pointed things out and was not intermediating as a formal review with 3 or more people.
The Formal reviews were based on the Fagan style, which included about 7 phases. The reviews were called inspections as this process was applied to any software artefact that was produced. 1. Planning - set a time in the schedule with PM 2. Inspection kit - author checks code, code is labelled etc and sends details to reviewers 3. Overview - given by author to reviewers 4. Preparation - individuals review on there own 5. Inspection - all meet and discuss the artefact 6. Rework/moderator check 7. Process improvement (optional if we went over/under the control limits)
In addition, the process was modified that defects were captured and analysed with ODC (Orthogonal Defect Classification)
In the main, I found this process very useful in detecting defects. The main benefits were IMO: - Bringing new developers up to seed on company coding standards and introducing them to the business rules. - More than one person inspecting the code, many minds are always better than one and you get a good result when everyone is focussed and have prepared for the inspection. - Collection of metrics, very useful in seeing trends and common errors that are made.
Major disadvantages are: - Control limits if you go over and under them, management then ask questions. For example you inspected 250 LOC of code in half an hour, not the prescribed 2 hours. A justification may then be required. - Can be a very long process if there small change in code < 20 LOC
Having moved on from the tech sector to the finance sector, code reviews are very ad-hoc and it all depends on the team leader and type they want. In general it just one person who looks at the code, mostly the team lead. They spend at max about 10-20 minutes. These types of reviews don't provide much benefit because the team lead does not always know specific business rule, hence if there are errors it doesn't get picked up and just gets raised as a defect.
A practise that I employee is get a fellow team member that is familiar with code/business rules and get him to review the code at my desk. I generally walk through the code with him and let them ask question why this done etc. I have found this very effective as this does pick defects or incorrect business logic.
In summary, getting people together that are familiar with the code, business rules and with the correct attitude will make a code review/inspection very beneficial in improving the quality of the code..
Joined: Jan 23, 2002
So then the question remains, were the Fagan inspections more bang-for-buck than what you're doing with taking a coworker to your desk, considering the amount of process and time involved?
Joined: Apr 08, 2004
Due to my constraints I would like more people to peer review my work. So yes, the co-worker option as an immediate affect and helps meet the deadline(s).
However, if the process was formal, metrics could be collected, knowledge spread to other team members, technical skill learned from reading other peoples code etc. I believe this has more benefits to the team/company.
IMO, I would agree and prefer a Fagan style inspection, if customized to your needs it would provide more benefits in the long run.
This is all based on my experience and would like to know what other people employ in regards to code reviews. The most important factor in both processes is that that the correct people are involved and have the correct attitude towards code reviews/inspections. People do play a big part in both processes
[ May 16, 2006: Message edited by: Shameer Subedar ] [ May 16, 2006: Message edited by: Shameer Subedar ]
Joined: Jul 11, 2001
Originally posted by Shameer Subedar:
This is all based on my experience and would like to know what other people employ in regards to code reviews.
We are doing a lot of Pair Programming. We also do informal peer reviews from time to time. I think that Pair Programming generally works better. I suspect that it would work even better if we switched pairs more frequently (but there is some resistance to this idea in our team).
We never tried formal reviews.
Joined: Jan 23, 2002
I don't have any prolonged experience with formal reviews (e.g. Fagan inspections) either. On one project many, many years ago there was an attempt at adopting more formal reviews but it was quickly abandoned as people felt the reviews weren't helping. After that, just like Ilja, I've been having positive experiences with more informal methods of peer review. Pair programming is number one for me but we've also had a good feeling about "micro" reviews a la someone reviews each and every SVN commit made during the past couple of hours.
One way to approach this formally is to assign different reviewers a different aspect of the code to review. They should be given the code to review 24-48 hours before the review meeting. At the meeting each reviewer highlights the issues that they have found.
The meeting is used to highlight any issues, not to have a prolonged discussion on what the developer needs to do to resolve the issue. This should be done outside of the review meeting. This combined with the fact the the reviews have been completed before the meeting means that the review meeting is kept relatively short.
Some aspects that can be reviewed : Conformance to Code Guidelines Peformance issues in the code Code keeps to project specific guidelines Inefficient algorithms (A senior developer can show better methods of acheiving the same thing) Localisation issues User Interface conforms to UI guidelines Functionality conforms to requirements