Slashdot Mirror
Are Code Reviews Worth It?
JamaicaBay writes "I'm a development manager, and the other day my boss and I got into an argument over whether it's worth doing code reviews. In my shop we've done both code reviews and design reviews. They are all programmer-led. What we've found is that code reviews take forever and tend to reveal less than good UI-level testing would. The payback on design reviews, meanwhile, is tremendous. Our code is intended for desktop, non-critical use, so I asked my boss to consider whether it was worth spending so much time on examining built code, given our experience not getting much out of it. I'm wondering whether the Slashdot crowd's experience has been similar."
No.
Discussion over, everyone go home.
Having worked on life critical type systems where every line of code was reviewed before making it into the product, I have to say that I've seen them add a lot of value when done properly.
They also cost a lot.
The first question I would ask in your situation is: are you doing them right?
Do bugs get discovered later after deployment? Are the bugs in areas of the code that were supposedly reviewed? If so, you might be doing it wrong.
And as much as we _hate_ the word... I have to say it...
METRICS!
If you truly want to make a decision on whether code reviews are worth it.. you need to know:
- how much does it cost to conduct the reviews
- how many defects are discovered in the review versus how many make it out the door (in other words, how good are you at it)
- how how much more does it cost you when a bug gets discovered after deployment? In a life critical system, it costs a fucktonne.. in a desktop app.. it may not be that bad.
Once you know these, the picture should be clear
Code reviews have a lot of value in two ways completely independent of how good they are at catching errors. First, they are a way to enforce various stylistic guidelines on code that make future maintenance much easier. Secondly, they are a tool for spreading knowledge about the code around to other members of your development team.
They can also catch some errors that are very hard to catch in any other way. I recently worked on a project in which I found an error that would've caused code to only fail in a very limited and non-obvious set of circumstances. The thing is, somebody would've almost inevitably encountered those circumstances and the phantom and nearly unrepeatable bug reports resulting from this would likely have never been solved.
I fear code ever stepping off into the land of incorrect behavior as there are few corrective mechanisms that will cajole the errant program back into doing something sane. The longer it goes without abject failure, the more weirdly wrong it will be. Therefore I think any and all measures to keep it from ever going there and making sure it dies as quickly as possible if it does are useful.
Need a Python, C++, Unix, Linux develop
Where I work we have code reviews at various degrees. I find someone else just reading over my code and emailing suggestions helps tons; It spots obvious errors, ways code could be done better, and just a ton of other things. It helps improves my own coding style too.
http://studentseeksnoodles.blogspot.com: General thoughts of an
Code Reviews are useful when they are done right. But before you start using code reviews you should introduce automated static analysis of the code during the builds. A lot of crap can be discovered by static analysis. This saves you a lot of effort on the tedious parts of code reviews.
then you're doing it wrong. Plain and simple.
Code reviews shouldn't be a "full stop, let's go over this" event. Code review should be a part of the every day workings of the development team. Nothing should go into version control's master/trunk/HEAD until it has been reviewed.
Sometimes you'll find that stopping to review a single module is helpful, but most of the time it's actively harmful to the team, since it takes developer's concentration off of what they're currently working on to review things that they half-don't-remember, which then makes the code review take forever.
Review and document inline with your coding, and you'll find you'll never need a "Full Stop" review.
"Victory means exit strategy, and it's important for the President to explain to us what the exit strategy is." G.W.Bush
When we did code reviews at DEC, they were done with several people generally familiar with the code, required considerable advance prep by the reviewers, and went over code basically line by line. This was not done for all code; nobody had time or resources. However, when a piece of code was doing something weird, it was a pretty effective way to figure out what might be wrong, where the programmer had been having trouble finding a problem. The one area where it tended not to work well was where the software was a driver that talked to some piece of hardware, and the programmer was the only one who really knew what the hardware was doing. The fact that the reviewers didn't know that hardware made finding bugs most difficult...
What we've found is that code reviews take forever...
Ugh. Are you reviewing each individual commit (where code reviews are quick and very effective), or are you rounding up a bunch of developers in a conference room and reviewing an entire module using an overhead projector?
Peer-to-peer reviews of individual diffs using good workflow tools have been very effective at several places I have worked and in open-source projects to which I have committed.
Some of the fastest team development velocity I have experienced has been with peer code reviews within the team.
A good style guide also helps...
Yesterday it worked; today it is not working; Windows is like that...
Have each developer line up with his/her/its code printed out on a cue card board and stand in a line up. Execute the least likely to make it to the compiler, then you will "motivate" the rest to write better code..
My group started implementing code reviews this month. To get in the swing of things, we decided to do a test review of existing foundation code, stuff that had been in the product for many months, had gone through multiple QA cycles, and had been shipped to the customer as part of the first "production" release.
In the first hour-long review, we found a number of significant issues, and one full-blown bug.
The cake is a pie
Design reviews are useful to catch problems early on, particularly the selection of poor algorithms or data structures. However, in the the software shops I've worked nobody does any documentation, which makes it particularly difficult to do a design review. So, as you can imagine, I haven't got much experience with this area.
Code reviews, on the other hand, are more about auditing code to make sure that people are following the coding standards and policies. After all, if you've got coding standards, how are you supposed to tell if anybody is following them without reviewing the code? Once your coding standards have been institutionalized -- that is, most people have internalized them and following them -- then what's the point of a code review?
So your best bet, then, is to reserve code reviews for junior developers until the coding standards are internalized; and use design reviews for the architects.
Just wondering
Even the best programmers make mistakes. Having another set of eyes is invaluable for detecting bugs before they become problems. Having to explain in words the rationale for a design decision often helps you better understand your own design, and to see potential problems with it. Sometimes you come up with something better on the spot. Also, if you get hit by a bus, your fellow programmers can take over without having to reverse-engineer your thoughts. Please, more code reviews.
As someone else noted, badly-run code reviews aren't worth much, if anything.
There was a lot written about code reviewing in the late 80's and early 90's that makes sense. If a review is conducted as a lesson in coding to others, nobody is going to get much out of it. If it is done as a last-ditch design review, that probably isn't going to work out well either.
If the staff is all people with lots of experience, it may not be that valuable. Alternatively, I see it as an extremely powerful tool for a staff that works mostly independently to come back together periodically and make sure everyone is on the same page. Especially when some team members have less experience.
Trying to bog it down with formality is pointless. But the early guidelines about "egoless" are right on target.
Where I work we do code reviews and they are definitly worth the time. 60% of the time the review doesn't flag anything. But by having anouther coder look at the code you can find those points when a comment would be very usefull, where an algorythm might break down to simple typo's , complexity issues and general readability
Code reviews are as much about code maintainability and ensuring the code follows standards then finding bugs.
Part of the PCI-DSS (Payment Card Industry Data Security Standards) security requirements is to conduct quarterly code reviews of applications that process credit card data, or put an application firewall on your network to monitor these applications.
Like most companies, we spent about 10 minutes working out how much time our developers would have to spend on this per quarter - and then we decided to drop $30K on the firewall.
then code reviews would be redundant. Sure, testing first is expensive. But probably no more expensive than code reviews. If you have a QA person or team checking the final product, then your bases are covered.
Even if bugs are not found during code reviews, code reviews help the developers know that they are accountable for their code.
How many per cent of your programmers change, i.e. move to other companies, every year? The higher rate, the more need for well written, easy-to-read code.
I have no doubts at all that code reviews are highly useful. They can discover coding errors that are insanely hard to discover other ways. In anything that was critical, I would advocate them without the slightest hesitation. However, since this about non-critical stuff, then it is far more questionable. I think you should still use them, but on more limit basis. There will always be the tasks that are highly technical internally that need a good thorough looking through the code as well as UI test, but generally it becomes much less useful in more simplistic tasks, and UI testing will quite often (still not always, but most often) reveal the same errors. A good random inspection might be a decent replacement under these circumstances.
Well, my last workplace had code reviews for everything, and I found them tremendously helpful. They accomplish a few things:
Furthermore, if I edit code that was written by (or is owned by) Bill, I'll ask him to review it so he'll know about the new feature I added (which is good, if he ends up having to support it).
A cat can't teach a dog to bark.
If you have padawan coders, or coders from another corporation.. code reviews matter.. stylistically they wont mesh, they may have some foreign habits that you need to either squash or embrace. These may be the people who do a C++ style for loop in perl when it is not appropriate. These may be best-practices nazis. These all warrant code review.
If you are modifying byzantine code from the people who write HP office-jet software.. you need code review.
If youre hooking up to an airplane or nuclear reactor.. you better know the answer.
If you have grizzled coders who are stylistically homogenized, doing work that can have bugs... IE microsoft checkers (which allows you to change checkers in mid-double jump).. code review isnt so nessisary..
Storm
Code reviews are of limited use for finding errors, and are certainly no match for structured and comprehensive tests.
Code reviews are one-shot, while properly written test-suites (on module, integration and system level) are repeatable. This means that you can easily run your test suites again when you change your code, but redoing the code reviews is too great a task. You simply can't re-review your code every time you make changes, but you CAN re-run properly automated test suites.
Secondly, test suites are deterministic while code reviews aren't. With test suites you can be certain to catch all the error conditions you are testing for, whereas code reviews have random results, depending on the experience, capabilities and attention of the reviewers. You don't want your bug hunting to be non-deterministic
Put your effort into proper testing, and use code reviews only when everything else fails, or in cases where they're used for enforcing coding guidelines or teaching people how the code works. Then once you got a proper test suite done, maintain it and expand it as you add code and functionality, and more importantly add new test cases every time you encounter a bug.
A code review is unlikely to uncover many errors. Most code is just too complex for another developer to spot errors. Unit testing is much better at that. What a code review can do is
I often don't like the choices people make, but I like the fact that people make choices. That's why I'm a conservative.
Now maybe there are reasons that reviews should be conducted in one way or another. My preference is formal group reviews for brand new code pr new team developers and over-the-shoulder reviews for substantial checkins thereafter. Between the two I guarantee you save more time than if the bugs went in and were only uncovered later in QA or worse, in production.
You often *have* to review a entry level programmer's work until it reaches an acceptable quality. I consider code reviews as a method of improving the programmer more so than the code. One an engineer is producing generally acceptable code it becomes safe enough to treat their code as a black box and wait for problems to be unearthed by testing. If you are shipping bugs your problem is testing, not code reviews. Finally, the cheapest way to do code reviews is for a manager to just scan submitted code from time to time and send out polite emails if they see something amiss. On the other hand getting five senior guys in a room to discuss the work of another senior engineer is a just going to result in unproductive, cranky engineers.
-- http://thegirlorthecar.com funny dating game for guys
Code reviews are worth it but often the "cost" of doing proper reviews are underestimated. To implement results of the review may cost a bit of time and money if not another review where if project managers and planners just assume its a "fast fix" then problems will arrise.
You should check out this reference: (Apologies, you need access to the ACM Library to read the article) http://portal.acm.org/citation.cfm?id=299161. Robert Glass discusses this exact issue. The article offers some references to research done using alternative approaches to inspections.
I don't know why the parent is modded "troll", he's 100 percent correct. And they get curry EVERYWHERE between the 1's and 0's, it's just a mess.
For a project that's more than one or two people, code reviews are vital. They enforce consistency in style and methodology. They also spread knowledge. They mitigate the "bus factor" (how negatively impacted your project will be if someone is hit by a bus). I've worked professionally at two companies. One of them did not have code reviews. Every time someone left the company, the project either grinds to a halt or they have to rewrite that person's work because no one else can understand it. There were a ton of rewrites. At my current company, we use Review Board (open source, built on Django) to do code reviews. People don't leave often but they do transfer around the team to work on different sections. That's rarely ever an issue because the code is understandable by all of us. I work in both the UI and the middle layer with business logic and occasionally DB related code. I can move between the three because the consistency that exists. When I move into a new section, I do make mistakes but those mistakes are caught during code review and I learn.
EvilCON - Made Famous by
My working life has been spent in projects developed by individuals or small teams (less than six programmers). I would describe my working environments as "CMM level 1 and damn proud of it." The teams I have been on have been consistently successful without having a consistent methodology. The success is the result of having a bunch of competent people who respect each other, and are motivated by the idea of producing something that customers will pay money for, because it works. And by the knowledge that they'll stick with the project for a while, and if they make any messes they'll be the same people who have to clean them up later.
I've suffered throughout my working life from inexperienced managers and programmers who suffer from what I call delusions of grandeur. 90% of the stuff that people learn about project management seems to me to be intended for use in projects with fifty programmers or more. Projects where the manager can't get a grasp of the project by walking around and schmoozing with people. Projects that are big enough that there are constantly people leaving and joining them. I don't know about projects like that. I'm inclined to think that formal management processes may be useful, even essential there.
But on small-team projects, it just gets in the way.
The problem is that the books that explain the capital-M Methodologies and the code review process and so forth never say, in so many words, that these are management procedures for projects with more than fifty people. They just say, "this is how you do it." And people come out of courses believing that "doing it right" means applying all this stuff. And that anything else is unprofessional.
The hidden assumption is that everyone wants to follow a career path where they manage more and more and more people on bigger and bigger and bigger projects and make more and more and more money. When I worked at a Fortune 500 company, people were very frank about it. It was a waste of time proposing or working on small stuff. You had to "think big" or management would never take any interest in what you were doing.
Well, I'm here to say that if you want to think big and manage like the big boys do, fine. But don't try it on a small-team project where the team members have gelled into a coherent unit, and know each other and work well together, and plan to stick around for a while.
Code reviews? Gimme a break. In the natural course of events, other team members are going to have to work with my code. If I don't care about what they think about it _when they're editing it to get their job done,_ I'm sure not going to care what they think about it in some room with a whiteboard. And we're effectively eviewing each others' code in the natural course of getting our jobs done, then sealing ourselves into a room with a whiteboard and no debugger, isn't going to be any better than what we naturally do without any formal process.
Quite simply, code reviews cost money and production bugs cost money.
We do code reviews for anything where it'll either be devastating expensive if we encounter a failure or if it'll be very hard to detect a failure. Otherwise, in my particular line of work, it's more economical to accept a lower cost and faster pace of development at the expense of dealing with a few bugs that are discovered in production.
Code review is often viewed as a compliance task, not productive. This video talks about what code review can be, and how to make it more effective.
Code Reviews
The simplest bug found post-ship is a helluva lot more than the most expensive code review you could ever have nightmares about. So probably just every second or third code review (or maybe even less...) has to catch just ONE simple bug for all code reviews to be cost effective. In other words, if two out of three code reviews are totally clean and the third finds just one bug, you're AHEAD in cost. And we all know code reviews catch a helluva lot more than just one simple bug each.
Also, code reviews prevent programmers from deliberately writing obfuscated code. FWIW, if you work for me and I catch you deliberately obfuscating code, you're on your way out. Period. It may take me a while, but you're gonna be GONE before long.
Yes- but would the time have been better spent on something else than an absolutely corner-case bug. That's the question. Not whether they can catch bugs... They can! But can you catch more with those same resources spent on UI testing, or writing automated tests. Or is the business better off accepting that little bug and spending the time developing new work. Just because a bug was found doesn't mean the exercise was valuable.
What we've found is that code reviews take forever ...
In one place where I worked in the past, we had a very simple rule: if you are doing a code review, and it takes longer than 10 minutes, then you stop it right there and return the whole thing marked as "overcomplicated" - if it really takes that long, then either the code is written in non-obvious ways and/or poorly commented (which will result in poor maintainability anyway), or the change is too big for one source control commit. By and large, it worked, even if you have to make exceptions occasionally (but at that point you know it's not a typical review, and pay more attention).
In addition to that, you might want to consider better tooling. If you're doing reviews by sending .diff files over email, you're doing it wrong - there are many specialized tools out there that will do automatic and smart diffing (including between rounds in a multi-round CR), notify people responsible for affected files, allow to set up the workflow according to your needs, enable attaching review comments and conversations to particular files and lines of code, and so on. The shop I was working for used Code Collaborator , and I found it to be pretty good, but there are plenty other similar tools out there, and you might even be able to find some good free ones.
You may not "see" it in the sense of fixing UI bugs, but in the long run it saves you TONS of time. It ensures people code to an internal coding style document (you have one, right??) which makes code more readable and maintainable in the long run. It also, of course, can catch subtle bugs that may be compounded in time with new features. Together these saves tons of money and time in the long run. Don't be short-sighted.
When I was working as a trainee, I was invited to the code review of a mediocre programmer. A bunch of people were there including a senior A/P who was my boss. My boss started ripping into it and I quickly realized the mediocre programmer - who couldn't code his way out of a wet paper bag - had taken one of my programs and tried changing it (including my name). It didn't work and the senior A/P was asking him 'WTF does this do?'
Oh the dilemma. Do I take ownership and defend "my" code, or say nothing and enjoy watching him squirm? I did the latter watching him go "ummmmm ummmmm ummmmm". Finally medicore programmer admitted he copied it and it was really mine. I said "Well it was working when you copied it off me."
Needless to say he ended up being promoted to manager. Not even a PHB which requires a bit of cunning, he was just a flat out idiot. I keep in touch with some of my coworkers to this day and whenever we want to describe a certain type of idiot we use his name. Duuuuuuuuuuuuuuh.
Our shareholders do our code reviews.
Steve B.
In my experience, code reviews are generally useless. Main problem is that very few software projects have decent documentation which can be used as base for code review. Without documentation, reviews generally end up being a R&D-wide (or worse) squabble.
As development manager, one should try to make the reviews an integral part of development process. Keyword is "integral." Results of review are hard to quantify, thus something should be done to make them useful to the other phases of development process. IOW, results of reviews shouldn't be something on their own, they should be used somewhere too.
One major use for code reviews is when their results are fed back into testing effort. There are companies which test software based solely on high level list of features. I witnessed at least once quite huge mess, caused by bogus design done by R&D: essentially trivial global option was switching code in several programs to alternative code paths. R&D on tight schedule to implement the option simply copy-pasted with minor modifications piles of code. Test department obviously had no idea about that and the option wasn't even properly documented. Code review might have discovered that sooner (e.g. before we actually had several customers attempting to deploy the feature), pushing R&D to do proper design w/o code duplication (unrealistic for commercially developed software) or at least feeding the finding to test department so that they could have tested the whole system twice (option is on, options is off).
Our code is intended for desktop, non-critical use, so I asked my boss to consider whether it was worth spending so much time on examining built code, given our experience not getting much out of it.
No results from core review is a positive result.
But there is a gotcha: if all your developers have similar/save background, then efficiency of code review would be extremely low as such developers tend to do same mistakes. For effective code review, you need people who view the software differently: e.g. developers of one component review another component. Best of all if they would also have different education and background.
But obviously developers themselves should be willing to participate in code reviews. Code review is quite exhausting (e.g. I can review only about 3.5K LOC per day - more my brains do not manage) and poorly motivated developers would do very sloppy job at that. If you can show developers with some graphs from issue tracking system that code review improves quality, then they might be more interested in the process. Yet, still, do not expect any miracles.
Honestly though code review was always my dream. As developer I do quite a number of trivial coding mistakes. When possible I adjust my coding style so that compilers can detect such errors, but commercial compilers I have to use are quite moronic (compared to GCC) when it comes to warnings. And not on one project I had seen people disabling compiler warnings altogether because ... because ... like hell I know what goes in heads of generic developers on pay roll.
All hope abandon ye who enter here.
This has been studied extensively. Peer review of code is much better at finding bugs than QA testing.
If you release code without QA testing and the process works for you, fine, you can consider releasing code without peer review.
If you wouldn't consider releasing code without testing, but currently release it without peer review, you are batshit insane. Sucks to be you. Sucks even more to be your support department.
In my shop, there is no formal code review process. But informally, we all have to maintain this stuff, and there aren't enough of us that everyone gets one nice, clearly demarcated area of responsibility. So in that principle, there is a strong informal process.
In the course of designing a piece of software, or subsystem there is a lot of "Okay, here is my approach. See any pitfalls?" During implementation, there is a lot of over the shoulder "could you take this over?". And once a piece of code is ready to be merged into trunk, a fresh pair of eyes are pulled over, and the code is looked over.
It's a lightweight process, code gets checked numerous times, and we don't run into cases where you suddenly have to review several thousands of lines of code. This has caught numerous bugs, time and time again, and catches the kinds of bugs that user level testing can be the worst at finding - corner cases and intermittent issues.
Code reviews are worth it, you just need to figure out how to do them so they aren't a pain and are useful. Keep in mind that pair programming is continuous code review, and is a good thing if at least one of the pair is a senior developer. The formal, get in a room, and review all the code since X is a waste of time. My team is fairly small, I have two scrum teams of 5 developers each. For code review, we have version control set up to mail all commit summaries to everyone on the team. One of the duties of the scrum master is to review all commit summaries from both teams, and bring up any problems with the team member. This is generally just a forward of the commit summary to the appropriate developer with comments in the email, and cc to the team. I found this works extremely well and does not interrupt the work flow at all.
It depends entirely on what your purpose for them is. You should have a good test suite that passes, good coverage, and have done some over-the-shoulder checks. Then the formal code review can be less about "Let's find bugs" as it is a style check and a chance for everyone else (who might get called upon to support the system later) to understand the general purpose, operation, and any clever bits. If your code review is supposed to be the bug hunt, then yeah, that's not efficient.
Long? What do you mean the signature at the bottom of every comment I post on Slashdot is too lo
Code reviews
if an expert:
if humble: definitely worth it.
if arrogant: a pain, but probably worth it
else if your boss (but not an expert)
if nice: alright.
if a jerk: a nightmare (lobby against reviews)
else
not worth it (lobby against reviews)
end
There's two aspects to make it work:
- Tools. You should put some effort into your tools. The features to aspire to are:
- tight VCS integration
- email integration
- visual diff viewer
- inline comment annotations
- web interface is preferable
- Policy.
- Every change should be individually reviewed, before it is committed (reviewer(s) give the final OK).
- Smaller changes should be encouraged. The smaller the better, but more than 1000 lines is too big. Reviewers should request large changes to be broken up if necessary.
- A single reviewer per change is adequate, on average. 2-3 is also common.
- Reviews should also be cc'd to team and other stakeholders; unsolicited reviews/comments should be encouraged.
- A consistent code style should be enforced in reviews.
The attitude of the engineers is important, but difficult to enact. It's particularly hard when you're bootstrapping a new policy like this into your engineering culture. Although the reviewer has the final say, in reality they don't have any real power over their colleagues' actions. So they should focus on constructive, concrete issues and back off once a reasonable debate has occurred. Design reviews are important because if design issues come up in code reviews it can get ugly.
Personally I am a unit test after coding person. This tends to result in a code review by default. Bad code would have trouble getting past this point in the process.
Damn europeans and their metric system.
How many shitloads are in a fucktonne?
If you actually got into an argument with your boss, then I'm guessing you went about things all wrong in trying to make your point. Some tips of advice for advocating a change of procedure:
1) Don't make this case in front of other people. This has to be behind closed doors. If you bring up something in front of others, it simply looks like you're trying to make the boss look like a bad guy to everyone else, and he/she will get defensive.
2) The easiest job in the world is to be a critic. If you come up with a well reasoned plan, with statistics, etc. that prove your point and propose a new methodology, you're well on your way to getting your way. Half the time subordinates pose a problem to me, they're simply dumping more work on my lap. I try to be reasonable, but it's a heck of a lot easier to approve something if there's a reasonable proposal to work with. Especially if the proposal looks to have a low risk and substantial gain. Not everyone wants to stake their reputation on certain projects.
3) The problem may/may not lie with your boss. He/she may have had this exact discussion with his/her boss and already been shot down.
4) You have to be genuinely motivated to improve your work product. The motivation cannot be "code reviews are a waste of time." If you pose something that way, you're essentially calling your boss an idiot for asking you to do them. Also, your boss is immediately going to be suspicious you're simply trying to reduce your responsibilities.
5) If that fails, you may as well embrace the reviews and dedicate your time to improving the way they are conducted. Spend some time researching what works and what doesn't. Then go to your boss and ask permission to implement some of the best practices. Managers get irritated when people expect them to solve all their problems. Do your best to institutionalize good practices and demonstrate that you're doing things for the good of the company, rather than to free up some time in your day. Do it in the name of "continuous improvement," and you'll be a hero.
6) You usually get two shots to make a pitch for a change. After that, you need to do your best to implement things the way your boss wants them. Anything more, and you're simply challenging his/her authority.
What most developers/tech staff don't realize is that code reviews rarely result in a significantly lower bug rate. Proper unit, integration, and user acceptance testing is your best defense against bugs. What code reviews do provide is increased code quality with regards to style and supportability as well as some cross training that can benefit support efforts. It can also benefit performance and other NFR areas as expert developer help novice developer find less efficient code but if your testing properly tests your NFRs this is less likely to be of any financial benefit other than the growth of your developer staff that might help in a future effort.
If you have problems with operating / support costs due to poor quality code and code of drastically different styles from different developers then code reviews can help but only if you write good code standards first so everyone is writing to the same standards. This becomes more valuable as the size of the organization grows. If you have 10 developers the benefit is probably minor compared to if you have 100 developers.
In general I'm pro design review in ALL situations and I'm pro code review only in instances where I have operating / support issues.
The purpose of the few coding reviews I have assisted was only to improve code metrics in order to avoid: ...)
- too much nesting
- too long procedure or files
- languages syntaxes that are rarely the best one (operator ?, struct in C++,
The modified code has far better metrics, but has exactly the same number of bugs and is less readable because of the late cueless refactoring.
I hate the article of Dijkstra about the Goto, because of these practices that degenerated from it.
Computer programing should be an art form like poetry and music (D. Knuth) and shall not be restricted to a subset. It is as stupid as refusing alterations in music or avoiding rare words in books.
Here's a little exersize you might want your boss to be involved in:
I suspect you already have a good idea what the outcome will be. That should be enough to tell you how effective code reviews are.
Automated code formatters/code inspectors, along with decent compilers/linkers (or interpreters) will surface most of the issues that code reviews find.
Instead of pissing away valuable developer time, put those reviewers to work writing and executing tests. Right away, you'll discover whether the code is testable. And then you'll discover whether its actually correct.
Tests don't have egos, agendas, personal axes to grind, or coworkers they don't want to piss off. They don't take vacations or sick days. They don't have opinions about the author of the code. They usually don't leave the company. They generally don't have an opinion about how many/few comments there are, or if the code has been formatted to corporate spec (unless those tests are executed as part of the automated tools mentioned above). Sure they can be drudgery to write, but its the only real way to know if the code actually does whats its supposed to.
007: "Who are you?"
Pussy: "My name is Pussy Galore."
007: "I must be dreaming..."
It's not entirely about what they reveal once they've been established as good practice - there is also the fact that knowing that they will be done leads people to treat their code more carefully to begin with. In the deepest sense you'll never be able to compare what your code would be like if you did not have them - this is definitely not equivalent to what they turn up.
For every problem, there is at least one solution that is simple, neat, and wrong.
The sooner people realize that specialized Programmers are a thing of the past and that System Designers with DBA and Programming understanding will be the future the better. Microsoft already has an application creator based on your DB and the full ability to customize your template for the app creator so you can fix bugs in the root and deploy that template out for any web application you have. This is the future and debugging and documenting the logic and syntax of the code is a waste of time. Document out the flows and build the DB first and then the program 2nd and rely on the database instead of the actual program will save companies millions in the long run. Local Windows applications and Web Applications are going to merge soon into 1 platform that is universal and the sooner you prepare for this the better. Check your design and database that follows that design, you will get a whole heck of a lot more than you would from checking the code.
Are code reviews useful?
Lets see. Right click -> View source this web page. In the first 10 lines I see a variable called maybe with no comments as to what it means.
Yes, code reviews are useful.
Best code reviews I have seen cost a lot of money in man hours and are usually are done by an in house group that specializes in it. For the last firmware review (2048k ASM) we had 3 salaried guys making over 120k a year spend 3 months on it to find 4 critical bugs and dozens of lesser ones. Shipping that firmware would of made us liable for over 2 million dollars worth of hardware. So if you look at it another way ~1/20th of the cost of the product was invested in the final code review.
Having the same group review their own code is like the fox guarding the hen house, he might be a vegetarian but I doubt it.
An Education is the Font of All Liberty
I've actually been going through the process of getting code reviews as a standard process on my own team. We've done them now and again in the past - often on other team's code that was being integrated into our platform - and it was typically a pain.
Enter A Good Tool(TM). We've been demoing some code review software lately and after settling on a particular tool that we find to work well with our workflow, the team has unanimously agreed that they find reviews beneficial. We don't have strict policies on how/when reviews are done, so it's encouraging when you see people are creating new reviews for their code of their own volition.
While we haven't found a lot of critical bugs, we have found lots of minor things, problems/shortcomings in unit tests, documentation problems (especially important because we provide libraries to other teams, we're not the sole users of the code), and even pre-existing bugs while doing maintenance. I think the biggest benefits, though, have been getting more eyes on the code to increase familiarity with it so it's easier for other people to do maintenance and bug fixes on when the original author is unable to as well as just generally opening up broader communication about various elements of style, consistency, improving code readability, etc.
The software says we've logged about 16 hours in the past month, across 7 developers. That's a pretty minimal investment. There was mention of good functional testing being all you really need, but if you're working on libraries and such it's easy to have bugs that don't show themselves in all usage scenarios. If well after a release another team manages to find a previously unnoticed bug in a library, the cost for them to track it down to our code, for us to fix it, put through QA, do a new release, pass off to the other team who then has to put their component through QA and deploy.... we've just burnt through a lot of time and money.
Will code reviews lead to perfect code? No. But I would undoubtedly say that there are plenty of benefits that make them well worth it if they're done in an effective fashion.
By the way, the software we settled on is Smart Bear's Code Collaborator, having also tried Crucible and Review Board as well as talking to other divisions about their experiences with code review software. It may not be the right tool for you, but we found it lets us bust through both initial reviews of the code as well as follow-up reviews to ensure any issues are being resolved appropriately. It's not the cheapest, but if it's the difference between a tool people will willingly use or a tool/process that people will bemoan, it's worth it.
As you say, they have no value in your environment. OK, maybe your team can't do reviews, maybe there is no point as the code is just a bunch of UI junk that changes constantly. It's no big deal. Not every team uses every software development technique, nor should they. I happen to write lots of code as a team of one. I'd like to get others to look at the code but that isn't going to happen, so no code reviews. I still get paid, applications work, customers are happy...it can be done.
Cardboard Cutout Dog
Possibly the finest essay on the subject.
Jokes aside, it basically posits that the value of the code review is not in the listening, but in not letting your ideas stay in your head as a ball of fuzz, and force you to make them concrete by talking to someone/something. Tends to be true about code, and many other things.
In my organization we separate "code reviews" and "desk checks".
Code reviews are formal meetings as described in several of the previous threads where you may call in a host of designers who sit down in a conference room, trying to dissect the code. These reviews are held seldom and normally only for major code changes which often affects other subsystems, to find ensure that the changes will not cause unexpected behavior of the system. This is very expensive.
Desk checks is when you call a team member over, show the code, and describe the changes you've made. This is obviously very cheap.
We have no hard statistics on the number of faults found in desk checks, but the general opinion is that they are very good to perform because:
1. everybody can find time to perform them
2. the coder receive a confirmation from a second mind/pair of eyes that there are no obvious faults
3. even though faults are not often found, the return of investment is high due to the low cost.
We plan to enforce desk checks on ALL changes. Code reviews will be reserved for the special cases.
However, desk checks and code reviews will never ever be able to replace a solid regression test. Desk checks are a compliment.
For some statistical evidence (built on a very small sample size) that code review is worth it, see this section of a chapter from O'Reilly Media's book Beautiful Teams:
http://www.red-bean.com/kfogel/beautiful-teams/bt-chapter-21.html#gumption-sink
That section is not about code review per se (it's about how a seemingly trivial interface decision affected code review), but it includes some code review stats from two projects, and discusses how frequently one project's code review catches mistake from previous changes.
(Disclaimer: I wrote the chapter, but it seems pertinent enough to this discussion to be worth posting.)
--Karl Fogel
http://www.red-bean.com/kfogel
It's pretty obvious Electronic Arts did away with code reviews many years ago.
#DeleteChrome
Lots of good replies here, so I won't touch on whether or not code reviews are beneficial. But one thing jumps out at me from the question:
"I'm a development manager... I asked my boss to consider whether it was worth spending so much time on examining built code..."
I hate to say it, but you are the wrong people to be considering this question. Code reviews are for programmers. It's good to hear that your reviews are developer led, but you've got some serious problems if you are trying to make this decision on your own. In fact, I'd go so far as to say that you have serious problems if you are attempting to make this decision at all.
Certainly, a development manager's role will involve some coaching (unless you hire a senior developer whose job is specifically geared to coaching the developers -- something I highly recommend). In that coaching role, you could of course guide inexperienced developers towards practices that work well for the team. But at some point practices should be self-selected. The developers can see much better than you can what is worth doing and what isn't.
It's appropriate to gather metrics and ask question about what is effective and what isn't. It can be a launching point for good discussion. But if your opinion of your developers is so low that you think you should make this decision, then you've got problems. Either you need to replace your developers or (more likely) get an attitude adjustment.
If it's ever not more economic to do code reviews, then I respectfully submit that You're Doing Them Wrong(TM).
The improvements in the general standard of code, and consequently its maintainability, should easily outweigh the modest time spent on reviews. Likewise, the efficiency benefits just from sharing basic awareness of how various systems work and useful coding techniques around a development group should be enough to justify the time spent. And those are both without allowing for any actual bugs that would have been observed by your customers but got fixed much earlier and cheaper because the review caught them.
Incidentally, if you don't think it's worth getting even a quick glance from a second pair of eyes on even a small bug fix, you should look up the research on how many bugs originate from a one- or two-line change to the code. It's a staggeringly high proportion.
Now, a lot of places have tried full, Fagan-style, heavyweight reviews, and yeah, those pretty much suck for most software development groups. But that doesn't mean you can't employ a lighter process with the same goals. With the kinds of tools available to co-ordinate reviews and annotate code these days, your overheads should be near zero and you can do a lot of the work on-line rather than shoving everyone into a room for a few relatively unproductive hours.
If you disagree, post your argument. (-1, Overrated) isn't your personal censorship tool for views you don't like.
If you did test-driven development then code reviews would be redundant.
No, not for everything.
I'm currently fitting a wiimote-shaped peg into an XTest-shaped hole. I find that I can point the nunchuk upwards by a larger angle than I can point it downwards. The code that reads nunchuk data should compensate for this fact somehow.
I'm not really sure how you write a test method for "Wiimote should be pleasant for human hands to hold and operate". Nor am I convinced that I would have found out my discovery without exploratory coding, something which TDD forbids or at least discourages and frowns upon ("write tests first").
At least, if I get what TDD is all about.
As a programmer, having someone else look at my code is good since there will always be mistakes I just cant see from looking at it. Someone else looking at it might pick them up when I cant, especially if they have more knowledge of bits of the codebase than I do.
Yes, code reviews are nominally about finding problems with the code, but they can also be used to...
* Share knowledge. You can learn a lot of new things by looking at someone else's code. Perhaps they use a library class you aren't familiar with, or have used a language feature in an elegant way. Just as an artist will look at other artist's work as part of their growth, programmers shouldn't be working in complete isolation. And even if you are the amazing guru that knows everything, then at least your teammates will be able to benefit from your vast knowledge.
* Ensure that every line of code has been seen by at least two people. Code that has only been worked on and looked at by a single person is a liability. If they leave the company (or project or whatever) you suddenly have this huge morass of code that nobody is familiar with, and you probably only start to realize this when there's a nasty bug that needs to be quickly fixed in that code.
* Tests can help with correctness, but do not necessarily make the code maintainable. Even if your code is correct, if a question comes up in code review, then that's an indication that you need to write clearer code and/or better comments if you expect anyone else to be able to work on the code in your absence.
That being said, a bad code review is probably not worth it. Keep the amount of code being reviewed at a single time relatively small. Don't involve too many people - just one other person looking at the code will provide a lot of value at minimal cost. If you start inviting managers and "process czars", then you're on the wrong track. Review code promptly: one-on-one reviewer led discussion works well (they read your code and ask you questions as they go). Code review tools can help, especially if the programmers are in different time zones. Everyone needs to make reviews a priority - aim for 24 hour max turn-around on a review otherwise you'll find everyone always being blocked. I think letting the author pick the reviewer(s) is important. That way if someone is a complete jerk about code reviews, nobody will ever include them in the reviews and you have minimized the damage that person can cause.
I am an experienced principal software engineer and team lead in a software house. Code reviews are essential part of our development process. Having said that we avoid lengthy reviews. We don't have review meetings, forms, sign off procedures, the stuff that put people off. Code reviews are a conducted entirely online via Jira forms, using a simple set of workflow rules. It is a non-intrusive, friendly process. I am considered to be one of the best coders, but I am not flawless. I find it hugely valuable for someone (at least as experienced as myself) to review my code. Even if I have the chance to slip it, I won't. It is for my own benefit. Most ideas are subjective and they pop up in human consciousness. I am not talking about detecting simple errors, but issues more of structural, the ways you structure code; i.e. efficiency vs maintainability, or generality vs usability. I don't believe there is a machine process out there that can successfully replace a human reviewer to find the optimum between these extremes, at least not for another 100 years. To sum up, "don't give up code reviews, but simplify them, make them developer friendly".
Which goes against the thinking for a lot of developers. They seem to take reviews of code personally, and believe everything they did is correct.
I go the other way. If my code is good, it will stand the test of a review. If one or a group of my colleagues looks at my code and doesn't find a fault then I KNOW it's good. I don't have to just THINK it because I believe so. If I can't explain why I did something in a review, it shouldn't get into production code.
Sometimes it's even simple stuff. I Do X, and someone goes "oh, we had to do it too, and wrote this bunch of code for it. Maybe we could combine the code into one usable module for both". It's stuff like that you can only really do in a good code review. It shouldn't JUST be done at a commit. It's something that should be part of the development process.
At my last job we called this "talking to the bear".
We found that occasionally slipping a live programmer in in place of the stuffed bear improved the quality of the process.
You won't catch many bugs code-reading, but you will keep up the quality and maintainability of your code.
My company has a pretty rigid set of processes and we find that we catch approximately 25% of our defects in code reviews. This doesn't count minor things like not following the coding conventions or bad comments, those are logged separately. We have reviews where we hand them off to just one other engineer as well as the Fagan style of reviews, depending on what we feel is appropriate for the particular piece of code. In our industry(embedded software), it isn't exactly easy to push out a patch once the code is released so that plays into how/why we do it the way we do. I wouldn't say the review process is cheap, but neither are warranty campaigns. Pay it now or pay it later I guess...
Even the best people occasionally typo, put in a bad/wrong comment or just flat out make a mistake during coding. Personally, I'd love it if we had the resources to review every single line of code I write.
At work we're currently in a heated debated whether we should adopt peer code reviews (what the boss wants) or paired programming (what us devs want).
We feel that paired programming provides all the benefits (and them some) of peer code reviews but doesn't have all the cons of peer reviews. Here's a quick run down of what we came up with.
Pros for paired programming:
* A natural knowledge backup is created (2 devs know how the new code/design works)
* More sound design decisions can be made during design/development stages (two heads are better than one).
* Complex problem solving.
* Fewer bugs. Better quality product.
* Less total overall time spent coding/maintaining.
* No new software to purchase/install/learn (I.e., peer code review software - Review Board, Crucible, etc.).
Cons for code review:
* If the design decisions are poor and need to be reworked, we don't find out till the code review.
* Code review step likely to get skipped with tight deadlines.
* Chance for egos to get bruised.
I'd appreciate anyone else's comments on this comparison.
Thanks!
SD
One company I worked for did regular code reviews. Unfortunately. It quickly became an annoying waste of time that devolved into punctuation checking. The only thing that mattered was how many spaces were used to indent lines of code, how many blank lines were between subroutines, whether dashes or equal signs were placed before and after comment blocks, and other minutiae of meaningless, non-functional detail.
Unfortunately, and I'm sure this will come as a surprise to everyone, this did little to improve the quality of the software, or reduce the number of bugs.
But, hey, at least QA had job security, you know? I guess that's something.
"My country, right or wrong; if right, to be kept right; and if wrong, to be set right." --Senator Carl Schurz (1872)
how many programmer are working with you. Can you exclude some of them are assholes? Can you exclude some of them are hating their job, want to leave tomorrow and are waiting to sabotage something? Can you exclude some newly hired guy is inexperienced or an idiot?
Well if nothing of that matters to you, you don't need anybody having looking at their code. However if something bad (sbdy of your shop stealing data etc....) happens and you will be asked if you had a well defined procedure how code enters you codebase, it might save your ass if you had code reviews.
Honestly, code reviews, historically, have proven to be about ten times as cost effective as other techniques. If you're not seeing good effects from them, the odds are that you aren't doing them right.
Not that this is a big surprise, as few people do them well.
Get hold of some of the literature on Fagin reviews, and make sure you're following the guidelines:
(1) not too many people in the review: 3 minimum, 7 maximum.
(2) the programmer isn't allowed to speak, except potentially to explain something
(3) no one can suggest solutions: identify a potential problem and move on
(4) close the loop: make sure all solutions get documented and passed to review participants.
(5) a single review can last no longer than 1 hour.
Get a software package or share "diffs" via email. You should really not accept code until at least 1 or 2 other people have taking a quick look through the changes.
Code reviews are important not only to filter out mistakes, but to also make sure multiple people are familiarized with the code.
But managers are really bad at handling special cases. Sometimes it's more important to commit code than to do a code review (at my work we can't svn commit until we put in Review Board entry that has at least two "ship its"), flexibility in process is also very important. Most companies don't "get" that, they just look for a silver bullet they can apply broadly.
“Common sense is not so common.” — Voltaire
The only valid measurement of code quality
http://www.osnews.com/story/19266/WTFs_m
software is bugs. a code review may find a few, but it will miss many others, because most bugs are a missed/misunderstood/unknown requirement.
and as for them being "a way to enforce various stylistic guidelines on code", that's simply a nice way of saying, "they provide a forum for pedantic programmers to pummel one another about stylistic ridiculousness.
I have always heard great things about code reviews. However, our company recently began a policy of formal code reviews and they have not lived up to my expectations. We rarely find any real bugs or serious design flaws. We find things like minor coding standard violations, or "you should move this statement here to make things clearer". While these findings may help improve the code, I don't think that they justify the time spent in review. And these reviews are turning out to be very time consuming, not to mention one of the most boring things I have ever done at work. This group of developers is one of the most talented and experienced groups that I have worked with. One would think that if code reviews would work for anyone, they would work for us.
Incidentally, we do keep statistics on defects. Unfortunately, the minor improvements we find make it seem like we are finding a reasonable number of defects.
It's how much bad code never gets written in the first place, because the programmer knows his/her code is going to be reviewed.
When you know your code will be reviewed, you resist the temptation to take the quick-and-dirty shortcut that you know will get picked up in a review, you do it right the first time.
Quidquid Latine dictum sit, altum videtur (anything said in Latin sounds important)
" First, they are a way to enforce various stylistic guidelines on code that make future maintenance much easier."
Yes, I've heard that for many years, but have yet to see any proof that it's true. Perhaps stylistic guidelines are really used because of our compulsion to create order where none is required.
Besides, checking those guidelines could be performed by any competent admin prior to a review.
Especially if the reviewer can force changes in other people's code as the result of it. I mean I had one where I had some logic that looked absolutely insane. This happened because I was using functions from another programmer and he had nonsensical return values which pretty much forced my hand. (I pointed this out to him but he refused to change it, claiming it was intentional.) The code reviewer asked why is it that way, I told him and he pretty much pulled rank and told the idiot to fix his code. (Unfortunately the idiot then had it in for me for the rest of the project and would go and whine and cry about every last little thing which didn't make things very good.)
Did you know 80 to 90% of the moderators on slashdot wouldn't recognize a troll even if one dragged them under a bridge.
Code reviews are somewhat useful for small changes. But I find that for large changes, they are TOO LATE. If someone has gone forward and made non significant changes to your software, and you happen to catch his misconceptions during the code review (dependencies he shouldn't have pulled in, improper encapsulation, volatile and frequent usage of memory, etc), he won't be able to just "address" your concerns with a few changes. He basically has to scratch most of his work. And unfortunately, few managers will see the benefit of "implementing something" better since that requires more time, and the feature as implemented "works".
Likewise, when the software design is done too long before the actual task gets to the implementation phase, the design may be out of touch by the time you get there because new constraints and problems have been discovered since the design phase.
That's why I like to have quick whiteboard sessions before someone starts implementing something. I like to know the dependencies their module will pull in, the services it will require, I want to make sure the module is kept as ignorant as possible in terms of what it knows about the rest of the system, etc. When those things are caught early on, is it almost free to correct them. But later, there is a cost. If the task is long, I like to see multiple snapshots along the way to again course correct anything before it gets too far down the road.
I work at a large video game company. I don't believe things always were the way they are now, but right now everything is tasked against "features" and it is hard for most managers to understand the necessity of infrastructure work, of unit tests, of proper module encapuslation and definition of dependencies (otherwise you find your unit tests impossible to write using your module's public interface). Every sees the short term. It is easy for them to understand that something is done when they see it in the game. But it is harder for them to understand that you want to make the process of software development more predictable, you don't want half the features in the game to be broken half the time and the shipping product to just be an instant snapshot of working features that will break as soon as the next cycle starts.
So, code reviews are useful, but they really aren't the most useful thing since they occur TOO LATE.
We have recently started doing code reviews using an online tools that ties very nicely into our SCM tool. All code changes have an associated defect or enhancement ID, are developed on a private branch, and prior to being integrated into the trunk have to be review by a member of a pool of subject matter experts. Since the team is world wide, having a web system to communicate between the various sites allows for faster response time, and helps ensure consistency in standards throughout the regions.
The product is by a company named Smartbear, called CodeCollaborator. Very pleased so far, http://smartbear.com/.
As with any software process, there are right and wrong things to do. First, the formal code review where invitees don't review the code in advance are pointless. Usually there is so much to go over, even if these are scheduled frequently, that anything more than surface is missed.
And then when you have a developer do a package and walk through, there are people not to invite. I've been in reviews where all but one of the reviewees took it seriously, and the one that did not, effectively wanted to review everything in the meeting because they didn't study the prep. Only invite reviewers that take it seriously.
A lot of unorganized code reviews degenerate into style comments and surface stuff. This doesn't help; the most important thing is to have reviewers that bring in their own findings like an audit. You can delegate things from developer to reviewer, like one reviewer could run through a profiler (ie java findbugs), another through metrics (ie java metricsreloaded), or another with specific data structures and algorithms expertise or concurrency attention. So... each reviewer can have a role. Each reviewer is a subject matter expert.
The best ones in my opinion are where the developer runs the code through a metrics tool to identify hotspots, like complex code, or things that have complex entry/exit conditions. Things where the developer actively wants to solicit opinions. Like... "How the heck can I test this?"
Again it falls to the people though. Artifacts are really important, as are action items and follow up. Another example; one review I was in we spotted a developer using floats for guids. We explained why this was a really bad idea, but because we didn't follow up on it, and neither did the developer, I spent a week hunting down a duplicate spurrious guid conflict. The team was pretty upset with the developer for not actioning a fix, but our management believed that buy in to process was optional...
Good luck. Reviews show professional acumen, but there needs to be some discipline in the prep, review, and follow up.
/\/\icro/\/\uncher
XP advocates to pair program 100% of the time. I would suggest that you guys try to work pairing into your development schedule. Most people are afraid because they feel as though it will slow down their velocity but it should actually accelerate it. You'd be done with the lengthy code review process, which is silly in my opinion. It's almost always better to have 2 eyes on code than 1. It keeps the developers engaged if they ping pong back and fourth (taking turns to code/test). It reduces the number of bugs. It increases the readability of the code (now both have to agree on how readable it is). It also helps spread the knowledge amongst more than just a single person (what if one got hit by a bus the following day?).
I keep hearing about these programmers getting hit by busses. In fact, I think I hear about programmers being hit by busses more than members of any other profession. Now, the busses that I've seen tend to be rather large things that stop frequently, and usually aren't going very fast. The generally poor quality of code suddenly makes a lot more sense when you consider that it is being written by a class of people who are so fatally inattentive as to be struck by busses with such improbable frequency.
Remember RFC 873!
Many programmers will improve their neatness and attention to coding standards if they know the code review is in the future.
.. Blub falls right in the middle of the abstractness continuum. -- Paul Graham
1) You want the meeting to be the first time you've looked at the code. That way you can ask a lot of questions that could have been answered in two minutes of reading the code (and thus waste minutes of everyone's time).
2) Don't assign roles. That way everyone can look at the code from the same viewpoint.
3) Don't stay professional or objective. Ad hominem attacks always help in reviews.
4) Don't set a time limit. Everyone loves meetings that drag on and on.
5) Be sure to review at least 5,000 lines of code in each review.
code reviews take forever and tend to reveal less than good UI-level testing would
WOW. Just WOW. You must be doing a numer of things incorrectly. Let me offer some suggestions: (Although, UI testing is definitely a pre-requisite here, and nothing will find more bugs than just using the software.)
1) Everyone must read and understand the code before the review.
2) Allocate time in the schedule for code reviews.
3) 4) No bosses, managers, testers, or other non-coding positions in the review.
5) Do not read the code line-by-line or even function-by-function.
Everyone should have the code marked-up with comments and questions prior to the meeting. The goal of the meeting is to go through those, not to go through the code itself. The meeting should start with: "Author: Has everyone read the code?" If there are any "No's" then dismiss the meeting and those people can read the code during the time allocated for the meeting. "Author: Are there any major design, or module-level questions or comments?" This is the time to address major items like using the wrong tools, libraries, etc. (Although in theory, those were caught during a design phase, but it can happen) Now, go through the comments and questions. Or go function-by-function if you can. Maybe with "This function does X: Any comments or questions? Next. This function does Y..."
At this rate, you should be able to review weeks of code in an hour.
Unless you have a bunch of 2-bit hacks fresh out of tech school/Uni. Otherwise good programmers can review their own code safely and effectively. Using automated styling-enforcement in Eclipse will keep everything clean and 99% of all algorithms you need are just library classes which have already been reviewed and optimized.
Now if you're a code-shop using .net and $5/hr Indian programmers...
I remember when I first started my current job, in a web developer position. We were using "templates" for developing custom products. The templates had about 90% of what was needed at any given time, but when I was first given a task that was essentially a new feature, I asked naively, "How do we handle this? I see that we're using templates, but this is new. I'd like to code this so that everyone could use it."
That was met with bafflement. As it turns out, every single developer-- when faced with the same situation-- had simply added to his/her own stash of code that was never shared. Therefore, a lot of time was (and probably still is, I'm not a developer anymore) wasted re-coding stuff that already existed. That is because the notion of delving into code, with the intent to share knowledge in a meeting, was a foreign concept.
Somewhere in between "zero" code reviews like the situation that I was in, and spending zillions of hours on tedious code reviews which may or may not be useful, there exists the right balance. Like anything other situation, you should avoid useless meetings. But if you have a particular project that would benefit from it in some way-- either from a re-usability or a time-saving or a code maintenance perspective-- I think it's worth it.
As always, the obstacle is finding someone in management who understands these concepts. I haven't figured out how to effectively explain the "obvious" to PHBs. You would do well to consider communication as a key necessity when wrestling with your question...
Curry is easier to clean. I wonder what you do when you find Hamburgers between 1's and 0's.
You probably eat it.
Some code reviewing is validated but much of the tiresome review work can be resolved by automatic tools these days. Tools like Lint and FindBugs.
But a review to make sure that the code is easy to understand and maintain is a different issue that can't be addressed by automatic tools. At least not until you get a tool that can semantically understand comments.
If builders built buildings the way programmers wrote programs, then the first woodpecker would destroy civilization.
I think this is actually the most obvious and non-controversial aspect of code reviews, and yet I don't see it mentioned anywhere above.
Shame. Pride. Vanity. Whatever. You know other people will look at it, so you make it look better. You take another look at it as if someone else might actually need to understand it, and you make it better before the code review.
That's an absolutely wonderful effect, even if the code review is a bunch of grunting and a rubber stamp.
Code review provides huge benefits. It may seem like a lot of work to find "just" one or two bugs an hour (and typically a good review will turn up at least that) but those bugs can be fixed without engaging a full QA cycle. The bugs turned up are often also exactly the kind that don't reliably turn up in test like memory leaks and race conditions. I've seen situations where just a identified issue provided savings that justified the entire cost of the program. Those are pretty serious issues but even a typo can produce considerable paperwork if it gets into a release (test>triage>dev>fix>retest).
The primary effect of code reviews has nothing to do with finding problems during the review itself. It improves quality before the code ever gets to review, because people care far more about what they do in the first place if they know there is even a chance others will see and criticize them later for doing it wrong.
This is why stores put up fake security cameras. The notion that they have someone sitting there watching the camera continuously is ridiculous, yet a camera has a huge effect on people's tendency to commit crimes nonetheless.
It entirely depends on which stage of development you are.
If you are early in the cycle, and just building PoCs to validate architecture and design, then code reviews will be great in establishing basic discipline.
If you are late into development and want a code review to find out issues in design, then sorry, you are too late.
Code reviews establish base standards BEFORE the battle.
Like battle readiness inspection of troops.
It tells how you will code under fire.
But once the battle starts, you don't conduct inspections, because they are a waste of money and hell they do nothing.
"Doing what i can, with what i have." ~ Burt Gummer
The biggest problem with today's code is yesterday's code.
If you want to be productive you must have good code to begin with.
Its very difficult to start doing things correctly when you're working with code that is a ball of mud.
I was about to say this. I've worked with some really good developers and I consider myself "good" as well. We would not do code reviews for _all_ new code. We would ask each other to review particularly hairy pieces, though, which would often result in insightful and non-obvious refactoring suggestions, and rarely in finding bugs. Security related bits would always get a full-team review (including QA). Code reviews were required for all but the most trivial bug fixes, since they're cheaper and you don't always remember what the old code did 100%. Closer to shipping, two reviewers were required. There were very few bugs overall.
I'm pretty confident that I can code faster and with higher quality than pair programmers of virtually any skill level. If they're bad, they'll inevitably churn out bad code (although it will be better than code written by just one bad programmer), and if they're good, pair programming will slow them down and lead to more defects because neither of the programmers will have the whole thing 100% in his head.
I work in a large programming shop. I've participated in both worthwhile and worthless code reviews.
The worthwhile ones require preparation. During that preparation you read the code, dividing issues into two categories: minor and major. Minor issues go direct to the programmer and aren't open for discussion during the meeting. So programming style issues don't make to the the table. The convenor collates the major issues, and these are addressed in order during the walkthrough meeting.
Of course, not all major issues are bugs. Sometimes issues are unclear code which must be tidied to make maintenance cheaper. Sometimes issues show a lack of understanding about the customer's industry -- for example, tying down a attribute of the problem in code which we know is open to change and thus should be configurable. Sometimes the issue is a misuse of a API, and this class of error can lead to a system-wide review (the classic error here being strcpy()).
The reason programmers don't like code review is because they show what the abilities and limitations of the programmer and the reviewers are. They all like to dream that they are coding heroes, and reviews bring them back to earth. But as the technical lead I prefer that to encountering problems post-shipment. Also, I don't want heroes on my team; I want people who are self-aware, who know what they are good and bad at, who working to make the bad better, and know when to ask for help rather than shipping deficient code.
The most important step to achieving good code reviews is to aim to do good code reviews. This means designing the review process so that it is effective. A three hour meeting which reads code which has not been viewed before, which criticises style over substance, which offends rather than teaches, and drags on missing the forest for the trees is a a failure.
The other really important purpose of code reviews is to examine that 1% of existing code which is causing 30% of the fault reports. Don't allocate that issue to a programmer to work on alone. Allocate it to a programmer, and review the old code as if that programmer wrote it. Then the full force of the team's intellect and experience will be bought to bear on that troublesome issue. The effect on the programmer is also good -- they don't feel as though they've been handed the troublesome child to work on alone and unassisted.
It's an old question, and there has been some academic research on the topic way back in 1980. Probably there has been predecessors and successors to this one. This study marks peer code review and dynamic simulation as the two most useful methods.
Take a look: Benefit Analysis of Some Software Reliability Methodologies by Robert L. Glass, http://portal.acm.org/citation.cfm?id=1010797
Until this is understood, additional discussion is futile.
If pair programming is cost effective, you've hired the wrong programmers - pay more and get good developers (and, as a manager, set priorities to reward quality). Teach developers to desk check their own code -- every true "bug" found by your "pair" or missed by them but found by QA or the Customer should be viewed as a humiliating embarrassment by the coder - something that happens every few 10K of code but really requires a humble round of beer for the entire reveiw/QA team.
Yes, part of Development is a creative process - but that's not an excuse for eliminating accountability or responsibility.
Fixing a bug, or fixing a design flaw? If your customers aren't going to ditch you for the occasional glitch, and it sounds like yours won't, it's much more important to focus your resources on making sure that they have a good overall experience.
There's no failure quite as dissatisfying as a complete and total solution to the wrong problem.
The recent story about HyperVM security holes teaches that code reviews can be a question of life and death. See the story on the register and maybe you will understand... Sad...
Thomas
... like a fun place to work.
Coming at this from a general control perspective, controls operate at both detection and prevention. The code reviews may not be detecting many problems, but their existence may have an impact anyway. If nobody is reviewing code at all, would people approach their task differently? What would anyone know about the software quality other than "it works"? The process may also be valuable to management for other reasons. What does anyone know about the programmer's level of skill? Is the code documented for maintenance?
Over dozens of projects I've seen 2/3 of code reviews fail to yield tangible benefit, but the other 1/3 of the code reviews were wildly successful in finding defects. Primary difference between the successful and failed reviews was homework. In the effective reviews multiple reviewers spent about an hour before meeting to review the code in respect to a checklist of items to watch for. Those checklists did not address formatting issues that can be corrected with a code beautifier. At the review meeting each person presented their found defects, which led others to find other defects in a synergistic effect. The checklist itself was subject to review. Because everyone, including the programmer whose code is under review are trained in the technique, there wasn't the usual defensiveness or the feeling everyone is ganging up on one person. The expectation is that defects will be found.
The reviews that failed, failed for lack of preparation, lack of formal standards expressed in a easily digestible checklist, and a lack of training so the sessions degraded into carping sessions about one person not liking another's variable names.
A multiple person code review is an expensive proposition. I'd use it only on critical code and on public facing documentation.
Code reviews work because when developers know their code is going to be scrutinized they improve their level of coding. I've run developer shops for over 30 years and I've found that no matter what level the developer, if they are aware that their peers will review their code that take more care over it. Will reviews make a poor developer a good developer? Of course not, but it will improve the quality, layout, documentation and general understanding that a developer has for the code he or she writes. And as a bonus poor developers can get insights from good developers when they sit in on reviews.
Code reviews are valuable. Try both automated and human reviews. Automated reviews are as simple as coming to the table with a profile of the code and quick ratios that make sense to your application. We run scripts against our source code control system to 'score' certain things... even a basic % changed is useful in prioritizing human review. Finding redundant code is another good check. Human reviews can be useful with the right folks. Subject matter experts (on the problem space, not technology), senior engineers/architects, and SA/DBAs are always good. Don't waste time - bring it in, review the high risk areas. More reviews on common libraries, less (or none) on single use situations.
I haven't seen anyone comment yet on how frequently the code under review will change. If you are writing a library or something that's going to be pretty stable, then the cost of a formal review is easier to justify.
But if the code is unstable, and going to be modified over and over, then the benefit of a formal review depreciates very rapidly. For some industries and products, this doesn't matter. But for others, the cost of the formal review can be too high.
Just do them right:
1. Each commit should have an explanation of what the change does, and should be small enough that the reviewer can do it quickly.
2. Your organization should prioritize code reviews over other work; in many cases the review is blocking something.
If your reviews are kept small, and are a high priority, they add enormous value and shouldn't negatively impact your work.
Code reviews have the following perhaps non-obvious benefits:
- They ensure the implementation does justice to the design
- They help pass institutional knowledge to the developer ("This function has an existing implementation over here...")
- They ensure code readability (especially when used with a formal style guide)
- They help keep the developer honest, when he or she might take shortcuts or be lazy with a certain function.
- By mandating code reviews, you have a pressure from the reviewers to keep each commit small, which encourages incremental development, which discovers design flaws early rather than after 10,000 lines are written.
Code reviews aren't really a great place to FIND bugs. Yes, obvious bugs will stand out to an experienced developer, but the reviewer is another human, and he or she can easily miss the same bugs the developer missed. Really, unit tests are where you catch bugs, and a reviewer is usually in a better position to identify incomplete unit testing.
When I did code reviews, I first had to understand the problem being solved. Then, I did a walk through for a) function descriptions being documented, either by being apparent, or with explanation. I would verify that all return codes were being verified, and that an error return code was posted to a log, as well as being displayed. The result of that practice was 1) programmer had one or two bug reports on his desk the first week, and half the amount in the second, and by the third week, the program was considered clean. Month end programs were expected to be clean after 3 or 4 uses. yes, they are worth it.
Leslie Satenstein Montreal Quebec Canada
H1B Visa Indians modded the parent post "Troll".
Ideally, the reviewer will understand the context of the code and the context of the review. Reviews should only be conducted by 2 engineers who both have a deep sense of what level is appropriate. I hate ending up with some ass who thinks every piece of code controls life and death, and happens to also think your variable naming is also going to kill people, too.
Code reviews are only as good as the reviewer. I've worked for companies that do the team over-head approach -- mostly a waste of time. Pair programming -- valuable for some programmers, frankly I don't mind having someone look over my shoulder, but it's boring as can be to watch someone else program, especially when I'm faster. Reviewing code of a failing project can be very valuable, recently I was asked to look at another team's project at the company where I work. I took on a few assignments to complete parts of the project, I learned that they had spent a year of development time and money to write a database access layer. I reported to my boss that they had wasted the companies money, I rewrote my portion using the Microsoft supplied DataSet and showed a simple operation taking 4 seconds in place of 90 seconds. Fortunately my boss took the corrective action and let the team go. The entire review took about four months. I know that is not what you are thinking of when you talk about code reviews, but, just looking for bugs in code is not enough. You have to have a competent senior programmer who knows how to evaluate code. Two bad programmers are not any better than one bad programmer. Throw out the silly coding standards that say tab stops must be 4 spaces, and ask a good programmer to look at the code to see if he is willing to take ownership. If you don't know if you have a good programmer, ask a programmer to add a couple of automated unit tests to the code you want reviewed. That will result in a better code review than any meeting or pairing.
I'd say it's hard to say. If you are going to be expected to keep adding features, or work on something else, then the code reviews would be useless -- you could find out some code is crap but would not be given the time to do anything about it, as long as it barely works. If you have some control over improving the product, then do code reviews. Sounds to me like your current boss is in the first camp, doing code reviews wouldn't help at all then.
Then there's the powers of ten cost of fixing problems...
Design = $1
Development = $10
Debugging = $100
Deployment = $1000
- Frederick P. Brooks
The Mythical Man-Month: Essays on Software
Engineering, Anniversary Edition (2nd Edition)
Think of it this way....
So, anything you catch in design costs you a "buck" for your effort. If you catch it during development, it *still* only costs you "10 bucks."
If you wait until debugging ("100 bucks") or deployment ("1000 bucks"), you're hosed.
You can be more or less effective during a code review, but as long as it catches stuff, it's still far easier, cheaper, faster, better to catch it there, than later... Yes, it may be 10x harder to catch during a code review than a design review. Well, you're just proving the Mythical Man Month to be true. Your boss knows it, too, and he's trying to stop the vicious debug -> deploy problem.
Do you want screaming customers at 4:00a, calling you about some obscure error in a program you wrote, when it might be avoidable? Does your boss want to earn blame for a team of "bad coders?"
I'd say that it's good that your boss wants this bit of extra discipline -- for your sake, his, the customers', and the company's....
The importance of code reviews depends of where you work and about how serious people making it are.
As I see from where I am, code reviews at least tends to bring the code to be easily readable by anybody as it is made to enforce the programmer follow the rules set by the company. ...) to gives some hints to the reviewer to speed up is process and perhaps find something.
As for the cost of code reviews, you can manage them. Some part of it can be made with some script (parsing through files, check for unused variable, compute comment / code ratio by function,
As for now, you are working on non critical PC software but when you are on some low memory, old crap machines, you like to check if there is any memory leak, or at least memory fragmentation. those can be found with a code review before being found by a user (but yeah, the programmer should find it and fix it before he commits his code).
In my experience, code reviews help the developer much more than they help the product. And this is a Good Thing.
Take me, for example. A few years ago, I was just out of college, 9 months on the job with a global consulting firm, working on a large Java-based web application for a government department. I was assigned a complex and poorly-defined module to write. While my first stab at the module worked fine, passed unit testing, and satisfied all the functional requirements, it was kludgy and would have been inelegant for maintenance programmers to work with. One of the project managers took the time (about two hours!) to go over my module, reading the code until he understood it, and then asked me penetrating questions and made really good suggestions about how to change it so that it didn't just work, but was elegant and maintainable as well.
Those two hours, and the subsequent 30 that I spent rewriting that module, cost my firm one or two thousand dollars, I suspect, on a fixed-price contract that was already over-budget. From an economic point of view, it made no sense for that review and rewrite to occur. But they made me into a much better developer both from a technical standpoint (I learned a better way to design the algorithm) and from a social standpoint (I learned a better way to make my code maintainable). I really liked being part of a company that was willing to sink that time and money into helping me improve my skills.
So should companies have code reviews? Absolutely -- if they care about increasing the skills and maturity of their developers. If they intend for their developers' skills to stagnate, and they don't understand the value of investing in the continued education of their staff, then surely not.
A cat is no trade for integrity!
Having standards for coders and requiring compliance from ALL will speed things up a bit ... it helps prevent the "coding style" harping from taking valuable time.
Our code reviews also take in account compiler efficiency/deficiency knowledge ... this can help free up a lot of time (from later optimization efforts and repeat debugging of compiler issues) and help teach newer coders about such things.
Email pass-around Source code management system emails code to reviewers automatically after checkin is made.
It works much better than expected. We started on a 1st version about 2 years ago with 4 developers. After the shipment, we had a "what was good/bad, what could be better"-meeting. The bad things were the same shit as in so many projects: not documented dirty and unmaintainable code, which looked like a quick perl 1liner to check an e-mail address, but it was Java and C++! If your co-worker is touching the code parts you're interesed in or working on, you have to review his check ins and vice versa. At least it ensures, that the checked code is:
a) Documented
b) Doesn't contain 100+ characters long undocumented regex
c) Doesn't look like your favorite pasta dish
d) You know, what is going on and it makes it easier to maintain that code in the feature
By the time we doubled the team and it seems to help. If some developer gets 2-3 "WTF?!!"-mails a day from his co-workers, he starts to think before commiting the next time..
Cancel the code reviews and you'll end up with more errors even though the code reviews themselves don't find that many errors. The very act of having code reviews prevents a lot of the problems that would be found in a code review because people are more careful when they know someone else is going to dig deeply into their work.
The only thing necessary for the triumph of evil is that good men do nothing.
We can't let this discussion pass without a word from Dijkstra (EWD340):
Argument three is based on the constructive approach to the problem of program correctness. Today a usual technique is to make a program and then to test it. But: program testing can be a very effective way to show the presence of bugs, but is hopelessly inadequate for showing their absence. The only effective way to raise the confidence level of a program significantly is to give a convincing proof of its correctness. But one should not first make the program and then prove its correctness, because then the requirement of providing the proof would only increase the poor programmer's burden. On the contrary: the programmer should let correctness proof and program grow hand in hand. Argument three is essentially based on the following observation. If one first asks oneself what the structure of a convincing proof would be and, having found this, then constructs a program satisfying this proof's requirements, then these correctness concerns turn out to be a very effective heuristic guidance. By definition this approach is only applicable when we restrict ourselves to intellectually manageable programs, but it provides us with effective means for finding a satisfactory one among these.
I'm a Programmer. That's one level above Software Engineer and one level below Engineer.
My company instituted code reviews and I spent some time coming up with why they are useful: http://jasonhasalife.blogspot.com/2008/07/code-reviews-are-your-friend.html I am a huge fan, just for what can be learned. But I agree that often corporate 'code reviews' are nothing more than a boring meeting.
Think of a place where there are no code reviews. Where people hide there code and never learn from each other. Thats where I work... I say do the code reviews.
have you tried to use scrum as a style for running those development projects? i had positive experiences with that. but it's very different from the "classical" project style and it is important to follow it's rule completely - not only to take the parts you want of it - the scrum leader should have some training in his role before taking that position (there are courses you can take and even certifications to get ;)) and the product owner (mostly the customer - whoever it is) also has to comply with that as he has a quite different roll, too, compared to the classical project style....
see here http://en.wikipedia.org/wiki/Scrum_(development)
Code reviews have a lot of benefits, most important to find bugs during development phase and to enforce coding standards -- and they help to share knowledge about a peace of code with other team-members (called collective code ownership). But most often traditional inspection methods consumes a lot of time, hence developers are discouraged doing code reviews during their regular work. Nowadays physical code-review meetings are also a problem, because development teams are shared across offices/countries/continents, having different time-zones. The planning of review-meetings is hard or even not possible. Another problem is the delocalization of code in object-oriented and dynamic programming languages. Inheritance, dynamic binding & co. make it hard to understand an instruction, since developers use foreign classes and frameworks, and the business logic is shared across them.
Based on our experiences from previous projects we developed a code-review tool which follows a different approach: The review item is one changeset in the revision control system. Further author/reviewer assignments are defined. The reviewer subscribes to changes (new revisions) from the author and gets a new review task if the author makes a commit to the respository. The assignments may be done in a peer to peer manner, where the developers inspect the code changes of one or more team members. Reading each others code has the consequence, that developers also learn the code from their colleagues, thus supporting collective code ownership. A more hierarchical approach would be to assign a senior developer to review the code for new or less experienced developers. The senior developer in the role of the reviewer may enforce coding standards and better code quality within the development team.
The review-tool is called ReviewClipse (see http://www.inso.tuwien.ac.at/projects/reviewclipse/ for more information). It is free, full-integrated into Eclipse, and very easy to install and to use. All review data is stored within files, shared with the already configured repository of the Eclipse project. So there is no need to setup any server-side application, apart from the already existing versioning system.
I haven't done all that much HTML.
And I have references for all the places I have worked who will tell you that they felt I contributed a great deal, and who will confirm that I write good code and know what I'm doing.
Unfortunately, driver development is not something I've ever done before, and it would take me time to learn the things I'd need to know to do it.
Need a Python, C++, Unix, Linux develop