Code Reviews- Do They Really Exist, In Practice? 36
dante101vr2 asks: "While working in the programming field I have heard a lot of mention to doing code reviews, however I have yet to see one occur. Along with my job, I have also talked to a number of others throughout the IT community who laugh when they hear mention of a code review, claiming that there is not enough time in the day. My question to you is, do code reviews exist, and are they a good tool or are they just time consuming?" For those of you who do have normal code reviews, what small things do you do to make the whole process go along as painlessly as possible?
They do exist (Score:1)
In "more important" environments (like NASA or medical software), they're probably a lot more common. But where I work (making video games), we tried it once and then decided it just wasn't worth it. Mostly because all the crappy stuff it turned up would take years to fix... ignorance is bliss.
Yes they do. (Score:2)
They need to be enforced from a high level, and a committment from management to take the time to do them. We have a mandatory checkless, no new feature can get into system test unless it is complete. On item is a code review. (Documentation is anouther) Test gets to decide what they will allow in test and if code reviews weren't done it isn't allowed.
Our cheif archatec insists that he review a lot of code to, and he is serious about it.
Code reviews are not allowed to last longer then 2 hours. If you need more time (and you almost always do!) then do it anouther day.
Re:Two key problems that can come up (Score:1)
People who think that their work is so good that it doesn't need to be reviewed are wrong. These are the very people who need their code to be reviewed. *Everyone* can be a better developer. *Noone* has nothing more to learn.
You can make it easier, though. Allow peers to review each-others code rather than stipulating that it's the team-leads job. Make sure that there's a review meeting rather than just an exchange of paper, and that meeting should be a discussion (the reviewer could be wrong too).
Reviews are so useful in increasing the quality of the deliverable and of the developers themselves that it's worth annoying non-believers. They'll come around eventually.
Re:They do exist (Score:1)
Me too, sort of. We tried it, we agreed it was a good thing and we ought to do more - but they never actually happened, because the short term time pressure never left time to do it, even though we agreed it almost certainly would save time in the long run.
--
Two key problems that can come up (Score:2)
1) A code review is essentially judgemental. Even when the reviewer(s) are careful to "review the code, not the coder", the nature of the interaction is that person A looks at person B's work product and perhaps finds it less than good. This can be uncomfortable for the reviewer (who doesn't want to say bad things about a coworker's code) and the reviewee (who might be hesitant to see or admit weakenesses in their work product). Reviewers often hold back valid issues for this reason.
2) Reasonable people can disagree. Sometimes it's the difference between a weakness and a design decision is unclear.
Rare, but they can work very well. (Score:5)
My first exposure to something like code reviews was writers' workshops; everyone submits a poem, story, whatever, and then everyone critiques everyone's works. From this, I learned the first rule of both writers' workshops and of code reviews:
REVIEW THE WRITING, NOT THE WRITER
Being reviewed is tough on the author's ego; spare him or her as much as you can. If everyone gets a turn in the hot seat, they'll learn the difference between flamage and constructive criticism.
Here's the second rule: People hate performing less than they hate preparing for code reviews. It's heresy, but I've had a lot of luck with zero-prep code reviews: Everyone shows up, gets a printout, and away we all go. It can take a lot of time in the review, but the total time spent is smaller (and, IMHO, more effective). In reality, people skip the preparation; why not plan for it?
Third rule: Reviews identify problems, not solutions. You don't have to know a better way to know something's wrong.
Fourth rule: The author is there to learn, and to answer questions, not to be defensive.
I brought code reviews into the small startup I joined a few years ago. Final rule: everything deployed to production gets reviewed. It worked; not only did my co-workers accept it, but at least one of them, when he left for another job, got people there to do code reviews the same way.
Code reviews aren't just for finding bugs; they're for improving the quality of the code, for whatever your local definition of "quality" is. I've always considered maintainability to be fair game.
My current project has a fairly rigid process, put into place long before I arrived. Here, they do desk checks and call them "reviews." It's not as good, but it's better than nothing.
Well, we tried. (Score:4)
I've done "mini reviews" where I randomly grab code from the repository, and when I find something that looks interesting, I start a message thread to discuss the pros and cons of it with the team.
Code review for us meant inviting 5 people : the original code author, a moderator, and 3 reviewers (although the moderator can also be a reviewer). Everybody was told what code would be reviewed, and given printouts so they could read up. Then we took 2 hours where the moderator runs through the code and the reviewers offer comments to the author. I think we did somewhere between 1000-2000 lines of code that day. The end of the code review is a list of items that can be changed in the code, broken down by priority -- should be, might be, etc.. Don't feel like you have to put the whole team in a room! It will be impossible to schedule, and you won't get through any code at all because people will spend too much time arguing about the details.
Don't invite the boss until you've got a good process in place. Otherwise people will wait for the boss (who is probably not a fulltime coder) to lead the meeting, when in reality he probably just wants to listen. The idea is that it is a peer review, not a criticism by your superiors. Likewise, have a good mix of junior/senior programmers -- everybody's opinion counts. When I ran the above example I put names in a hat and drew them randomly.
Making it mandatory is important, in my book. You need enough reviewers to make the discussion interesting.
Also, decide in advance what the scope of your code review is! Some people will want to talk about indenting styles that they don't like, or schemes for naming variables. Is that what you want to talk about? Do you want to focus on things like uncaught exceptions and null pointers? Or something broader, like whether a particular path through the state machine has been handled properly? You have to tell them up front, otherwise you'll get a wide range of opinions on the level of detail that you should be discussing.
If people aren't used to code reviews, it's important to explain to them that the benefit comes in solving the problem before it happens! When you see a method that doesn't have a scope declaration on it, it looks completely trivial to point that out. But it's saving you time later (I really did have a case where we'd created a new package and were trying to extend some old code from a different one. The junior programmers on the case spent quite awhile (days?) figuring out why it didn't work, and finally decided to just not use a new package. Turns out that a good part of the code in the old package was using the default package-private scope and thus not extendable into a new package).
Unit test! If you can't get people to review the code, get them to review the unit tests.
Wherever possible, review new code. The biggest problem I have here is that we've got a 3 year headstart on writing code without reviews, and now I can't rally anybody into meetings to go back and review existing code. When I do, any changes that are needed don't become a high priority. Make it a point that when new code is being created, just make the code review a part of that process.
Duane
We do them (Score:1)
The code reviews are formalized, and result in a list of changes required to the code. When the developer makes the changes, the changes are once again reviewed.
The design and flow of the code is not reviewed here, since that was done when the detailed design was written and reviewed. However, the code is verified to follow the design.
For code that has been code reviewed and accepted, minor bug fixes do not go through a formal review. However, a senior devloper must be brought in to view the changes and confirm them.
Major code changes go through another code review process.
I find code reviews (and all of the other peer reviews in the process) to be helpful, although they are time-consuming. We tend to find actual code and logic problems more in the junior and intermediate developers work, but they do occur in some seniors work as well.
Most of out code reviews find problems in the devlopers not following our coding style guidelines.
Re:Two key problems that can come up (Score:1)
If the company has coding style guidelines, and a proper detailed design document has been written and approved, then the code review cannot be judgemental. Granted, discussion about whether to use a do..while or a while loop may come up, but that decision is the devlopers not the reviewers. As long as the code is not buggy. Through the discussion, the coder may find that a do...while would be better suited to the problem at hand, and then change the code, but it should not be a formal request that comes out of the code review.
Of course, if the coder does change from a while to a do...while, then his/her changes need to be reviewed again.
As to your point number 2, an approved Detail Design Document should solve that problem. A Detailed Design document also needs to go through a review process.
Re:They do exist - and yes they are very useful. (Score:2)
In that case, I'd say there are a few serious problems with your working environment. I'll bet that at least one of the following apply:
1. Your code is being reviewed by people that are not programmers. I can't really see anything too productive coming out of this. If you're working at a place where unqualified people are conducting code reviews, maybe you should start looking around a bit for other opportunities
2. You're obfuscating your code (either on purpose, or just out of habit). Anyone who is not commenting their code decently (excessive verbosity is almost as bad as not commenting at all!), or who is writing muddled, unclear code, is hurting the team and company more than helping it. There is a reason code standards exist, and while nobody likes being told how to style their code, they work.
3. You're using Perl.
Ok, I just don't like Perl. I know it's got lots going for it, readability and consistency aren't some of them. (I'd better stop now before some militant Perl fanatics show up at my doorstep...)
"Intelligence is the ability to avoid doing work, yet getting the work done".
Pair programming == instant code review (Score:1)
http://pairprogramming.com
I've found that other approaches - i.e., the draconian "no code gets checked in unless it has been through a formal review" just leads to people not wanting to write code. So any crap that makes it through the review stays in the system for a long time because it's such a hassle to jump through the hoops to change it.
Yours,
Tom
I have a few more cents to contribute (Score:1)
Use code reviews to develop coding standards. It's okay if you don't have coding standards, but once you debate a style issue, document it so you don't have have that debate again.
Code reviews are great CYA. Let's say Developer A gets to maintain Developer B's code. Now, if Developer A is cutthroat/incompetent/lazy/insecure, they might be inclined to rip on Developer B's code as they're looking at it, possibly in the earshot of peers or managers. Involve your peers in the code review in the first place! I think it can shut up a lot of people. They have an opportunity to openly rip on your code, in front of the boss and everything. In my experience, folks shy away from such openness -- and if they do rip on your code and have a valid point, a better application is the result. If they're wrong, they get pointed out as wrong. (Yes, politics could get in the way here -- possibly I've just been lucky in not seeing this be a problem.)
Make code review attendance mandatory. Not in a "do it or else" sense, but in a sense of "do, or if not make sure you have a good reason, pre-approved by your manager, why you cannot participate." Code reviews are not fun, but I think you can learn a lot about good coding practices by questioning another or by having to answer questions about how you do things.
Code reviews can help reuse. In code reviews I've gotten a chance to see code worked on by other developer groups, and saw some neat objects that I've since used in my applications. I think politics (and time schedules) can get in the way of some formal sharing and reuse. This lurking type of sharing/reuse has helped me on more than one occasion. (Yes, I leave the original programmer's comment headers in place to give credit where it's due :-)
yes, but rare (Score:2)
I've worked in two environments where code reviews took place.
When I worked at TIS on the Trusted Mach [tis.com] project, code had to be reviewed by the "trust engineers" for compliance with the security model. Problem was that some of these folks, while very knowledgable about the theory of building a trusted operating system, were not all that knowledgable about C++, and a lot of their problems - in some cases, the vast majority - were cleared by developers explaining basic programming issues.
A few years later I worked at TRW on EDOS [nasa.gov]. Code reviews were a mandatory part of the process. Unfortunately, in my experience they ended up more focused on ensuring that the code met the style standards (indentation, brace placement, format of function headers, and the like) than on the proper functioning of the code. (Of course, that may be because my code was just so good that no one had any suggestions, questions, or issues other than where I put the curly braces. B-) )
Tom Swiss | the infamous tms | http://www.infamous.net/
Re:They do exist - and yes they are very useful. (Score:1)
4. The other coders are beginners compared to the poster. There is a whole world of difference between a beginner programmer who is just about capable of writing a simple console program with one main method and no other methods (yes, they're "barely qualified" at best, but they do exist in the industry), and a Kung-fu expert Java or C or C++ programmer. In the real world, the inability to understand pointers, for instance, cannot easily be remedied with either comments or coding standards.
We do too :) (Score:1)
My experience (Score:2)
When I've done pair programming (in an Extreme Programming [extremeprogramming.org] context) I feel like I get most of the benefits of a code review, but over 100% of my code. And since the reviewing is integrated into the coding process, I always feel like progress is being made. Indeed, now solo programming feels a little unsafe to me, like swimming in the ocean with nobody around to notice if I get sucked out to sea.
passive code reviews (Score:2)
that said, i'm sure the formal code review has its place. but who really wants to work that way?
Yes and No, Good and Bad (Score:2)
I've worked at a large government contracting comapny, and they had code reviews as part of the ISO process. Which meant that they occured regularly. And I've also worked at a small company, where everyone said "We should do reviews" but then never did. I take that back... they tried one, no one prepared for it, so it flopped, and a follow up was never scheduled.
Now I'm not saying that preparation is absolutely necessary. If everyone is familiar with the code, they can do a good impromptu review. But if you have no clue what the code does, you probably won't be able to comment on anything but the style , unless the code is pretty rudimentary. Also, giving people a chance to read through it at leisure lets you have a relatively quick meeting as people run down their red-lines rather than sitting and reading code in a conference room.
And when you actually review, review the code, not the coder! Some people tend to turn code reviews into an ego driven contest. They may not release any code until no one will be able to find a single bug when it comes up for review. Others may pore over code, desperate to find any bugs they can, in order to bash the reviewee. Once you get into this kind of situation, no one is productive since they're way to concerned with the reviews to actually do anything else.
But I've also seen very good reviews where constructive criticism came up, suggestions were given to improve architecture or code flow, and people on both sides learned something. And that's what you have to keep striving for, because it may not come very easily.
Force Yourself (Score:1)
Voila! Gather a meeting with the print outs and you have instant code review. At least, that is what we do.
Jeremy
Usually first thing to get cut (Score:1)
Design review at my last company (Score:2)
They do exist - and yes they are very useful. (Score:3)
Does this procedure help? I'd say it does - especially with a product as large and complicated as DB2 UDB, there may be side effects that are not obvious to just one programmer. There may be logic problems that you have missed which are clear to another person. Bad coding style leading to problematic maintenance problems in the future can be caught.
Just as nobody would dream of submitting a major article for publication in academia without having it proof read by at least one peer, it doesn't help a large product to have large volumes of code stuffed into it without somebody other than the original author looking over it. So while I used to find the process of code reviews cumbersome and time consuming, I now feel that code reviews save more time and energy in the long run.
Cheers,
Toby Haynes
Our Review Experience (Score:1)
We had (dot-com bust-up means that we no longer do anything) a reasonably comprehensive review process as part of our development process. The whole project team (developers, managers and QA) would show up to a project kick-off where the requirements would be discussed, and then meet again for a design review. This made sure that everyone was on the same wavelength, and had some idea of how the rest of the project hung together. Then the coders went off and did lower-level design and coding, and the QA folk wrote test plans and tests. Once the coding was largely complete then code reviews were done, and then the formal testing by the QA engineers.
The code reviews were moderated by a QA engineer, and the author and a reviewer or two. Because we had a coding standard (it wasn't too heavy, and had good buy-in from the developers) the review could concerntrate on making sure that the logic of the code was correct, and that all the requirements that were supposed to be covered were sorted. This usually took an hour or two, plus about that in preparation too. Everyone's code got reviewed, not just junior coders, so that everyone was exposed to all sorts of code, and junior people or those new to a language could learn from seniors and pick up house idioms.
This might sound a little over the top, and sometimes it was, so if everyone involved in a project agreed, we decided which steps to skip or shrink. This meant that we could use the full process for big or important projects, but left us with a structure for smaller, quicker projects. Our usual project timeframe was in the few weeks to few months, and this seemed to work. It's just a pity our company got bought by an organisation whose business plan didn't involve revenue.
Re:They do exist - and yes they are very useful. (Score:1)
"Peer" being the operative word. In my previous job, where I did have peers that understood the sort of things I was doing, we never actually had code reviews -- no time. In my current job, I submit code to review all the time, but none of the reviewers actually understand it.
Don't get me wrong. It still helps. I have the sort of mind that can see other peoples' mistakes in a flash, but not my own. Not, that is, until I start to explain them to somebody else. So I still boost the practice.
Re:They do exist - and yes they are very useful. (Score:1)
I don't obfuscate code, partly because I have no reason to hide what I'm doing, but also because I often have to go back into that stuff myself and add features or tweaks. But all the other reasons you advanced, plus the one by a followup poster, apply. C'est la vie.
Re:Force Yourself (Score:1)
:)
At my last shop we emailed diffs of our changes before checkin, which worked quite well in an environment where lots of people worked remotely.
Where I am now there isn't really a review process, and I think it really hurts both the code quality and the developers' level of understanding of those areas of the code where they don't work often.
Y2K (Score:2)
Late 1998, I did contract work at a long-distance company. I worked on a Y2K-compliance team. We did weekly code reviews, just to make sure nobody missed anything, or broke more than they fixed. That was the one and only time I did a code review outside academia.
Definitely worth it (Score:1)
30 years, regular reviews at all levels (Score:4)
Portions of my comments will seem redundant, but I think that there is enough new material for the entire essay to be useful. I'll see how the moderators feel about that, of course. :)
My experience extends over three decades, and I'm here to say that product implementation reviews are indeed real, in academic and industrial enviornments.
First off, the review cycle is more than just reviews of the code -- the best projects had reviews at every major milestone. This was especially true of those milestones that required the ball be handed off to the next group to work on the project.
Some reviews are major deals: marketing definition review, product design review, and development assignment review. Some reviews are relatively minor, but just as important: detailed design reviews of hardware and software, post-implementation reviews ditto, and integration reviews. The difference is the number of people involved. Major reviews required a fairly large conference room, and in the instance of development assignment the companies I worked for would book a large ballroom to hold all the people involved in the project. (Smaller projects took over the lunchroom.) The other reviews were two- and three-people affairs: peer review and critique would have you handing over notes and source to a peer, and the two of you plus the supervisor (task lead, group lead, whatever) would go over the results. Integration reviews may involve as many as five to ten people, depending on the level and diversity of the elements being integrated.
Then there were the bug-track reviews...
By the way, in one small company the peer reviews were very informal -- sometimes the review would be done over a cup of coffee during a break, especially when the patch was small and localized...but the review was done because it saved so much grief down the road. Especially grief from The Boss.
There was another part to reviews in the small company that was lacking in the large companies I worked for: documentation reviews. Part of the job at the small company was that any change to the code required changes to the corresponding interface, control block, and other design documentation. No change was allowed to go through without the documentation changes being included in the change report. It was the only way for eight programmers to be able to provide maintenance support for 19 man-years of assembler code AND be able to also develop new products.
The best review system? The one that let people learn about and fix mistakes with the minimum of fuss. At ALL levels. One of the first things I learned was that promoting the people who made the fewest mistakes was a recipe for disaster, because it penalized people for being aggressive with implementation so that the company could satisfy customers. Granted, sloppy programming should be frowned on, but the whole point of the review system was to let the system catch any mistakes so each person didn't have to waste time going through code sixteen jillion times to try to find that "last bug". Better to let a new pair of eyes catch it quickly.
When you analyse all the "systems" of developing programs that have been promoted over the past five decades, it comes down to having multiple eyeballs catch the silly stuff and the tricky mistakes, and to not allow sloppy stuff to even enter the stream. For if you know that your code will be seen by others, you take pains to not only get it right but also to get it pretty.
This last, to me, is one of the benefits of OSS development -- you will be judged by a LOT of people. :)
Seldom, but they're worth the time! (Score:3)
We've started the (sometimes painful) habit of doing a "post-mortem" of each release, and start the next round of coding with a code review. Although it would be better to do it 3/4 the way through the development cycle, we've found that this is the only place we can "squeeze it in" and still be able to spend sufficient time on it to be beneficial.
There's nothing better for catching potential problems, learning other areas of the system better, and forcing people to justify their coding practices (or change them for the next round) than a code review.
We've also found that it helps the junior programmers by having a more experienced person evaluate their code, and make suggestions.
So, it's more of a post-mortem analysis, sometimes too late to actually end up with improvements to the current code, but it's helped our team prosper, and sets the pace for the next iteration.
MadCow.
Re:They do exist (Score:1)
I've heard the same thing about just about any process improvement that, when done right, can improve the quality of your product. Controlling your code baseline, knowing what your requirements are, testing your code, knowing the status of your project. They all involve discipline and perseverance, and there are plenty of excuses for not doing them at the first sign of resistance.
The proper name for this kind of resistance is "passive agression."
The real problem with code reviews is finding two people who know enough about the problem domain and the technologies being used to do the review. In a lot of firms, even one qualified person is hard to find. Having your code reviewed by someone who doesn't have the faintest idea what you're doing might be good training for them, but it's not a review. I suspect this is one reason firms try it once, then throw up their hands.
The trick is not to starve projects of resources (like enough qualified coders). Good luck convincing the managers!
Re:Our Review Experience (Score:1)
I've found that it was better to have the best techie (TD or group tech lead) run the review. It takes up some of their time, but it makes the review an occasion for mentoring rather than nitpicking.
The tailoring suggestion, though, is a good one. Just be sure that steps aren't being tailored out in order to hide embarrassing screw-ups. The people who think they need least review are often the ones who need it most. If I saw a lot of resistance, I'd run those reviews myself (I was in charge of the project).
And as to the "taking it personally" problem mentioned in other posts: encourage the people who personalize issues (either pro or con) to leave. Reviews give a good opportunity to see who those people are. Backstabbers and prima donnas will kill your project and your business, no matter how technically capable they might be. Reviews will be hell for all concerned if that kind of behavior is tolerated. System development is a collaborative effort, and that means collaboration is a necessary part of the job. Much of the resistance to reviews is a denial of that fundamental fact of life.
Re:They do exist - and yes they are very useful. (Score:1)
They say if its useful to do code reviews, it should be useful to do them all the time. ie. Pair programming.
They claim to have evidence that it works better in the long run than anything else...
Yes, and they can be useful (Score:3)
- engineering co-ops
- newer programmers
- changes to core code
- bug fixes done near deadlines
This last one kept more than one serious bug from going out the door. When the pressure is on, there's a temptation to make "just a little fix" that frequently would introduce a new problem. When a freeze date was close, the build manager for a project would review any changes before allowing them to be installed. Code reviews were also very helpful for engineering co-ops and other newer programmers. As much coding as you've done in school or on your own, you were probably the only one besides the grader that's ever seen it, and graders usually weren't looking for good code, but for code that worked the way the professor wanted it to (I had one exception to that; the prof for my OS course required following a coding convention.) A code review for newer programmers helps more experienced programmers impart their wisdom about coding conventions (every large project should have one), choice of data structures and algorithms, and documentation & comments (every large project should have someOnce or twice... (Score:1)
I'm a consultant and have only been on one project in which code reviews were done. In theory, at least. When we first started development, we vowed on a code review a week, maybe two. There were two code reviews, both, actually, of my code, which were even useful. The code was printed and distributed to all developers and the QA guy, and everyone looked it over and gave feedback. It was useful. I was impressed. The first time. Then, two days later they picked apart my code again, because one of the larger egos on the team couldn't make it the first time. This was not useful.
It was also the last code review that was done.
Two months later, when the project's deadline was racing toward us at a breakneck speed, it was discovered that another of the developers had hard coded a whole chunk of the EJBs we were using. Needless to say, management demanded to know why we hadn't done more frequent code reviews. The answer? Not enough time.
If you do an hour code review, it takes at least a half an hour to prep, for both the reviewers and the reviewee. When you're on a tight deadline, you don't always have the luxury of a half hour.
beth
Design Reviews (Score:1)