|
|
Many years ago I worked with a team that did very effective code reviews. These reviews were much closer to "inspections" rather than reviews.
About a week prior to the review meeting, the person who wrote the code circulated it to the team and maybe one outside reviewer, along with specifiying roles or tasks for these reviewers. These roles/tasks included - code standard compliance, OO structure, proper system decomposition, proper layering, etc. When the review was conducted, the first task was to ask each participant how much time they spent preparing. If the time wasn't deemed sufficient, the meeting was rescheduled and those who didn't prepare were asked to devote some time to this.
We probably reduced our serious defects by 50% based upon these reviews and the code maintainability went up dramatically. I've since been workinbg with a group of people who don't understand the purpose or benefits of code reviews. I've consistently offered up my code for review, but the only comment I get are "I don't like that variable name", or some other useless rermark. I've also gotten commenst like "the code is done so we don't want to change anything, do we?" My only sollace is that the defect rate within my code is less than 1/2 of any other team member's - maybe they need the code reviews.
Well executed code reviews are the easiest way to improve the quality and maintainability of a system. Studies have shown a payback ratio from 2 to 5 times the cost of doing the review - money and effort well spent! Plus, it is an opportunity to teach and learn better techniques.
Dale Thompson
|
|
|
|
|
Dale Thompson wrote: Well executed code reviews are the easiest way to improve the quality and maintainability of a system. Studies have shown a payback ratio from 2 to 5 times the cost of doing the review - money and effort well spent! Plus, it is an opportunity to teach and learn better techniques.
There's a section on code reviews in Steve McConnell's Code Complete 2nd edition, where he says that code reviews are far more effective than testing in reducing code defects. However, as suggested by this survey, they are hardly ever done and when done are often done poorly. McConnell also describes how they ought to be set up.
I couldn't see a discussion of this in McConnell's second edition though.
Kevin
|
|
|
|
|
Yep, most of the bugs in our production system came from hotfixes where the coder failed to ask for a review.
"Oh its a small change, its gotta be right."
This statement is false.
|
|
|
|
|
Chris S Kaiser wrote: "Oh its a small change, its gotta be right."
They're often the most dangerous!
Kevin
|
|
|
|
|
We would raise our hand and shout in the room: "Anyone free for a code review" and rarely would there be silence. We did this for every checkin. It was great for short pieces, but longer ones required a bit of scheduling and patience.
This statement is false.
|
|
|
|
|
In good old days when I worked in San Diego, we had formal code reviews, and found them very useful. It wass not the review sessions per se that made such a big difference - but the fact that every developer new that his code would be reviewed by others made some sloppy coding techniques go away.
Another great benefit of formal code reviews was sharing the knowledge accross the team.
|
|
|
|
|
Reviewing something is always good whether it is code or design or something outside programming. It will certainly improve quality if done at proper stages of development.
But, too much reviewing is bad and too little is also. Attributes and methods of a component has to be reviewed atleast 3 times and any change in it also. Whether a private string variable names start with s or str is irrelevant as far as its name is correct and conveys proper meaning and purpose.
I prefer to do reviewing at design level rather than after all coding is over. Whether in OO environment or procedural environment, I prefer coders to decide all attributes, methods, indexers, events and as far private variables as possible in advance. Then they will be reviewed before their implementation starts. (It was difficult initially and lot of help was needed especially for freshers). Reducing the size of methods is of utmost importance while doing this.
When this level of checking is done, it is my experience that the maintainability increases very much. The general comments that if we do this type of planning, we will be wasting a lot of time and delivery date will not be met etc are rubbish. It is not the first delivery dates that matter. It is the final go-live date that matter for a serious client.
When a change is there, the class structure is reviewed and points of changes are first spot. Doing it then will help reduce turn around time again. This will also help reduce impacts after change.
Chances for a very hot fixes would have been over by above procedure. Ofcourse, there will be urgent fixes etc. But, jumping straight into code pages and doing somethings in a zig-zag way will not help fix problem and will take more time. A 15 minute meeting before such hot fixes will also make it into more stream-lined and no client, I believe, will loose anything over that 15 minutes.
Some persons/companies prepare rules that all string variables should have prefix "s" , a space has to be put before and after all binary operators, etc. I personally do not believe in them, (but follow) and reviewing to ensure such things is a waste of time.
But, there are other areas where reviewing after coding is beneficial. Whether coders have closed all DB connections after their use, whether they have applied transactions properly, exceptions are caught properly etc. has to be checked and these checkings will do a lot of good in future.
|
|
|
|
|
The Usefulness of the code reviews I participated in varied wildly. If I limited the review to one or two persons who were interested in how the code really worked then they learned something about the design of the code (hopefully) and I learned which parts needed additional comments so that I would not have to think so fast.
I did find a few errors while reviewing my own code, although no one else ever found any (Not counting the compilier).
However, most Formal reviews consisted of people who were there because they had no choice and could care less. They never looked at the code given to them in advance and never commented on anything except variable names and header comments (I have always felt that the only comments of any value in the header was a description of the design, but our standards called for listing all of our variables - which were also commented below where they were declared [with the same comment]).
My reviews always started with my asking "Any Questions?" and if no one said anything, then I would say "Thank you for reviewing my code" and pick up and leave.
Other people would read their code to us reviewers (sleepers) and never get to the design of their code. Usually these were the people who had completed their code and were not willing to make any changes. So why bother? Oh yes, our standards said we would review the code. In most of these cases, the review ended because of time exhausted, rather than code exhausted, and I would give them my marked up pages were I had found errors. I do not know if they ever looked at, much less corrected, them.
|
|
|
|
|
I'm the only developer in my unit, so even if we had code reviews I doubt anyone would know what it all meant.
|
|
|
|
|
I have the same problem. I'm the only C++ programmer in a department full of Java Devs. They do try, but explaining COM to Java devs results in an almost instant extinguishing of comprehension even consciosness, for that matter, on several occasions. Funily enough, it still keeps me careful though.
T
|
|
|
|
|
I consider myself a solo developer because I am the only one editing my code. I still do review my code, mostly using Windiff.exe. It is amazing what I find when I read my code after one or two months of work. It is like proof-reading an essay.
I worked many years for a large software company (Microsoft) and we had formal code reviews. I learned a lot by reviewing other's code as well as having my code reviewed. Code reviews are also a good way to learn what others in your team are doing. If you need to modify something, you have an idea what the code does.
|
|
|
|
|
danmorin wrote: It is amazing what I find when I read my code after one or two months of work.
I couldn't agree more. Isn’t it strange how after finishing writing an app is when you’re most likely to see the best way to do it.. I've had considerable success prototyping (hacking quickly through as much as I can), before sitting down to a more considered design process. I think some of the benefits in just sitting down and writing code got thrown out with modern methodology fashions. To a rigid rationale there is no room for factoring in the magic of such an activity. Reviews are a huge benefit but they can never be more than a hind sight tool.
I'd like to know Microsoft's opinion on this matter. You’ve had an intriguing insight seeing behind that particular veil.
Tom
|
|
|
|
|
"Isn’t it strange how after finishing writing an app is when you’re most likely to see the best way to do it.." indeed hindsight is a great thing. Code reviews must surely be seen as a luxury. In the real world it's not always possible to schedule it in to a given project, not where i work anyway...
Dan
Never get off the boat!
|
|
|
|
|
I know what you mean. Sometimes it seems strategy just can't keep ahead of the need for fire fighting. Especially, when trying to evolve a large app out of it's "rushed to first profit" version 1.0 status. My forerunner's idea of automation was to fire key clicks at the app he wanted to control.  It might have worked but he wasn't even careful about the windows he targetted. If the user was unlucky when in certain app Trillian occasionally sent out random text messages
Tom
|
|
|
|
|
TClarke wrote: To a rigid rationale there is no room for factoring in the magic of such an activity. Reviews are a huge benefit but they can never be more than a hind sight tool.
I'd like to know Microsoft's opinion on this matter. You’ve had an intriguing insight seeing behind that particular veil.
At Microsoft, each team chooses their coding styles and how they do thing. Many teams have chosen the Hungarian coding style (p for pointer, i for integer or index, etc) however some teams, like the Windows NT/XP team, has no coding style at all. As for code reviews, it is usually the developer who calls for a code review and 3 or 4 volunters agree to participate in the review. A "code review" is not a "design review"; it is to find anything wrong in the implementation such as a parameter out of range, memory leak, etc. Many times, a reviewer will ask "what those lines do?". This means the code should have more comments for those particular lines. Sometimes a reviewer may tell that there already exist a routine or object to do such and such, however this is rare because we also have time to discuss design strategies. During the design strategy, the methods and objects are suggested to speedup the implementation. In my experience, all those "meetings" were informal, requested by the developer who wrote the code.
A code review is like proof reading an essay. You do not change your story when you proof read your essay, but may correct a few sentences and perhaps insert a short paragraph.
|
|
|
|
|
That seems perfectly rational.
It seems to me that if a developer who may be writing something large and/or critical can dynamically bring to bear on the situation a larger group of experts the company's processes are succeeding. A modern day calling in the cavalry. I can think of alot of situations where that could of helped.
In my company, if that became necessary you'd hear a huge bang as a million hours of MS Project effort going up in smoke.
Thanks for the insight
|
|
|
|
|
TClarke wrote: In my company, if that became necessary you'd hear a huge bang as a million hours of MS Project effort going up in smoke.
I never used MS Project or Power Point in my entire life. In fact, I never installed them on my machines because I never had to open any of those files.
I learned UML at school however I never used it later in my life.
|
|
|
|
|
Me neither, but management love them here. Every now and again I get a few bars printed out for me. I look at them until I'm absolutely sure there's nothing useful in them, then I write useful notes on them and stick them to the wall. To the untrained eye I don't mind the distraction at all.
UML: I've still not been able percieve a design better in an abastact paradigm I use rarely than I can in the one I use every day and certainly not as quickly.
Couple that with some of the daft rubbish that's come down from our architects I don't think your missing out on much.
I've never met a decent developer who didn't know the structure almost as soon as he heard the problem.
Tom
|
|
|
|
|
It's interesting to hear your comments on UML. I am coming onto a project where UML is being used to model the business domain. It's a form of the so called model driven architecture. Basically using UML to design classes and the database to persist them.
It's *somewhat* neat, but, having spent a lot of time doing db design/class design in the natural way (with code), I find it somewhat opaque. I question the value of creating these models.
|
|
|
|
|
Instead of using some UML software, I use a whiteboard with color markers. With a whiteboard, I can draw the diagram I want, typically a class hierarchy with important data members and methods. If I like the drawing, I use my digital camera to take a snapshot of the image. The image is copied from the camera to the "doc" folder in case I want to recall the drawing later.
Most of the time, just drawing something and brainstorming is sufficient. It is like discussing a [computer] problem with a colleague - it helps to generate news ideas to solve the problem.
|
|
|
|
|
It's worth remembering that UML is just another logical language.
As such, if your familiar enough with it you can use it to design systems. It's just an abstract document that's an aid to thought. Writing UML is writing one totally abstract document to describe another less abstract document(your code).
However, you can't compile it and it's form won't betray any system particular issues. UML is entirely ignorant to peculiarities of any OS or libraries. The structure of a applicationis obviously crucial but most development issues arise from the libraries and technologies we use.
I don't mean to sound like I'm writing UML off. It's our most successful effort yet as a tool to aid system thinking but while it may be capable of describing a full complement of system structures(so can most languages though), in my opinion it's clunky aesthetic is still a long way from any real connection with the way the brain sees theses things.
Worth learning, even worth using but beware, its not finished. There are so many ways of chopping up problems and presenting them to our brains in a way that accelerates subconscious reasoning.
Tom
Tom
|
|
|
|
|
Whiteboard rule! I'd prefer to jot down my ideas on a white board and have other developers engage in a discussion on design etc.
We made the buttons on the screen look so good you'll want to lick them. Steve Jobs
|
|
|
|
|
I've always worked alone and never had the option of a code review. On the other hand, I have viewed, fixed, modified, and redisgned thousands of lines of other peoples code that came in from outside the company. So I guess that would make me an expert reviewer.
Does anyone need a full time C/C++ code reviewer?
INTP
"Program testing can be used to show the presence of bugs, but never to show their absence."Edsger Dijkstra
|
|
|
|
|
We have an SPI (Software Process Improvement) team which is part of the Architecture group that monitors code across all the projects. It is informal in that you aren't pulled up in front of a panel and reviewed but it is formal in that there are people tasked to review code and standards to be met.
We also then do occasional presentations between projects on architecture and code in the hope that fresh eyes will spot places for improvement.
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
