Archive for June, 2006
Last year saw and upserge in people going to the cinema in the UK, while our US counterparts have been seeing a decline… but don’t worry there is a new cinema on the block! (or idea at least!). Three german companies have come up with “Cinevision 2006”. It runs at a 10 megapixel resolution thats 5000×2000! and you thought your 1080p high def home cinema was good!
Just take a look at the quality and the size of the guy in front of the screen! So when’s it coming to the UK? ðŸ™‚ give IMAX a run for its money, ok maybe not ðŸ™‚
Just have to be a bit more careful on the rotoscoping! people will REALLY see your mistakes up big! ðŸ™‚
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.
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.
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.
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:
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:
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:
ACTION: Missing Header/Footer
ACTION: ASP Comments (including related files) missing
ACTION: Use include to lmskeepalive.asp rather than static code
ACTION: Header/Footer missing
ACTION: Point to gatekeeper (update link)
ACTION: Header/Footer Missing
ACTION: Not enough documentation
ACTION: Remove code which is not being used (Line 15,17,34 to 40, 41)
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
Sorry I haven’t posted for a while I had a small problem with my laptop (I think the photo shows you why!). They always say things come in threes. My laptop harddisk died on me last week, I had a loverly time running around Sydney trying to find a replacement hard disk. I’ve spent the last week trying to recover my data, that of course I had carefully backed up! ðŸ™‚ ok most of it but not all!
I’m sure everyone knows about The Universal Law of Gravitation, but the same thing can be shown in class rooms. You may be witness to The Second law of Gravitation.
“Students will gravitate towards the back of the class room. The seats in front fill up last.”
The only reason I think that this happens is so that the students don’t get picked out by the tutor or just don’t want to get eye balled by the tutor. But I generally prefer to be at the front. Less distractions on whats going on and also its a lot harder to dig out a book when you get bored :).
It turns out that if you sit at the front or down the centre of the class your more likely to retain more information. Research shows that people placed randomly in the front and down the centre of the room will retain not just 10% more infromation but almost twice as much!
At Uni I remember a mate of mine who sat right at the front of the lecture hall. Unfortunatly for him he fell asleep. The lecturer then walked up to him and slapped the table next to his sleeping head. He awoke to see the entire lecture hall looking at him. From that day on my mate religously stayed at the back of the lecture hall, for all his lectures. If only the lecturer knew that that one incident could have reduced my mates learning capacity by nearly 50%!
How to snowboard for free in Austria
This is a clip about how a couple of snow bums stayed in Austria (one of the most expensive resorts in Europe) for the season for free. I especially like the ketchup soup!
I’m back in Syndey after a rather long flight. I went via San Fran and had to wait in San Fran for 5 hours. I was flying with United whose airplanes are as old as anything! I did notice one of the flight crew using their mobile as we landed too, I know they are now thinking of making mobiles usable on planes but I still thought it was “illegal”.
Mikal is coming over from NZ for the weekend as its a bank holiday in NZ this weekend. Need to buy a bed tho! ðŸ™‚ I’ll post some pictures of ma house when I get their! ðŸ™‚
Anyway just chilling out in Sydney Central. I’m working for a UK client out here for a bit and my plan is to sort out my show reel while i’m here too. Anyway time to hunt out some food… starving, i’ve been living of airplane food for the past 24hrs… not good. I do remember when I first flew and thought that plane food was amazing coming in different little containers etc. Think i’ve changed now ðŸ™‚