Are You Too Good For Code Reviews? 495
theodp writes "Why do some programmers,' asks Chris Hemedinger, 'place little value on code reviews?' This apparently includes even Programming Greats like Ken 'C' Thompson, who quipped, 'we were all pretty good coders' when asked about the importance of code reviews in his work. Hemedinger, on the other hand, subscribes to the school of thought that peer code reviews are Things Everyone Should Do. Not only do reviews keep you on your toes, Hemedinger says, they also 'improve quality, ensure continuity, and keep it fresh. Who can argue against that?'"
We need more testers / QA as well (Score:5, Insightful)
And some of that needs to testers who are NOT coders or people who are not mainly doing programming.
Re: (Score:3)
With automated testing coming along in it's depth and flexibility, implementing the tests themselves is becoming it's own version of programming.
One of my biggest gripes is testers who don't know SQL. How do you set up your test cases without preloading the database? If you're manually entering data through the app you're testing you still need to verify that data got input correctly and that i
automated testing can let stuff fail but still pas (Score:2)
automated testing can let stuff fail but still pass the auto tests and it still misses the GUI / user experience testing.
Re:automated testing can let stuff fail but still (Score:4, Insightful)
Re: (Score:2)
When your input field is overlapped by your nice little logo, there's no way to enter anything in it (for the average Joe). You page may "work" but if users can't figure out how, it it as good as a broken database.
Re: (Score:2)
Functionality tests can also be important for regression testing. I remember a ruby site I once worked on where, due to some change, registration of a new user didn't work anymore. All the individual classes worked fine, but something wasn't redirected correctly, or not properly stored in the database or something like that. Automated functionality tests allowed us to keep an eye on that.
Re: (Score:2)
One of my biggest gripes is testers who don't know SQL. How do you set up your test cases without preloading the database? If you're manually entering data through the app you're testing you still need to verify that data got input correctly and that involves looking under the hood in the actual DB.
And yet we wonder why there is a rash of SQL injection attacks on public websites and servers by teenagers who are pissed at the world.
Re: (Score:2)
But testing is not code reviewing.
However there are tools that can help a lot when doing code review. But you also have to agree on certain policies and also agree on what shall be ignored since some review remarks dropped by those tools are just foolish or counter-productive.
As for testing - that's a later stage in the process of development.
Re: (Score:3)
As for testing - that's a later stage in the process of development.
Apparently by teenagers and young adults angry at the world who like nothing more than to 'test' websites with a nice little Iranian program, complete with a GUI and tutorial. They are all to happy to share their results with as many people as possible... all for free. Normally you have to pay for that:P
Re: (Score:2)
Test at earliest possible moment (Score:4, Insightful)
As for testing - that's a later stage in the process of development.
Testing and code reviews should occur at the earliest possible moment and be integrated throughout development. Bugs cost less when they are found earlier.
Re: (Score:3)
The problem with code review is that it's expensive. For someone else to go through all the code to the point that they understand all of it, they have to invest at least as much time as the coder.
They don't, so they don't, so the code reviews become skimming for obvious flaws, which is well and good, but it isn't a code review.
To do it properly, your code becomes twice as expensive. That's cheap, but try explaining that to a PHB.
I don't always test my code, but when I do.... (Score:3, Funny)
Re: (Score:2)
Awesome!
Re: (Score:2)
And some of that needs to testers who are NOT coders or people who are not mainly doing programming.
Dude, you need a comment reviewer.
Pure Arrogance (Score:2, Informative)
Anyone who is anti-code review is an arrogant snot. No one is perfect, and we all have the capacity to overlook things.
Re: (Score:3)
Code review is purposefully a politically loaded process which enables management to divide and conquer and keep wages down.
Such processes can get hung up on stupid details when coder A's readability is better than coder B and C's, but unfortunately for A, is also not "standard" and s/he gets slammed for it and told to conform even if it makes for slightly less readable code just because it satisfies some corporate "this is how we do it and you don't question it".
I think code reviews would be much more valu
Re: (Score:3)
Code review can be that, but doesn't have to be. For example, at my place of work, code review results aren't documented, and can't be brought up during personnel review. The purpose of code review is quality (for us).
Re: (Score:3, Insightful)
Re: (Score:2, Troll)
Re: (Score:3)
You *can* do all that stuff, it's pretty unlikely though.
Unless you are a coding god (and I'm pretty damn good) then you occasionally make mistakes (we're all human) and you'll always benefit from the perspective of another skilled professional.
I wouldn't work with anyone that had a serious opposition to reviews. Everyone misses something sooner or later. You are not as special as you think you are.
Re: (Score:2)
Re: (Score:2)
I contend that you can do all of those things without someone looking over your shoulder
Because humans never err, right?
Re:Pure Arrogance (Score:5, Funny)
Re: (Score:3)
Re: (Score:3)
The problem with those "best practices" types is they usually spend 90% of their time nitpicking, and the remaining 10% copy/pasting Java patterns out of someone else's email. So yes, I agree with you, fuck 'em.
The truth about code review is that you're not actually supposed to pick on stupid shit. The goals are usually:
1. Make sure you didn't miss something important
2. Make sure other people can read and understand your code
Beyond that, who cares ? Code review isn't about belitting your colleague, it's
Re: (Score:2)
If, after 13 years, you don't know that other people need to be able to read and understand your crappy code, and you don't think it's important "how it is written", then you're hopeless. I would never hire you, no matter what you produce for "output / functionality". Sorry if this sounds harsh, but you're the type of programmer that everyone who has to follow you HATES.
As for me, my biggest source of pride is when programmers come up to me who have read my code and compliment me on how clean and well writt
Re:Pure Arrogance (Score:5, Interesting)
Because if it's written with an eye only towards effectiveness, and not towards standards and readability by others; then by the standards of any project larger than "A script I use to clean up my home directory" it's crappy. He's being kind of an arrogant ass about it, but his overall point is completely valid. If you quit or get run over by a bus tomorrow, and I (for a value of "I" that indicates a competent peer familiar with your company's standards and the language in question) can't pick up your project/module and continue development/support then it's not "good" code no matter well it works. That doesn't mean you can't be creative and awesome in your implementation, it just means you have to write the shit so someone else can read it; and in a pinch finish/fix it. That's what standards are for.
Re:Pure Arrogance (Score:5, Funny)
He's being kind of an arrogant ass about it,
I earned my Arrogant Ass Badge honestly through hard work and paying my dues, so I reserve the right to pull it out on occasion. :)
Shitty code can come up with the right answer (Score:3)
I can't figure out why you jump to the conclusion that my code is 'crappy'.
Perhaps you are the exception, but I (25 years exp) agree with the other poster in a general sense. When someone says "I want my code to be judged on it's output / functionality instead of how it is written" that is a big warning sign. People who make such statements are often those who slap together mediocre code as fast as they can, with only narrow use in mind ... code that in the long term tends to be difficult to maintain, buggy beyond the original narrow use envisioned, and difficult to adapt for rela
Re: (Score:3)
The question is, are code reviews really effective? I worked on a project where we did extensive code reviews and we rarely found any real bugs. Going through a program line by line and finding a non-obvious bug is very difficult. Also, a thorough code review is also very time consuming. It takes almost as much time to review code as it does to write it in the first place. When you have four or so people reviewing every line written, your productivity for the project goes way down. In my judgment, code revi
I agree 100% (Score:5, Insightful)
Re:I agree 100% (Score:5, Insightful)
Yes code reviews can be painful especially if you work with some douchebags.
I don't think the problem there is the code reviews.
Re: (Score:2)
Re:I agree 100% (Score:4, Interesting)
I have a huge ego. I freely admit it. I work from home because I can't fit my ego in the office doors.
On a serious note, I prefer code reviews. From the ego-centric side, it has two major benefits: it allows my ego to show off, second it helps the less-experienced/newer developers to learn new techniques, or just learn the code, often in areas they haven't spent a lot of time looking at. That means less time helping them later. It also happens to have the side benefit that their questions can make me think about the problem harder, and not-infrequently uncover typos, thinkos, under-developed (or over-developed) features, or plain bugs. Even for us egomaniacs.
Re:I agree 100% (Score:4, Interesting)
I've found that code reviews are most productive if you pair experienced and inexperienced coders. An experienced coder is probably writing good functional code, but may not be writing code that's easy to understand. Having code reviewed by someone less experienced helps catch things where someone new to the codebase would struggle to understand the code. This should prompt additional comments or (ideally) refactoring into a clearer form. The inexperienced developer also benefits from being forced to read and understand someone else's (hopefully good) code. In the other direction, the inexperienced developer gets helpful feedback on their design and techniques.
Code reviews done by someone with a similar skill level tend to be a waste of time. They'd typically implement things in more or less the same way, so they either skip over things and don't give much feedback or they're really pedantic but not very helpful.
There's a point when... (Score:3)
... code is good enough.
There are times when code reviews make sense and times where they don't, having the wisdom to know the difference is key.
Re: (Score:2)
And that's when it builds with no warnings whatsoever using the most paranoid settings and with a lint analysis that's silent. Then it's good enough.
Re: (Score:3)
Ohh, and it does what the customer expected...or was that part optional?
Re: (Score:2)
Warnings and lint are a start, but they say nothing about whether or not things actually work.
When the required functionality is specified, and a test procedure that actually tests the required functionality (to the degree appropriate for each function's level of concern) passes successfully, then I'll say that the code itself doesn't need further review.
When new specs come along, or a new programmer is added to the project, some form of code review is called for.
Re: (Score:2)
having the wisdom to know the difference is key.
Wisdom would dictate that the author is incapable of judging when it is good enough. Ergo, even if it is already good enough, the code review is necessary in order to make that determination. Fortunately, good code is relatively easy to review.
A matter of time... (Score:2)
I love code reviews; I usually like doing them and I really want my code reviewed too. But it can be like pulling teeth to get co-workers to do them in a timely manner. So yeah, I commit unreviewed code unless it feels risky.
Re: (Score:2)
I love code reviews; I usually like doing them and I really want my code reviewed too.
I agree, it's one of the better learning experiences short of 'pair programming' (which I can't stand).
The single biggest problem I have is that time is never budgeted for any serious review times. Reviewing minor changes can be quick but if it's entirely new code, it needs to be reviewed in how it interacts with it's interfaced components and that add significant time to figure out if it's working properly or not.
And that my gov't contractor employer has this ridiculous 'every line of code must be r
Re: (Score:2)
I commit almost hourly, reviews are for when the team needs to understand what's going on in the code.
In the "serial monogamy" relationship that developers have with code around here, divorce and re-marriage time is usually when a review is appropriate.
Welcome to the industry, Chris. (Score:4)
Perhaps peer code reviews were less important 30 years ago. The constraints around running code were much tighter. There were fewer layers in the stack where bugs could lurk. Memory was scarce, instruction sets were limited. Computer applications were like my 1973 AMC Gremlin: fewer moving parts, so it was easier to see which parts weren't working correctly and predict when they would fail.
Welcome to the industry, Chris. This might have been the funniest thing I read all week.
I'm not too good for code reviews (Score:5, Insightful)
I'm too busy for code reviews.
Development schedules are almost always ridiculously compressed. There's no time in the schedule for code reviews. Just like there's no time for thorough testing.
But the software only has to be 'good enough' for people to buy it, so there's no ammunition for developers to use to get a better schedule.
this (Score:3)
In the time we all spend reviewing my code, we could have each fixed separate bugs in the software or completed a new feature. Not only does the code review practically halve my productivity, it halves everyone else's.
Re:this (Score:5, Informative)
I've caught bugs in code review that would never in a million years have been caught by testing. The customer would've encountered them sporadically, and in situations in which they'd come to rely on the broken behavior working implicitly. The would've submitted a bug report and we would've been unable to replicate the problem in house. The customer's opinion of us would be lowered, and we'd have all these bug reports in our system that nobody could figure out.
That is, until someone reviewed the code and discovered the bug.
So, basically what you're doing is robbing Peter to pay Paul. By not doing the reviews you end up with less time in the long run, not more.
Re: (Score:3, Insightful)
Re:this (Score:4, Insightful)
That's all nice and dandy, but the problem is that in reality it still makes sense sometimes to produce and sell buggy software. This is so for a variety of reasons. Amongst them the fact that actually pulling off a business is a very multifacetic thing, and the product is only one of many aspects. In many cases, the cost of making a good product is simply prohibitive when viewed in the grand scheme of things.
Reality is messy.
Re: (Score:2)
In the long run, more of your code will be "perfect", but someone else could have made more code that is "good" in the mean time.
If you want to do campfires and share coding tips, it should be done when things are broken. When it's broken is a great time to discuss fixes and "the right way to do something". Get everyone together and have them brainstorm on what the problem is.
When things are working, it is not such a great time, because sometimes those fixes end up breaking something else. If it ain't br
Re:this (Score:4, Insightful)
My goal, as a developer, is to make sure QA never catches any bugs. When they find a bug, I'm embarrassed, as I should be. Developers who have a purposeful strategy of waiting for QA to find their bugs should be fired.
Re: (Score:3)
And it's been shown that the earlier a defect is found, the cheaper it is to correct.
Re: (Score:2)
>
But the software only has to be 'good enough' for people to buy it, so there's no ammunition for developers to use to get a better schedule.
This is why "life safety" industries have quality standards that require documentation of design controls, verification and validation activities.
Thing is, most industries have "fatal accidents," where the business itself is killed by relying on a bunch of technology that nobody really understands. Capitalistic evolution hasn't had enough generations of software based companies for the "fit and nimble" mammals to take down the dinosaurs yet.
Re: (Score:2)
I'm too busy for code reviews.
And yet I'm guessing there's time to it right the second time. How much time is spent on patches and bug fixes?
Maybe if you had code reviews, you'd be less busy.
Re: (Score:2)
Re: (Score:2)
There is a difference between a job and a profession, the person doing the work.
Meh, not so much. Last time I looked every profession was under attack. Labor in general is under attack. Any task that cannot be performed by a machine, for whatever reason, is being redefined, outsourced, minimalized, etcetera. Even real professions, like doctors, engineers, teachers, lawyers. And I bet you just thought "wait, teaching isn't a profession" which goes to show how much that profession has been destroyed.
Who in management thinks of coders as being in some sort of profession? The coders are
Re: (Score:2)
This is typical and why there is much failure in software today. Developers claim no time for code review. Software gets deployed. Bugs found in software. Company spends MORE time and money to fix it after the fact rather than before. I don't blame you, per say, I blame your managers who set your schedules. Most managers are not software people -- managers need to understand the importance of code reviews.
Company gets to charge customers for an "upgrade".... Profit!!
Re:I'm not too good for code reviews (Score:5, Funny)
I heard that Geological processes have pretty relaxed schedules.
Depends on the reviewers (Score:2)
Code reviews are certainly not a magic bullet. Maybe they are applicable to a situation, maybe not.
Code reviewers are human, so are subject to all the potential problems there. I've seen code quality dragged down by code reviews because the reviewers either could not or chose not to (for various reasons, including inter-department politics) to understand a piece of code (even when it could be shown that it was the 'standard' or best current practice solution.) Ultimately the code was degraded down to the
Obvious answer is obvious (Score:3)
Because code reviews end up being an enabler for OCD coworkers to tell me how to indent.
Re: (Score:3)
Because code reviews end up being an enabler for OCD coworkers to tell me how to indent.
This.
Conflicts in particular coding styles can easily derail a code review that is supposed to review for security and bugs into how something could be made minimally more efficient while eliminating overall code readability/maintainability. And there is always the guy who never comments his code, the peer reviewers take time to read through it trying to figure out what he is doing, provide comments and include a note t
Re: (Score:2)
I've almost lunged across a transatlantic gap and strangled someone through a phone line because they indicated I needed to rewrite my entire program to encode the names of the design patterns I was using into my object names and methods, or they would block my submission. This from a fellow who bulk submitted about 30 changes resulting in the build being broken for a week and a half. ...
That said, there IS a place for it. Code architecture is somewhat subjective, and two people may come up with a more ef
In the perfect world maybe .... (Score:3)
But I don't have all the time in the world, so I do what I can given the time that I've been given to get work done. I consult my peers often enough that reviewing all the code all the time is a waste of both my time and theirs.
It shouldn't be necessary to review every scrap of code. I think I've written enough that I have the basics down pretty well. And when I get into new subjects, I always talk it over with someone to see if we can find an optimal way of doing something.
If two people have already agreed on the best way to code a specific task, having them review each others work all the time is a waste of time.
Re: (Score:2)
But I don't have all the time in the world, so I do what I can given the time that I've been given to get work done.
So you, like me, are posting on /. at 11AM EST.
The problem is poor developers... (Score:5, Insightful)
The problem is that the need for code reviews is driven by lax, sloppy developers who don't see regression testing as a requirement, and who foist crappy, untested code that, in many cases, they haven't even tested.
As a consulting exec (experience side) who oversees software delivery I can't even begin to express the stunning crap that I see developers submit for "qa/review". Crap that doesn't even WORK correctly in the first place is submitted for testing, with the QA feedback often "Does not work". Aside from the hours of UE and QA resources this burns with useless testing, it highlights what I think is both an increasing lack of accountability and a lack of professionalism within the development community in general.
What's driving this I have no idea... less formal CS training? Looser languages? Web-centric apps? Lower end standards? Higher demand = more crappy resources? Whatever it is I'm seeing it everywhere, and it's driving me nuts. The lack of an appreciate for regression testing is absolutely insane... code reviews are just symptomatic of a larger problem, which is a lack of quality and skill.
-rt
Re: (Score:2)
The problem is that the need for code reviews is driven by lax, sloppy developers who don't see regression testing as a requirement, and who foist crappy, untested code that, in many cases, they haven't even tested.
Code review and regression testing are separate animals.
Code review is an application of the hive mind to the problem at hand, and an exchange of knowledge among the worker bees which can, in an ideal world, lead to better productivity for everyone after the meeting.
Regression testing is an (imperfect) attempt to be sure that requirements are met.
The two are complimentary, both can be misused, abused, and turned into a mockery of what they should be.
My own experience (Score:2)
Hemedinger says, they also 'improve quality, ensure continuity, and keep it fresh. Who can argue against that?'"
You mean besides the hordes of contrarian Slashdotters looking for the word Insightful to appear next to their posts? Heh.
Well, I wouldn't bother trying to argue with that. I know that I'm more apt to design and implement my scripts a little more thoughtfully when I know somebody else is likely to pick them up and work on them. Unless you suffer from 'code fright' it's difficult to picture a scenario where this wouldn't be preferred.
See monkey do (Score:2)
If you are too good for code reviews, you should still do it. It will teach your reviewers how to write better code. Yeah, that's why you should.
Re: (Score:2)
If you are too good for code reviews, you should still do it. It will teach your reviewers how to write better code. Yeah, that's why you should.
I once ran into a really excellent programmer, much more clever than I am. Where I would compare a pointer to NULL by writing "if (p == NULL)" he really preferred "if (! p)" because it demonstrated his superior intelligence.
One day he changed, for no good reason, a line "if (p != NULL)" to "if (! p)" and checked it in. Unlike every other programmer in the company, without a code review because he was an excellent programmer who didn't need code reviews.
That line of code was only ever executed in a gra
Code review as a method for knowledge passing (Score:4, Insightful)
I tend to use code reviews as a method for showing other team members the work that I have done so they will know about the changes. I work with a large number of geographically disperse people and having 1:1 conversations about every little change can be tiresome and wasteful. Code reviews are an excellent way to get around this problem.
Another bonus for code reviews, along the same lines - is in teaching people new to a platform about the kinds of bugs to expect when working on the platform.
It also keeps me honest, did I just add a debug, or do I also need to add trace? If I forgot the trace, code review will pick that up.
"formal" code reviews: a waste of time (Score:3)
Having another pair of eyes on your code is a very good idea but this whole concept of everybody getting together and put somebody's work to shame is bad for team dynamics and a waste of time.
I rather have a team dynamic where any developer can pick up a task involving work on somebody else's code, this way code reviews just happen naturally.
Short answer: Yes (Score:2)
Longer answer: Code Reviews are a "best practice" that really needs to be driven by management and ingrained in the culture as "one of those things that you do" like showing up on time every day (even if that time is 10am), participating in Scrum, turning in your timesheet, etc.
If management is lax and doesn't require code reviews on a regular basis (or, even worse, does them "as needed"), then it is an awkward unpleasant process that can become counterproductive.
So, most places I have worked have treated c
Code Reviews Don't Find Bugs (Score:2)
Effective code review would take a huge amount of hours and the customer just isn't going to pay for it. It ends up just a show so we can check off that box on the CMMI audit.
Re: (Score:2)
In my shop, if a code review shows that you did not follow proper design standards, you redesign it (re-implementing, if need be). It's bitten me before, but in the long run, I'm thankful. I realize this is not SOP across the industry due to schedules and whatnot, but I think it should be.
Re: (Score:2)
I've rarely seen code reviews find anything useful. They mostly turn into nit-pick sessions about naming conventions while real bugs slip right through. And they're always done at the last minute
Clearly, you're doing it wrong. Not saying that your particular shop is capable of doing it right, but if nobody is even trying to do it right, what do you expect?
or shift the solution elsewhere (Score:2)
there are many tools that produce better software quality, btu they all seem to be up-front, and require a large amount of programmer time. This is probably because they are produced by programmers for programmers.
so test-driven-development is very fashionable at the moment, even though it doesn't really work well with legacy code and can take an amazing amount of time to set up for new; similarly code reviews are great, but require lots of time for a (presumably) better coder to check what you did is ok (a
Code Reviews are never done right. (Score:2)
The benefits of code review or intangible immeasurable benefits in the future, most likely going to benefit whoever is going to be in my chair at the time. So why invest in it? That is another tack taken by managers.
But if bugs are found, we could estimate the resources needed to fix it and expand my empire. That is a
I don't like looking at other people's code. (Score:2)
I don't so much mind having my code looked at. But I don't like looking at other people's code. Does the application work? Great. I don't need to see your code then. If you're happy and the user is happy, I'm happy.
Absolutely Nobody (Score:2)
Nobody is so good that they should not have their code reviewed.
If you think you are that good, this is the first sign that you shouldn't be coding. Management Needs You.
Waterfall Vs. Agile (Score:2)
Are you working in an environment in which you're the only developer working on your particular piece of code and everyone else has their own little fiefdom as well, or are you working in a more collaborative environment, such as an Agile/Scrum/whatever environment that practices paired-programming, collective code ownership, and TDD? I've worked in both (I'm currently in an Agile environment and never plan to go back to the "old" way of doing things in case you're wondering where my bias lays.), and I gott
It depends (Score:2)
Judging by... (Score:2)
the amount of craptacular software that is out there, both free and paid, I would say the answer is yes.
Between the exhaustively documented bugs in Civ and AC (to name two games from the same person/group), to the WTF? in many open source packages, coders, as a group, believe they are too good for code review.
From a time before software tools (Score:2)
Now things have changed. To expect people to find the time in their schedules to try and understand the sofwtare that one of their colleagues has written is impracti
Code review pros and cons (Score:2)
Code reviews are good for finding bugs and - if you happen to have one - confirming to a coding style guide. This means that we'll be more likely to find bugs, and the code will be potentially easier to maintain. Seems like it's an easy win, at the cost of some small amount of dev time.
However, there's a big list of detriments.
Let's take the average developer, who stereotypically has a social coping mechanism that is ... shall we say off-by-one. Take the thing he does well, and subject it to criticism ev
Re: (Score:2)
Code reviews are good for finding bugs and - if you happen to have one - confirming to a coding style guide.
I used to think style guides were a "Good Idea (TM)," but, then, I started getting lazy and importing other people's source code. Big chunks of it. From within the company, from LGPL sources, etc., and, guess what? Reformatting all that code to fit a style guide is a colossal waste of time.
I actually like the "personal style" that we use here, every programmer has their own preferred way of writing their C code. As a programmer, you should be able to decipher code regardless of the style, so, now, when
Code reviews need people intimate with the code (Score:2)
All these things are good. Unit tests, code reviews...
However, they all come with conditions to make them useful.
Unit tests for example require clean abstracted code to be useful... and they cannot be used to test integration and communication without significant effort. My own view is the primary success of unit testing is that it forces developers to write well abstracted structured code... not in the actual benefits of testing units.
Code reviews require skilled people with good knowledge of the code to
The secret to effective code reviews (Score:2)
Re: (Score:2)
The secret to code reviewing is very simple: Don't be a Dick (tm).
So very true, but since virtually all programmers are male....
I love code reviews (Score:2)
About Code review (Score:2)
I seriously wonder how many peopel ehre know what code reviews are about?
a) Drop your ego. You are not gods gift to programming, and you can always improve.
b) Many eyes make for shallow bugs
c) It ensures there is a control on creep.
d) It provides good and more accurate feed back regarding scope.
e) It lets coders know if their code is understandably
f) It bring Jr. Coders up to speed faster and increases their skill
g) It lets Jr. Coders inform Gray beards of new techniques.
h) It ensures people aren't slacking
ReviewBoard (Score:3)
If you use some software, then the process is MUCH easier. http://www.reviewboard.org/ [reviewboard.org]
Good, the Bad and the Ugly (Score:3)
I *wish* I had code reviews! Granted, I would also like to choose my reviewers, or at least set a minimum bar ("first, write the FizzBuzz program in the language that the code we are reviewing was written in"). I can understand not wanting to subject yourself to people who really shouldn't be reviewing code (one comic I saw had a "reviewer" stating "all those curly brackets and semicolons really ruin the feng shui of the code"). And maybe some (very rare) people don't need code reviews (I suspect that most people would get more out of reviewing Ken Thompson's code than Ken Thompson would get in return; in other words, learn from the greats). But most code seriously needs to be reviewed. Just drop the ego, get over yourself, and set some standards for reviewers and coding style, then review.
Samba slowly moving to mandatory code reviews. (Score:3)
We already have this for all release streams. Only one person (Karolin) the release manager can commit to a release stream - all other changes have to be associated with an open bug report and go through at least one code review before going in.
Master branch is still more open for rapid development, but we're moving in the direction of more and more code reviews.
I don't like doing code reviews, but I like having my code reviewed as other people find my bugs :-).
Jeremy.
If code reviews are good... (Score:3)
...then continuous code reviews are great. Keep two sets of eyeballs on the code *as you write it*. Yes, the antisocial archetype hates pair programming, but it simply works, even if it is uncomfortable (as code reviews often are).
The real problem with formal code reviews is that all too often, the reviewers are so distant from the day-to-day issues in the code they can't to much more than give superficial suggestions ("I don't like how you named this variable", or "we need more comments here"). Arguably, to do a truly thorough code review, you'd have to spend *at least as much* time prepping for the code review as it took for the coder to put it together in the first place...quite probably several times more.
If they really wanted to make them more useful, though, code reviews should be conducted on running code, and any review item should be immediately fixed in the code, and the whole regression suite of tests run in front of all the reviewers.
Nobody wants code reviews (Score:3)
Rather, almost everyone loves the idea ('oh yeah, we should do that'), but as soon as you attach the time/budget cost to it they decide it wasn't that important anyhow.
And they are extremely time intensive. Coders need to walk through the code line by line in advance for the review to be useful, then there's the drawn out meeting itself, and usually a manager wants to be there which adds more budget and slows things down with useless questions, then the writeup.
Ah, you say, but it will save time in the long run, because it takes less time to track the bugs down now rather than one at a time later. But (almost) nobody is willing to trade possible future time loss against significant right now time loss.
It depends on who's doing the reviewing ... (Score:3)
A nice caricature can be found here: http://www.jsquared.co.uk/jennyl/verity.htm [jsquared.co.uk]
Don't believe that his sort of response during code reviews is all that uncommon.
Re: (Score:2)
Even good coders make mistakes. There can be various reasons for this, maybe someone was suffering from insomnia the night before and their mental processing is slower. Maybe they were working on a part of a project and there are integration issues in their code between a part of the project that they were not as familiar with.
Not every issue in code can be found by peer review, and not every issue in code can be found by testing. A good team has a combination of good coders, good peer reviews, and good
Re: (Score:2)
A good team manager will factor in testing. Like the actual coding, testing is just a part of the process. Would the business stakeholder argue that he would use an untested product himself? Or use their clients as testers? Then tell his boss to terminate his contract, there cannot be any compromise for quality.
Re: (Score:2)
I can. I found the solution was to do code reviews of other people's code and find the same mistakes. I hate pair programming, test-driven design, and related fads, but code review is one solid piece of good practice I really miss about industry.