Are You Too Good For Code Reviews?

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?'"

Comments

We need more testers / QA as well

[Joe_Dragon]

And some of that needs to testers who are NOT coders or people who are not mainly doing programming.

Re:We need more testers / QA as well

[pixelpusher220]

I would also say we need more testers who 'are' coders, just not 'the' coders of the software being tested.

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 involves looking under the hood in the actual DB.

Re:We need more testers / QA as well

[Z00L00K]

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:We need more testers / QA as well

[memyselfandeye]

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:We need more testers / QA as well

[memyselfandeye]

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:We need more testers / QA as well

[Tsingi]

teenagers who are pissed at the world.

That's a redundant statement. You could just say teenagers.

Re:We need more testers / QA as well

[EraserMouseMan]

Yeah, goodness. Maybe if all the lazy IT support and testing staff would quit trolling /. and get back to their job we could get back to the original topic here. Code Reviews by coders who know how to write code reviewing each other's code. Not UI crap, or testing in general.

Re:We need more testers / QA as well

[seyyah]

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.

Re:We need more testers / QA as well

[gl4ss]

if you start hiring people who actually have an hour of work per week.. well, eh, you might as well just have _real_users_, or those testers blow your budget and office morale - if you start just testing with random people who got no idea what the application is for, they're pretty unlikely to be able to be instructed to test the new functions on every iteration. btw. automatic testing is a joke, it just test something you already prove works in the automatic setting and generates totally, totally useless reports which tell nothing of the state of the actual project, quality of code or of how re-usable the code is. just increasing the qa team can lead quickly to the qa team doing the specs and therefore cementing what the program is, while they can't have an idea what the program could do. also if code reviews are only done by people who got no experience in programming for the target system with the tools chosen, the got no idea why something is done like it is. unit testing to the max in a hurry with a moving design of course is a joke in itself.

IF the project is re-implementing something that was already designed and proven to work then all this is different, of course, but that rarely happens(and in that case you could actually take a lot more shortcuts to conserve memory, no need to build every class like it could be extended later in the design perioid, block potential security holes in the design, by implementing _less_-many production j2ee servers fit to this-, etc etc). I don't mind code reviews but I'd rather not have them while the sw is still in design phase and nobody knows what the end result should be.

Re:We need more testers / QA as well

[Synerg1y]

I'd take more QA over testing anyday. As coders we can fix our bugs with enough time if they are pointed out to us. I can see the benefit of optimizing code and making it efficient, but I doubt the cost is justifiable. The end user who knows nothing about code and doesn't care, only cares if the textbox is working correctly or not, I guess though... I can get specific... There are different types of programming, application and hardware as an example. Hardware code is low level and something like an incorrect pointer can directly translate into a 1% performance loss from that hardware component. Thereby why multiple drivers exist for say nvidia / ati graphics cards. Application level programming uses the hardware, and boy do we have a lot of it and it's cheap. So between buying a 20k server with 60gb of RAM, or paying a developer 50k annual salary most CIO / CEO types choose the server.

Re:We need more testers / QA as well

[DrgnDancer]

"Not UI crap, or testing in general."

And this is exactly what's wrong with open source software practices in a broad and general way. Not that you aren't right that this thread has moved beyond the original intent of the article (though the OP did so deliberately, he was trying to expand the conversation, not misunderstanding the point); but the attitude that UI and user testing is "crap" is a huge and ongoing problem. Not all commercial software vendors get it right by any means; but FOSS developers, in a broad and obviously not universal generalization, seem particularly hostile to the idea that UI and interface development or testing are important. Here's the thing. Underlying code and algorithms are hugely important. If the software doesn't work, no one wants to use it. Equally so, if no one can figure out the UI, or get the software to do what they want it to, no one wants to use it. "UI crap" is every bit as important as underlying design for success of a project or product. At least if we measure "success" as "anyone but the author cares to ever use the damned thing"

Re:We need more testers / QA as well

[joocemann]

Not if you're EA. EA just puts out unfinished games and treats their happiest early-buyers like beta testers. Just buy any EA product and see what I mean.

Re:We need more testers / QA as well

[Anonymous Coward]

Any type of review is part of the test process. That includes design, requirements, code, etc. Early review can save tons of time and money by catching issues that can be hard to fix after implementation, or worse yet, deployment.

Re:We need more testers / QA as well

[Anonymous Coward]

As for testing - that's a later stage in the process of development.

That's bullshit. You should test early and often. Don't wait until all your code is done to start testing.

Re:We need more testers / QA as well

[marnues]

Way to skew the conversation so that you can preach about programmers who don't take end-user testing seriously. Get a life.

Re:We need more testers / QA as well

[Anonymous Coward]

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.

In my opinion you are wrong, as most probably you've most experience in development not in testing.

The idea of blackbox testing is to not mess with the internals of the system under test. From functional (end-to-end) point of view "correctness" of inserting the data is as good as reading it back in expected way, modifying it without unexpected loss of detail and so on. Even if you make a mistake in the SQL command, eg. format the time using incorrect time zone, from user point of view its irrelevant as long as the time is displayed correctly (or "as it was stored correctly"), printed out with expected data, send to external system and so on.

Changing database contents directly also involves issue related with putting system to a state to which it would never enter by functional ways. Thus testing it in a way it will be never used by the end-user. This is wrong from system test point of view.

Though testing of course is not only system test and black box testing. To catch the bugs you mention you'd need sort of integration and/or unit testing. But this usually you do against design or architecture not against the functional requirements. And for that you usually don't need a qualified tester but a programmer.

Re:We need more testers / QA as well

[arth1]

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.

Re:We need more testers / QA as well

[Z00L00K]

And it can also be very hard to have someone understand the code above the building block level.

It's one thing to have nice steady stable building blocks but if they are combined in a way that looks fine from a review perspective but are completely off the mark from a performance perspective then you have lost anyway.

It's important to get the overall picture right - even if the details look stupid. Some designs may actually have originated for performance reasons, reliability reasons or to avoid deadlocks. It's way too easy to shoot yourself and everyone else in sight in the foot by using an incorrect overall design. And those bugs are usually the worst ones - the system architect failing to understand and consider all factors.

I do know about architectural failures and how easy it is to make them because I have done a few myself. And those are almost never visible during a code review anyway - and often neither during testing. It's only when you get to odd production scenarios that you see them.

Re:We need more testers / QA as well

[ElKry]

teenagers who are pissed at the world.

That's a redundant statement. You could just say teenagers.

That's a redundant statement. You could just say "That's a redundant statement."

Re:We need more testers / QA as well

[mattack2]

automatic testing is a joke, it just test something you already prove works in the automatic setting

Maybe you should look up the word "regression".

Re:We need more testers / QA as well

[Shoe Puppet]

ditto.

Re:We need more testers / QA as well

[Anonymous Coward]

That is not a redundant statement, unless a redundant statement redundates the following statements. And it shouldn't, because new insights and ideas have clearly come to light. Of course, without the first redundant statement, there would be no following non-redundant statements, making the initial statement non-redundant, though in an off topic context. Now the following statements are rendered false or offtopic, through no fault of their own. So what is this? Ceci n'est pas une statement. Help! I'm stuck in a loop factory!

Re:We need more testers / QA as well

[shutdown -p now]

If you are the only developer on the team who can understand the added code, that exposes the flaw in your development methodology. Either you're submitting a single change that is so large that it cannot be easily comprehended even by someone familiar with the existing code that it modifies - in which case it should really be split into chunks and submitted (and code reviewed) incrementally. Or else you are the only developer on the team who is familiar with the code you're changing, which makes you a single point of failure.

So, enforcing a consistent code review policy ends up also enforcing good architectural patterns, and spreading code ownership around the team so that there are no code areas for which only a single person has good understanding. Both are desirable.

Re:We need more testers / QA as well

[shutdown -p now]

Code review is not intended to catch high-level design issues - that's what design reviews are for. But that's not an argument against having CR, it just means that it shouldn't be the only QA step. It doesn't substitute for design and spec reviews, unit tests, or auto tests.

Re:We need more testers / QA as well

[Tsingi]

LOL!

Re:We need more testers / QA as well

[MoriT]

Someone will have to do that eventually anyway. Whether it is before that code is released or when something breaks while that coder is on vacation, it is more efficient to have multiple people aware of the code and familiar enough to give good feedback. The right answer is to just do it. My PHBes don't notice the coders who spend half their time reading slashdot, why should they notice code reviews?

Re:We need more testers / QA as well

[Your.Master]

"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."

I don't see why that would be true.

First off, if it's for a bugfix, you're completely glossing over the time spent identifying the root cause, design, & best fix. You're also forgetting the first-stage testing that the coder did themselves.

Second, if the code is well-designed and structured (and doesn't have a damn good reason to be obfuscated, eg. some hand-optimized assembly sequence), then it should be much easier to read than it ever was to write.

A book tends to take much longer to write than it does to read, even if you're the editor. A television show takes longer to film than to watch and critique.

Re:We need more testers / QA as well

[arth1]

Being able to read the code doesn't impart the knowledge of why something was done a specific way, or whether it wasn't but should have been. You have to be familiar with the codebase, or the commenting has to be so extensive that it (again) takes twice as long to code.

Else, what you're getting is not much more than what the compiler can tell you. And this is my gripe with how "code reviews" are often abused -- instead of actually reviewing the code, it turns into quick eyeballing and someone saying OK (much like unit tests that are written to be passed, not to fail the code).
Code reviews are invaluable, but are too seldom done as more than a rubberstamping process.

As for your analogy with books, you must not have done a peer review of a book. It's a lot of work.

Pure Arrogance

[Anonymous Coward]

Anyone who is anti-code review is an arrogant snot. No one is perfect, and we all have the capacity to overlook things.

Re:Pure Arrogance

[GigaHurtsMyRobot]

What if the people who would be reviewing your code are 'best-practice' consultant types? The only reason I've ever been opposed to code reviews is that development practices are very very subjective and I want my code to be judged on it's output / functionality instead of how it is written.

I realize a lot of people don't agree that it is very subjective, and I think that's the problem... It's like arguing with those who think climate science is 'settled'.

Re:Pure Arrogance

[Anonymous Coward]

You sound like the type who doesn't understand the value of company-wide coding standards. If I was your employer, I'd fire you for that kind of shit attitude.

Re:Pure Arrogance

[JoeTalbott]

The attitude that the ends justify the means in software engineering is a problem. Software isn't a write once and forget it product nor is usually written by one person. The more people that need to work on a piece of code the more that code needs to adhere to the groups coding standards. I would highly suggest that everyone who writes code treat it as if the code is also a part of the product. Avoid shortcuts, be neat, be concise, and look at it as if you weren't the one who wrote it.

Re:Pure Arrogance

[GigaHurtsMyRobot]

You're right... Over my 13 years of professional experience as a developer in companies ranging from very small to very large, I've never come to appreciate something as ridiculous as 'company-wide' ANYTHING. You don't hire me for my ability to conform! As for my attitude, it kicks ass... I believe anything is possible, and I'm highly motivated to solve problems that others think are difficult. I respect your different opinion and would choose to not work for you.

Re:Pure Arrogance

[GigaHurtsMyRobot]

I contend that you can do all of those things without someone looking over your shoulder... Just like you can be a good and moral person without being god-fearing. (sorry to make another analogy, especially a religious one, but this one is a personal favorite.)

Re:Pure Arrogance

[Nursie]

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:Pure Arrogance

[Reality Master 101]

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 written it is. That's called the respect of your peers. And yes, I get that all the time. (actually, in one case, it was kind of embarrassing because this geeky girl kept following me around with big puppy dog eyes. But that's another story).

Re:Pure Arrogance

[Lunix Nutcase]

I contend that you can do all of those things without someone looking over your shoulder

Because humans never err, right?

Re:Pure Arrogance

[GigaHurtsMyRobot]

I don't have a serious opposition to them, and Pair Programming is one of my favorite things. I will however defend certain things, or oppose certain changes. I am passionate about development. I was just explaining why I have opposed them at times in the past, since the OP tried to generalize.

Re:Pure Arrogance

[GigaHurtsMyRobot]

I can't figure out why you jump to the conclusion that my code is 'crappy'. Cool story though, bro.

Re:Pure Arrogance

[luke923]

I've found that alot of devs who are anti-review aren't arrogant, but rather insecure.

Re:Pure Arrogance

[gumbi west]

Software isn't a write once and forget it product

Obviously you haven't met perl.

When I brought this objection to my local perl guru he pointed out that for him it was often easier to rewrite perl code from scratch than debug old perl code. aka, it's a feature!

Re:Pure Arrogance

[Reality Master 101]

I can't figure out why you jump to the conclusion that my code is 'crappy'.

Because you don't want people to judge "how it is written." Why would you not desire this if it was well written code that you knew could past muster? There's no such thing as clean code that someone doesn't like just because it's a subjective judgment. Crappy code is crappy code, and clean, well documented code is clean, well-documented code. I'd bet that if I looked at your code, I would find nary a comment. I can smell your attitude a mile away ("Comments just slow me down, I have too much to do, if they can't understand my code without comments, they shouldn't be programmers, blah blah blah"). Unfortunately, it's all too common.

Re:Pure Arrogance

[DragonWriter]

The attitude that the ends justify the means in software engineering is a problem.

No, its not; the only possible justification for the costs incurred by the means used are the benefits provided by the ends acheived.

Software isn't a write once and forget it product nor is usually written by one person.

True (well, often true, but actually there is plenty of software that actually is "write once and forget it"), but the error of evaluating software that isn't write-once-and-be-done as if it was write-once-and-be-done isn't that there is something in general wrong with the view that the means taken are to be justified by the ends acheived, but instead failure to accurately consider the actual value of the ends acheived and costs of the means used. Its the specific evaluation that is wrong, not some general principle of justification.

The more people that need to work on a piece of code the more that code needs to adhere to the groups coding standards.

True, but also note that the more people that need to work on a single code module, excluding the number resulting simply from the length of time the software is in service and turnover in assignments, the more likely that there is a more fundamental problem with the development methodology than people not adhering to coding standards. Having more than one person responsible for the same piece of code is as much of a problem as having more than one piece of code responsible for the same external resource.

Re:Pure Arrogance

[MadKeithV]

Colleague of mine used to have the following sig: "Write code as if the person who will be maintaining it is a violent psychopath who knows where you live."

Re:Pure Arrogance

[calzones]

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 valuable if they were totally blind an anonymous as a means for each coder to gain insight into what others think of his/her code without knowing who is doing the critiquing, and without management ever knowing about any aspect of it.

Re:Pure Arrogance

[DrgnDancer]

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

[HaZardman27]

How would you rate your code in terms of ease of maintenance? I'm not trying to say what you do is wrong, but I was under the assumption that many best-practice methods were there for maintenance purposes more than achieving functionality. In my shop we do code reviews mainly so other developers know what you did, so if a problem pops up they're familiar with the changes you made. This also helps spread knowledge of the system around.

Re:Pure Arrogance

[HaZardman27]

I'd bet that if I looked at your code, I would find nary a comment.

I call that "job security code," as in, "if nobody understands my code, nobody will fire me," and I find it fairly obnoxious. Obviously there are some things which are so basic that they don't need to be commented, or are self-documented through meaningful naming-conventions, but if I stumble on a 500+ line spaghetti-mess of a function with no documentation, I see red. It's not that I can't parse through the code and figure out what's going on, it's the fact that nobody should have to spend that long just to figure out what's happening in one function so they can trace an error.

Re:Pure Arrogance

[HaZardman27]

Oh, that's just genius. Why correct mistakes in otherwise functioning code when you can just increase the risk of introducing new errors?

Re:Pure Arrogance

[Reality Master 101]

Yes, and the "how does this do what it does" comments aren't even the most valuable. Real pros describe what a section code of does in context of the rest of the code and why it's there, and why it wasn't done in a different way. How the code affects things in other parts of the code. In other words, the programmer's complete thoughts should be in the comments.

Re:Pure Arrogance

[mad.frog]

Code review is purposefully a politically loaded process which enables management to divide and conquer and keep wages down.

You must have an awfully dysfunctional management. On my current team, code reviews came about by demand from the engineers, not from management.

Re: anonymous code reviews, meh, that could be made to work, I guess, but I don't see the point -- I'd rather have a discussion in the open.

Re:Pure Arrogance

[Reality Master 101]

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. :)

Re:Pure Arrogance

[Surt]

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:Pure Arrogance

[ralphdaugherty]

If programmers are so stupid that looking at their code elicits comments that they need to write code correctly, then they need more than a code review. They need supervision.

Other than that, you people have way more time on your hands than I do.

  rd

Re:Pure Arrogance

[imp]

This is pure arrogance. You aren't *THAT* good. Code is more than inputs and outputs. You should be judged on how you do it. It has been my near universal experience in the last 30 years of reading/reviewing code that the people most opposed to code reviews tend to be the producers of the worst, hardest to maintain code in the tree.

Re:Pure Arrogance

[GigaHurtsMyRobot]

Hah... well I'm glad someone sees another angle on this besides good coder / crappy coder. Yes, it is also a process management uses to minimize the importance of any one developer while trumping up their own, eliminate anything resembling pride or ownership of ones work, and reduce any sort of hardships due to turnover.

Re:Pure Arrogance

[billcopc]

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 about covering each other's ass. When dealing with an asshole reviewer, the solution is not to scrap the code review process, rather scrap the bad colleague. If the person reviewing your work starts acting like a holier-than-thou passive-aggressive jerkoff, I say whack them in the teeth with your Model M!

Re:Pure Arrogance

[billcopc]

If you were anyone's employer, you wouldn't be posting as AC.

Re:Pure Arrogance

[Javagator]

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 reviews just were not cost effective for our project.

Re:Pure Arrogance

[GigaHurtsMyRobot]

Like I said, I think it's a very subjective thing. I had to jump in and help fix an issue for someone who was a major 'best-practice' junkie (he was on vacation)... His process was built using wonderful design patterns and everything was made generic and abstract to an extreme level. I thought he was great until this, because it was one of the most tedious things I've ever done, debugging his shit line by line. I get the impression if you had seen it you'd think it was great code, which is what I expected of it to.

Re:Pure Arrogance

[GigaHurtsMyRobot]

Oh no... did I say something to make you think this??? *teeth-gnashing*

;)

Re:Pure Arrogance

[GigaHurtsMyRobot]

Hahaha, well said Billco. I'm all about code reviews if that's the point of them.

Re:Pure Arrogance

[Progman3K]

I performed code reviews at one place where I worked.

I was assigned to do it because I was perceived as having the most experience.

I always did it in the following manner:
  - I tried to understand what the programmer was trying to accomplish
- I looked for bugs or defects that might have been missed
- I never tried to wield 'standards' at anyone

I'd sometimes make suggestions if it was warranted but mostly it was a question of mentally running the code and making sure the output matched the requirement.

You've never met such an un-arrogant type as me, who was universally loved by the team and always confided in by everyone. I saw my job as making sure the mates were happy because I always had faith they were competent.

Re:Pure Arrogance

[Entrope]

You're almost right -- but please save me from stream-of-consciousness comments. I have seen comments where I think the programmer's complete thoughts *were* in the comments, but they were not helpful because there were related assumptions that weren't part of the conscious thought, and they were hard to follow because the programmer had to exert effort to communicate clearly. (Not because of a language issue or anything like that; this programmer just didn't have much practice at explaining things in writing.)

Good comments convey the things you mentioned to a future maintainer, tester or reviewer.

Re:Pure Arrogance

[GigaHurtsMyRobot]

Sounds good man, a very smart and productive/positive way of doing it, while still accomplishing the desired outcome. I'd work with you.

Re:Pure Arrogance

[Anonymous Coward]

That's called anti-social behavior. It benefits you to the detriment of everyone else who is, presumably, forced to work with you. That makes you a selfish, egotistical, dipshit. The other anonymous poster was right, you should be fired frequently until you decide to pull your head from your ass. Sadly, our industry embraces prima donnas like you rather than treating you like an adult, to the detriment of the entire industry. It makes good people and programmers, like me, needlessly suffer and frequently regret working in this field.

I bet the world permanently smells like shit to you.

Re:Pure Arrogance

[SgtPepperKSU]

Yes, you were opposing certain code review comments, not opposing having code reviews. Which is what the conversation is about.

Of course you don't have to blindly make every change that comes up in a code review. That's almost as bad as having no code review. You have a conversation with the person who made the comments and provide a compelling argument on why they should complete the code review without those changes being made.

Re:Pure Arrogance

[gorzek]

I wholeheartedly agree with this but I think it deserves some clarification.

Code review isn't and shouldn't be about judging someone's coding style. It is certainly worthwhile to have some company- (or at least project-)wide standards regarding things like interfaces, documentation, and the general structure of your programs, but at a line-by-line level there's no reason to quibble over exactly how someone wrote a specific piece of code unless there is an objectively better way to do it, or there is actually something wrong with it.

I think some developers object to code reviews on the basis that it implies a lack of trust and that it will result in them being told how to code. That is not what it's for. We all make mistakes, and sometimes someone else will know a faster or shorter way to do something, and everyone's code benefits when this knowledge is shared. I have both learned a lot and taught a lot by doing code reviews.

None of this excuses code that is written in a way that's difficult for others to understand or maintain, or code that is not documented.

Even if your code already works, it's very likely someone who reviews your code will know some way you can improve it--to make it faster, more concise, or easier to maintain. It has the added benefit of letting someone else understand your code. There is really no good reason not to do it--the quality implications alone make it worth the cost.

Re:Pure Arrogance

[gorzek]

I agree with you again. :)

I deal with legacy code that's 20+ years old, and the most frustrating aspect of it is the lack of documentation. While I can easily figure out what the code is doing, why it's doing it is much less obvious--and without comments to give me some clue as to what this code was meant to accomplish, no original requirements to refer back to, I have no idea if a given module is doing what it should. I can always explain exactly what it does, but that's only half the work. Putting comments in your code doesn't take that long, and it not only ensures you can refresh your memory next time you have to look at it ("Why did I do it this way? Oh yeah!") it also means someone else can maintain your code later, knowing not just what it does but what you intended it to do, and why.

Coders who take a "job security" approach by not documenting are just assholes and frankly they should be fired if they won't document. It's part of the job, take it or leave it.

Re:Pure Arrogance

[Anonymous Coward]

It all depends on how expensive errors are.

More often than not, the bosses would rather have crap launched than not have anything. It's normally easier for the boss to sell crap than it is to sell nothing.

So no surprise if there's no time nor budget for code reviews. The few programmers in the project are all busy adding features and fixing obvious bugs.

Re:Pure Arrogance

[pthisis]

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 reviews just were not cost effective for our project.

That's been my experience as well. We did regular code reviews for a while, but found we got more bang for the buck dedicating that time to pair programming and more developer time writing regression tests. It's not that code reviews were completely ineffectual, they're just (for most things) a very low return on the amount of time invested compared to alternatives.

So now they're optional--occasionally when there's a particularly hairy piece of code or something the author will call for a review, and usually that makes them higher payoff.

Re:Pure Arrogance

[GigaHurtsMyRobot]

I think this is a ridiculous path for the argument to take. I've inherited hundreds of programs, and a majority of them were shitty in my opinion. I did what I had to do, figured them out and maintained them, or made a case to re-engineer them. I never categorically stated that these people should be fired, or that their heads are in their asses. Your whole post is so generic, slanderous and whiny, it doesn't even make sense.

What is this nonsense about the world smelling like shit? I don't get it... I am an extremely positive and helpful person. My StackOverflow profile contains less than 5 questions and over 600 answers.

Re:Pure Arrogance

[Sectoid_Dev]

I've written Perl for the past 15 years. True, it can be a difficult language to read if some asshat insists on coding using the fewest keystrokes possible. Other than that, it is perfectly readable if you are willing to take the time to metally parse the code and have a reasonable amount of comments that describe the 'why' of the program.

But I am utterly amazed at how few places I have worked that actually do have code reviews. Granted most of the time they catch surface errors, but the real benefit of them is to spread out the knowledge among the team. Not just the knowledge of the code, but everything else around the code; coding is easy, but understanding the technical infrastructure and business processes around them are the pain. The places without code reviews tended to have developers siloed to their programs and nobody knows nothing outside their own little domain.

Re:Pure Arrogance

[AJH16]

There is a difference between design review and code review. With good experienced coders that are not lazy, code reviews offer little in the way of finding bugs or ensuring that code is readable as the coder will have already written the code read-ably. It is a waste of time and limits the agility in responding to business needs. That said, some developers may benefit from code reviews if they are still learning the system, may make obvious mistakes or are learning the technology. In this case it could be beneficial.

That is very different from design reviews where you review the overall direction of code and make sure that it fits well with the overall system and what other developers are doing. This is very valuable and critical as long as not overdone. If it starts being a large percentage of time, then it is probably time for a software architect to come in and help with managing the big picture and making sure developers stay in tune with it.

Re:Pure Arrogance

[Sectoid_Dev]

Wow, I think you need to switch to a different company. A lot of the positions I've worked, management had no idea about coding standards or code reviews, which is a whole different problem.

Re:Pure Arrogance

[cheekyjohnson]

Because you don't want people to judge "how it is written."

That does not mean that his code is "crappy" (which is subjective). There could be many other reasons that he does not want this happening. I'm not saying I think that code reviews are bad. I'm saying that it is unlikely that you know whether his code is "crappy" or not merely because he doesn't want it reviewed.

There's no such thing as clean code that someone doesn't like just because it's a subjective judgment.

It's subjective. So is "clean" in this instance. It is possible that someone could hate code that you deem "clean."

Re:Pure Arrogance

[curunir]

Moreover, resisting code reviews is directly overlooking something very important, which just proves what you're saying that anyone can overlook something.

Let's assume, for the sake of argument, that the person resisting code reviews is that mythically-rare example of someone who doesn't overlook things and doesn't make mistakes. If that's the case, why would you waste the opportunity to have other mere-mortal members of the team learn from reviewing the divinely perfect code that this individual produces? By questioning why certain decisions were made when writing the code, less experienced coders can learn to improve their own code. To deny them that opportunity would be entirely selfish and counterproductive to the goals of the team.

Basically, code reviews should have multiple goals. In addition to improving the code being reviewed, they should improve the code that hasn't yet been written by improving the coders who will be writing it. To only consider the first benefit is being incredibly shortsighted.

Re:Pure Arrogance

[Savantissimo]

Write code as if the person who will be maintaining it is a violent psychopath who knows where you live."
Should be modded insightful. Consider the already precarious mental stability of someone choosing to work where you do. Then consider the potential destabilizing effects of maintaining some of your coworkers' code. Apply a bit of introspection, and you might well start thinking about how to locate the guilty and fantasizing about the most appropriate fate for that idiot who committed that 500-case switch statement...

Re:Pure Arrogance

[Anonymous Coward]

Anonymous, lol. I can take most any piece of code developed here, and be able to tell you with 95% certainty who wrote it.

Unfortunately, the few times that I've been in a code review, the guy reviewing my code was a manager. He couldn't have coded it himself if he tried, and he wasn't very good. The few things he picked out were quite frankly just plain wrong, and it got to the point I told him I wasn't going to change it, and he could leave it as is, or fire me. Then again, I've been programming for 29 years, and he went from a manager at Denny's to an IT manager 3 years before then.

Re:Pure Arrogance

[PingPongBoy]

If programmers are so stupid that looking at their code elicits comments that they need to write code correctly, then they need more than a code review. They need supervision.

Other than that, you people have way more time on your hands than I do.

  rd

Now there's a segue. One of the learning techniques in machine learning is called "supervised learning" where the machine is trained on a specific data set, and the desired machine action is known in advance. The idea is to be able to tell if the machine is learning correctly.

The code review, in my opinion, is a method to teach certain aspects of writing code. The ultimate purpose of the code is to satisfy the requirements for the code, but this achievement can be made in many bad ways. The code reviewer might not know better than the code reviewee, but if the coder feels there is an opportunity to learn better coding, then the coder may feel less resistant to a code review.

Sometimes coders are under pressure to rush their work. The code is likely not the best, and a code review would not add value unless the reviewer can provide instruction on how to produce a quantum improvement jump under the same circumstances.

Re:Pure Arrogance

[aminorex]

Arrogance is irrelevant to business efficiency. I had all my code reviewed for the past 3 years. 98% of the comments were vapid style conformance, whitespace issues. 1% were just stupid. 1% were useful. The cost-effectiveness of mandatory code-review is very poor. If the same time were spent on automating tests, it would have added real, lasting, incremental value to the project.

Re:Pure Arrogance

[sartin]

Code review is purposefully a politically loaded process which enables management to divide and conquer and keep wages down.

Perhaps there is another thing, also called a "code review", that is different from the dysfunctional mess you appear to have experienced. I've worked in organizations that did code reviews right. They were peer reviews, conducted professionally and without ego. It was no triumph to find a defect in someone else's code and no defeat to have one found in yours. We reviewed for real issues, not compliance with standards on indentation (which can be done by a program) or naming (though we would comment on names that didn't communicate well). Metrics were recorded and reported in aggregate; they were not used to try to find "bad" team members.

We found real design flaws. On one project, design reviews caught a major global timing issue that could have resulted in a patient's heart stopping and no alarm sounding at the nurses' station. The fix was easy given we found the problem early, but would have been much harder if discovered later in testing, or disastrous if discovered in the field.

We all learned from code reviews. We gained shared knowledge of the code base - on one project we had enough shared knowledge that the death of a team member didn't derail the project completely.

It's sad that these two very different things share the name "code review", but such is life.

Re:Pure Arrogance

[shutdown -p now]

It depends on how big your project is, really. When you have numerous components with complicated API layers between them (the result of 15+ years of slow code evolution with occasional component-level rewrites striving to preserve back-compat), it can be immensely helpful to have someone with a lot of experience with the code base having look at your changes. They can point out non-obvious corner cases in your use of APIs, spot potential performance issues, and so on.

Of course, when you have too much of that, it's also a good indication that your code is starting to rot, and it needs some major refactoring and cleanup.

Re:Pure Arrogance

[Hal_Porter]

Don't be culturally insensitive

http://gradha.sdf-eu.org/textos/klingon_programmer.en.html

Re:Pure Arrogance

[Hal_Porter]

You're,uh, still putting the old covers on your TPS reports.

Re:Pure Arrogance

[Hal_Porter]

If you truly believed in the strength of your code you'd be glad to defend it by single combat in the code review arena.

Two coders enter! One coder leaves!

Re:Pure Arrogance

[EmperorOfCanada]

Code reviews can be a great opportunity to grow but not when:
The reviewers are MSDN programmers who disagree with your choice of Linux and thus wouldn't rate theoretically perfect code better than a 1 out of 10.
The reviewers are Java programmers who disagree with your choice of C++ and while unable to understand C++ will rate your code a 2 out of 10 because it isn't Java.
The reviewers are crappy programmers who you diligently kept off your project because they suck but someone wants them doing billable hours so poof they get on your project through a back door. Then they start making changes to your code while reviewing it because their testing manager gave them write access to your code repository.
The review process would literally take 10 person hours and generate dozens of pages of paper work to review Hello World.
The 58 year old PhD in CS doesn't like your brace style and won't sign off until you change it.
The testing/QA department who had been successfully kept away from a project that is on version 3 with almost zero and only minor bugs in 6 years creates a code review process that finds "Serious Shortcomings" with your code that requires that they get 1/3rd of your development budget for version 4 before this project becomes an "embarrassment" to the company and results in your entire coding team working for a competitor in under two months then resulting in your old company being sued for millions a two years later when the promised version 4 is delivered late and broken.(true story)

Re:Pure Arrogance

[dragonturtle69]

MOD UP!

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).

And that is how it should be.

Let QA bring in management. :>)

Re:Pure Arrogance

[GigaHurtsMyRobot]

Yikes!!!! I love the beginning of the last example, "The testing/QA department who had been successfully kept away from a project." Classic. That is a sad story, what happened next. I'm not surprised though!

I agree 100%

[jlechem]

I have worked in places with and without code reviews. Yes code reviews can be painful especially if you work with some douchebags. But overall getting new eyeballs looking at your code often brings up things you hadn't thought about, etc. It doesn't make you or the other people bad, it's just how it is. Drop the ego and take some constructive feedback.

Re:I agree 100%

[flooey]

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:I agree 100%

[jlechem]

Exactly the code review itself isn't bad, it's the personal interaction between coworkers that has always been the issue from what I've seen.

Re:I agree 100%

[Anonymous Coward]

Ditto. Code reviews are amazingly productive for a number of reasons:

1) improve code quality, this can sometimes be a new approach suggestion for the code itself but can also be a sharing of knowledge and techniques used in the code to others. The learning/improvement can go both ways.

2) code style and approach consistency is improved. everyone gets on the same sheet and the code becomes more uniform which improves maintenance.

3) validation of code maintainability. If it can't be reviewed effectively then it sure as hell won't be maintainable.

4) all the obvious bugs, assumptions validation, and error handling validation people know about.

Re:I agree 100%

[gl4ss]

it does make the douchebags look bad and they'll later know that, so it's good. instead of code reviews though you should have code sharing among the developers most of the time, gets new guys up to speed faster as they can't bring their old libraries full of code snippets with them usually - and saves time when doing by the book sucks because the book was written in the same way as the strategy guide for duke nukem forever that came out a decade ago(and that's like 95% of the books for emerging platforms where the book was written before the api's shipped).

Re:I agree 100%

[Anonymous Coward]

How do you propose to eliminate douchebags from the workplace?

Re:I agree 100%

[Tanktalus]

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%

[ShanghaiBill]

How do you propose to eliminate douchebags from the workplace?

Where I work, we use peer reviews. Every six months, we rank our co-workers. Those listed at the bottom by nearly everyone either get canned or take the hint and start looking for employment elsewhere.

Re:I agree 100%

[TheRaven64]

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.

Re:I agree 100%

[d0nju4n]

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.

Exactly. When I was the "new guy" at my place of employment, "reviewing" the code of other, more experienced people was very beneficial for me. Not only did it expose me to areas of a very large, complex codebase that I wouldn't have otherwise interacted with, but seeing other (also more experienced) people's comments helped me to understand how many modules interact (or, are supposed to interact) with each other. For the curious, I'm talking about hard drive firmware. I put reviewing in quotes above because, when I was new, I wasn't able to contribute anything substantial to the review process (we're not talking about c++ syntax issues here).

Re:I agree 100%

[Anonymous Coward]

I think it all depends on the way the reviews are conducted... I have worked in places where the code review is really just a code phrase for "Let's get this guy no matter what" and I have worked in places where code reviews are done in a completely impartial and useful way. I will leave it up to you to guess which one of those produces the best code.

Re:I agree 100%

[Surt]

That system only works if you never exceed 50% douchebags, which can be a big problem at any company experiencing sustained growth.

Re:I agree 100%

[The Moof]

Every six months, we rank our co-workers. Those listed at the bottom by nearly everyone either get canned or take the hint and start looking for employment elsewhere.

That's an excellent way to turn your workplace dynamic into a "Good Ol' Boys" environment, regardless if it's good or bad for the business. It's akin to nepotism, you're just not related.

Re:I agree 100%

[Anonymous Coward]

I've never worked in a place where there were code reviews but the new role has it and it really does help. code reviews with test driven development are very critical and they for sure help your coding skills and writing more memory efficient code.

Re:I agree 100%

[Anonymous Coward]

well ego is bad and it is'nt too. if it were constructive criticism then having an ego does not help. but with regards to code reviews if the reviewer's standard is rubbish, i surely quit. there's some folks you meet that think noone can ever write better code than them. they're a disgrace imho. they need to keep their review to themselves. After all we're all out to learn something new everytime. I've started off as a java dev then moved onto c++/sybase/oracle and finally into perl. i miss c++ tho, but i'm starting to find that, the code reviews in perl can get your mind in a terrible state with the exception of Moose.

Re:I agree 100%

[billcopc]

Quicklime.

Re:I agree 100%

[Anonymous Coward]

OR you work with someone who *HAS* to say something about everything. Even though it is just fine. 5 other people looked at it and said 'looks good' but the one guy always has to say something. He wants to put his mark on everything. Otherwise he isnt doing anything...

Code reviews just bring out the worst for some reason. Many times when you have people like that they degenerate into style reviews. Which just makes everyone dread the code review. As they have to listen to the jerk...

Get one or two jerks/showoffs and code reviews are painful.

At work 90% of interpersonal issues at work can be taken care of by biting your bottom lip and saying 'what ever'. But with code reviews some people use them to push an agenda or bully others. Then for some reason *EVERY* comment must be acted upon. Even though there may be no issue.

Code reviews are totally great. Dealing with the BS that they drag along not so great.

Re:I agree 100%

[fahrbot-bot]

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.

Please don't confuse the purpose of a code review with teaching or, worse, coding to the lowest common denominator. Obviously, if something is coded in a really obscure fashion, it should be checked to see if that's appropriate and, if so, that it's commented well.

Re:I agree 100%

[recharged95]

That's why I love tools like TracPeerReview.

Code Reviews are either a learning experience (for the inexperience code and specific technology), or an ego competition, i.e. opinion party. Otherwise they are useless, especially when releases are within months.

Tools like TracPeerReview allow once to upload their opinions and actually review offline, which the coder can take action or ignore, without wasting tie sitting in a meeting. And they are historically stored/time stamped.

You know, you're only suppose to have meetings when it's really needed. And you don't need a meeting for a code review.

Re:I agree 100%

[Darinbob]

Code reviews should not be about giving a grade to the coder or judging the quality of the code. This attitude I think is what makes some people hate the reviews. Instead there should be a lot of knowledge transfer going on here. The reviewer can be learning stuff as well, such as the new APIs that were just added or thinking about ways they can interface with the code they're reviewing. Sometimes the feedback may be "we already have that routine in a library, you don't need to reimplement it" or "how will this work with the new device driver model that's coming out next release?" There should be no ego pain involved. If there are some douchebags then over time they'll be excluded from reviewing code if the manager is on the ball.

Re:I agree 100%

[steelfood]

Agreed. Discovering your own faults is probably the most difficult thing to do. It's easy to see the faults in other people though.

This inability practically is the definition of an egomaniac. Egomaniacs probably need code reviews the most. They also probably do it the least. The idea that doing a code review is a way to show off one's code is probably the only thing that would convince an egomaniac to do one. Getting them to answer the questions and accept criticism is a different story.

I consider code review the same as peer review in science and mathematics. If it's not checked by other people, it's not good enough to be "right."

Re:I agree 100%

[Hal_Porter]

You could arrange a "little accident" for them.

Re:I agree 100%

[internettoughguy]

When you exceed 50% douchebags, and the douchebags all get along, maybe they're not the douchebags ;).

Re:I agree 100%

[snowgirl]

... or they're really pedantic but not very helpful.

Hey, I thought being pedantic was helpful. :(

I tried to remind people that if I was nitpicking their code, that meant it was actually really good.

Re:I agree 100%

[Anonymous Coward]

Having code reviewed by someone less experienced helps catch things where someone new to the codebase would struggle to understand the code.

Maybe they should read the comments and the documentation.

Other than that, I wasn't aware there was a coding standard called "code must be easy to understand by inexperienced programmer".

There's a point when...

[blahplusplus]

... 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:There's a point when...

[Z00L00K]

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:There's a point when...

[Anonymous Coward]

You have to be kidding. I hope you are. Reviewing code catches /subtle/ and insidious bugs and poor design decisions that a compiler or auto-mated test suite will never catch. As a side effect, revisiting your code with someone else is a /learning/ experience.

Re:There's a point when...

[Anonymous Coward]

But that still doesn't mean it meets requirements, or even if it does, that it does so in a good way.

Re:There's a point when...

[bames53]

I think Z00L00K was saying the time for code reviews is after all the automated stuff already checks out. Obviously you shouldn't waste co-workers' time reviewing code that you already know needs more work.

Re:There's a point when...

[ArsonSmith]

Ohh, and it does what the customer expected...or was that part optional?

Re:There's a point when...

[JoeMerchant]

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:There's a point when...

[Remloc]

As a side effect, revisiting your code with someone else is a /learning/ experience.

Exactly a point I was about to make!!

After over a decade intense C++ programming, I finally had found a real world use for the ".*" and "->*" operators. Soon after the review, my 3 reviewers started using them also and our code improved in re-usability and readability.

Re:There's a point when...

[Anonymous Coward]

if(a=b){ .... ....
}

Compilers does not catch logic errors. For a start.

Re:There's a point when...

[MobyDisk]

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.

Re:There's a point when...

[Anonymous Coward]

Because that's "always" what the client wants to pay for, right? Nope, it's not, most of us are not building satellite control systems, sorry. Build in decent error handling and notification and fix any sticky issues during any maintenance phase.

Re:There's a point when...

[Lunix Nutcase]

Many compilers will throw warnings about that with the proper warning level set or if you are using lint as the GP said it will most definitely issues warnings about that line of code.

Re:There's a point when...

[mini me]

I tried compiling your code and got this:

warning: Semantic Issue: Using the result of an assignment as a condition without parentheses

Re:There's a point when...

[alexandre_ganso]

would care to share it with the world?

Re:There's a point when...

[MadKeithV]

Ohh, and it does what the customer expected...or was that part optional?

What? Fart butterflies, magically deduce what they actually wanted to do and print money at the same time?

Re:There's a point when...

[Anonymous Coward]

I am all for code reviews, but as you mentioned the use code reviews should be done when it makes sense. These issues have created a whole new set of code nazis whose mere existence is based on the absolute mantra that is counter productive.

Those who are fanatical, militant and trying to move up the management ladder (while trying to cover up the lack of knowledge thru a process) makes the argument that it has to be done all the time, while those who understand software engineering take a pick and choose attitude. Unfortunately the managers they are trying to impress buy in to the idea. I have worked in such places and seen how the productivity goes to hell, and the militant proponent eventually moves on to another company with a managerial title, to repeat the process yet again.

Programming is a philosophy, an art, as well as a science and engineering. Trying to lock it in to the absolute ends up in disaster.

Re:There's a point when...

[sconeu]

OK.

Requirement: Perform stock analysis and predict future performance within $ERROR_MARGIN

Code:

#include <stdio.h>
#include <stdlib.h>
int main()
{
  printf("Hello world!\n");
  return EXIT_SUCCESS;
}

This code builds with no warnings using the most paranoid settigns and has a silent lint analysis. But it's completely unacceptable.

Re:There's a point when...

[npsimons]

This code builds with no warnings using the most paranoid settigns and has a silent lint analysis. But it's completely unacceptable.

Funny thing, I can pick two (perfectly valid) warning flags for gcc that no function definition will compile silently with. I won't be a tease and just tell you upfront: -Wtraditional -Wold-style-definition. They aren't mutually exclusive (like they probably should be), at least in gcc 4.4.5. Warnings are a good start and help condition people to better coding habits, but you are right that they are not the end all and be all of code checking.

Re:There's a point when...

[Entrope]

Probably some kind of interpreter or command dispatch system, where the inputs and outputs are regular across a lot of functions. The code to build and dispatch those things is a lot simpler and clearer if you map the command names (or other search or trigger criteria) to pointers-to-member-functions than if you use a switch or if/then chain.

Re:There's a point when...

[Z00L00K]

You got the point.

And there was some irony in the statement too, but irony detectors are scarce.

Re:There's a point when...

[Anonymous Coward]

this statement will still be true if you substitute "code reviews" with anything else

Re:There's a point when...

[Anonymous Coward]

It was an example, one that have struck many times in the past with older compilers. The point still stands though. Compilers does *not* catch logical errors, erroneously implemented use of side-effects and other forms of valid code that does not do what you thought it would. You need a human and brains to catch that. And you're probably going to have a lot harder of a time to catch your own thinkos than some one else will.

Re:There's a point when...

[TangoMargarine]

Is EXIT_SUCCESS defined?

Re:There's a point when...

[Anonymous Coward]

Let me guess...you've never been exposed to big-O notation, have you?

Hint: Things slow down in a hurry when your algorithm has an exponential big-O.

Re:There's a point when...

[Anonymous Coward]

... and you can still understand it when you have to modify it 2 months later... or is that optional too?

Re:There's a point when...

[sconeu]

That's what stdlib.h is for.

Re:There's a point when...

[Darinbob]

That's not the point of a code review. It's not there to tell you if your code is good enough or not.

Re:There's a point when...

[steelfood]

The only time code reviews don't make sense:

* 1-off programs
* single-purpose simple programs
* non-critical, non-production programs (e.g. proof of concepts)

For everything else, code review is critical. People also tend to think minor changes don't need to be reviewed, but that's often untrue unless several developers independently decide on the same change and the change is very, very minor.

Now, the things to say during a code review, questions to ask, mistakes to point out, things to ignore, these are what widom and experience are for.

Re:There's a point when...

[Anonymous Coward]

That is a business, not technical, decision.

I ply my craft with as much skill and professionalism as I can, and I let the sales and marketing creatures make up lies about "undocumented features" and what is "good enough."

A matter of time...

[mdf356]

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:A matter of time...

[pixelpusher220]

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 reviewed' policy. We're not doing sensitive software stuff where absolute precision is required, it's just standard information systems development. Review some representative sample of the work product and move forward. The 'everything' requirement only breeds disinterest in the process itself.

Re:A matter of time...

[Anonymous Coward]

post your code here and we'll review it :)

Re:A matter of time...

[JoeMerchant]

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.

Re:A matter of time...

[mdf356]

post your code here and we'll review it :)

See anything committed by mdf to the FreeBSD svn repository. FreeBSD's review policy is mostly after-commit since all commits generate email to a mailing list, and the community is active in reviewing things that way. (Note that most of my commits for work are not to FreeBSD, so ... yeah.)

Re:A matter of time...

[Surt]

You should think about how to change your work culture. Where I work, helping out a coworker is job #2.
(And job #1 is handling a customer emergency. I last had to help out with job #1 over 4 years ago).

It's a cost/benefit thing

[Anonymous Coward]

When you're in a team of all around good coders, there's very little gained from periodically telling each other that basically everything is OK. Code reviews can lead to modification-pressure: You can't leave a good thing alone because then what would be the point of reviewing? In that case, coders get rightfully annoyed because nothing passes uncriticized. All in all, if you don't have code quality problems, why would you do code reviews?

(An intellectual is someone whose mind watches itself. Intellectual coders don't need external control.)

Re:It's a cost/benefit thing

[NorthWestFLNative]

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 testing. You need all of that (and more) for a good project. Good coders are not everything. Nor should they have an ego about their code. A good coder should realize that everybody makes mistakes, even themselves.

Re:It's a cost/benefit thing

[Anonymous Coward]

A good coder knows when to consult with colleagues, for example when working on unfamiliar code or interactions with someone else's code.

Even though no coder is perfect, there's a tradeoff between uncaught bugs and slowing a coder down for reviews. Bureaucracies aren't just hated because they hurt people's egos but also because bureaucracies usually become quite inefficient over time.

We're far away from the point where we strive for perfect code. Bugs are an accepted aspect of software. It's only the balance between cost and quality that we're interested in anymore. From that point of view, code reviews are not a necessity, but an option. (Perfect code does not need code reviews either, because reviews can't guarantee absence of bugs.)

Re:It's a cost/benefit thing

[dbrueck]

I agree with all of the arguments in favor of code reviews - at least in principle - and yet we never do them at my company for the exact reason you point out. I started a small company, hired a handful of excellent programmers, and we never do code reviews, and it *is* a cost/benefit thing. Yes, in theory we might uncover some bugs doing code reviews, but we just haven't found the cost of doing them worth it.

Based on experiences at previous companies, doing code reviews "right" takes a lot of work - you have to interrupt people from getting actual work done, and ask them to become familiar with a big chunk of code (because if it's too small a chunk of code, it's not worth the time, and if they don't become familiar enough with it to really grasp how it works intuitively, then at best they'll only find the trivial bugs that were already weeded out previously). Further, effective code reviews really need to be interactive and real-time, which means getting everybody's schedules coordinated and in the same room at the same time. Already the code review must provide some pretty amazing benefit to outweigh these costs.

Also, the modification pressure is *very* real - because we're investing all this time, people feel like they need to find something to comment on or fix, and so even though something might not be buggy there is feedback given to change it to improve the style or efficiency or whatever. All of those things are great, but if they aren't currently actual problems, then at best you're wasting time providing no net benefit, and at worst you're risking introducing new bugs by your refactoring.

Code reviews are one of those things that /can/ have benefits, but in practice we've found that the benefits generally don't outweigh the costs. The people advocating them generally overstate the benefits and/or understate the costs IMO. If people want to do them at their company, that's great, but us deciding not to do them is less about arrogance and more about pragmatism.

Re:It's a cost/benefit thing

[dbrueck]

Very much agreed. Yes, good coders make mistakes, but it doesn't imply that code reviews are an effective way to find those mistakes. By the time code is ready for a review (such that asking people to review it isn't wasting their time), it's code that has been written, fairly well debugged, and *used*.

Honestly, the most efficient way to find bugs in code is to use the code. Nothing else that I know of comes close in terms of return on investment and in my experience code reviews, while good in principle, are pretty far down the efficiency scale. It's generally way more effective to tell your team, "hey, I just finished feature X. If you guys can collectively find 3 bugs before lunch, I'm buying!"

Welcome to the industry, Chris.

[xxxJonBoyxxx]

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.

Re:Welcome to the industry, Chris.

[Synerg1y]

I highly doubt xxxJonBoyxxx has written much code in his days, ever seen an assembly stack buddy? OOP is a god send in comparison, all the layers are automated, and the syntax is relative to human language. The time it took to write hello world in 1970 and now in 2011 is a 2 figure hour differential if your just learning.

Re:Welcome to the industry, Chris.

[MonsterTrimble]

Fuck, now I'm going to jones all day for that 1973 Gremlin X I missed out on buying. FML.

Re:Welcome to the industry, Chris.

[Anonymous Coward]

Welcome to the industry, Chris. This might have been the funniest thing I read all week.

Really? Why? Seriously, why is this funny? Okay, let me ask another way - how long have you been programming? I think you have no clue about this topic. I think you are a brand new programmer - or maybe even have never coded - and either way you have zero knowledge of computer architecture, language implementation, and other basic computer science concepts.

Re:Welcome to the industry, Chris.

[Anonymous Coward]

Having seen both the code for the AT&T phone switch (an early user of K&R C, and a clear inspiration of C++) and the code of FireFox, I can state without any hesitation whatsoever: he's absolutely right. It seems modern architects take the idea of abstraction far, far further than is reasonable for the problem domain. You simply cannot decently debug a mixed C++/Javascript/IDL mess, so code reviews are much more important.

(That said, by the time it was Lucent, Bell Labs reviewed 100% of new code. It's far too effective to ignore.)

They're good

[Anonymous Coward]

Because not everyone is Ken Thompson =)

Your Employer or Client Can Argue

[Anonymous Coward]

You can quip that quality pays for itself, but tell that to clients and especially employers. If no one pays us to take the time to do code reviews, no one will. No, it is not just another cost of doing business, at least not to those who write the checks. We have a hard enough time persuading people to do testing still after piles of research. Automated unit tests are a hard sell for a lot of people.

Most clients and employers just want to have programmers spend the least amount of time possible on any given task and move on to a new one, quality be damned. In an ideal situation with consulting, this hopefully billed to the client as "fixes" or a new "version." In the case of products, it takes the form of patches, versions, or never gets fixed at all.

I'm all for TDD, peer programming, code reviews, etc. being someone who's favorite language is Smalltalk (allows true TDD IMO - coding TDD inside the debugger is amazing). The reality is people don't always do the right thing, such as picking the best language for the task, let alone allowing you to do something that would arguably save them money in the end. Code like hell, profits be damned.

Welcome to the real world. If you believe otherwise, you will have a rude awakening. I know of firms or places I've worked that do code reviews and the related TDD, extreme programming, etc. These places seem great, but usually they don't last, especially if your competitors are seemingly pumping out things faster, bugs or not.

Re:Your Employer or Client Can Argue

[sosume]

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.

I'm not too good for code reviews

[jeff4747]

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.

Re:I'm not too good for code reviews

[CapuchinSeven]

Which is pretty much why I gave up on computer science and am moving into Geology instead. True story.

Re:I'm not too good for code reviews

[Anonymous Coward]

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 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.

Re:I'm not too good for code reviews

[OldTroll]

This is a BS mindset. It is the equivalent of an automobile manufacturer saying I don't have time to tighten the bolts on the car properly. Yes, it's probably good enough to roll out the door, but the flaws will start showing through soon -- perhaps catastrophically. I think it's our job as developers to push back against those crushing deadline and own up to some professional pride. Stop being bossed around. There is a difference between a job and a profession, the person doing the work.

Re:I'm not too good for code reviews

[Anonymous Coward]

"When was the last time you sharpened your axe?"

Re:I'm not too good for code reviews

[Co0Ps]

Wow, your think programmers should be blamed when management fails at their responsibilities (planning). I'm not even going to bother explaining why that's insane since your name is "OldTroll".

Re:I'm not too good for code reviews

[JoeMerchant]

>

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:I'm not too good for code reviews

[codegen]

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

[OldTroll]

See, the funny thing is, I do tell my manager how fast I can go. It's part of my job to let my manager know how much I can accomplish. And I'm not just talking about my current one, but every manager I've had in the past 15 years. As in I'm a professional and I take my work seriously, which means I expend some effort to ensure that I can do the best work I can. It also means I expect and receive proper equipment and applications to allow me to do my work efficiently. Sometimes the company provides those resources and sometimes I do, but you can't build good software by slapping crap together at a thousand miles an hour. Or rather you can. For about fifteen minutes.

Re:I'm not too good for code reviews

[mcmonkey]

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:I'm not too good for code reviews

[hercubus]

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 overpriced employees who are a constant drain on profit; they must be minimalized or eliminated somehow, that's the mindset. Sure I'm generalizing but not many of us get to work for enlightened management, most of us have a mortgage to pay and when the person who can easily replace us asks us to adopt a particular goal, we do.

Re:I'm not too good for code reviews

[alexo]

Which is pretty much why I gave up on computer science and am moving into Geology instead.

I heard that Geological processes have pretty relaxed schedules.

Re:I'm not too good for code reviews

[Oligonicella]

"I'm too busy" has been used for decades and it has always been a failed argument. Code review typically saves time, not costs it. A shop that runs frantic will *always* run frantic. Also a good sign you're not building libraries sufficiently.

Re:I'm not too good for code reviews

[Anonymous Coward]

I agree with this, with a proviso - if there's not a code review, there needs to be automated unit testing. And by that I don't mean 100% coverage, all perfectly non-brittle, running in a super-slick build environment. The point is to have SOMETHING more organized in your process than "Well, I ran it one time, and it seemed to work." You can do that with organized unit tests, you can do it in a formal QA process with external testers, you can do it with code review. Code Review is just one tool in the toolbox.

I tend to work on very small teams, one to three developers, who are extremely advanced. In this environment, I find unit testing vastly more important than code review. If, at the end of the day, you've got time left over for code review, then maybe, but, like the parent poster said, the software only has to be 'good enough'.

Re:I'm not too good for code reviews

[Rogerborg]

I'm too busy because I don't do code reviews.

Fixed that for you. You can be a whiny bitch and blame everything on management, or you can explain to them that you can't row because you're too busy bailing, and maybe taking the time to stick a few corks in the boat now and again would help them to earn their bonuses.

Re:I'm not too good for code reviews

[interval1066]

Development schedules are almost always ridiculously compressed.

In my experience, which has been repeated over and over again, everyone thinks code reviews are a good idea, but every attempt to put it into practice goes nowhere. Everyone pays lip service to Agile programming, but no one actually does it.

Re:I'm not too good for code reviews

[__aanhjr1420]

The problem is that there's no suitable output from a code review. Other tasks are definable, e.g., delivering code that meets the requirements. A code review doesn't work like that. It's a nice thing to do for your peers to sufficiently examine their code in a review. But nobody gets kicked for doing a half-assed review. They do get kicked for missing a delivery. So when rubber meets the road, a thorough code review tends not to happen.

Re:I'm not too good for code reviews

[Omnifarious]

Sure I'm generalizing but not many of us get to work for enlightened management, most of us have a mortgage to pay and when the person who can easily replace us asks us to adopt a particular goal, we do.

And each and every single person who buys into this attitude is part of the problem.

Re:I'm not too good for code reviews

[istartedi]

This is why you shouldn't generalize too much about software. For some fremium web app, a little glitch here and there is fine. Getting first to market is more important than perfection. For mission-critical software you want perfection. That would include medical devices, transit systems, and aerospace.

What's important is for people to recognize the difference between Twitter and Trains.

Re:I'm not too good for code reviews

[keithpreston]

"Never enough time to do it right, but always time to do it over."

I feel sorry for anyone who works at places like this and I've worked at places like this before. Compressed schedules, vague changing demands, schedule slips when the hacked up mess finally falls apart. There are better ways to make software, but we just keep promoting good software engineers to managers that have no idea about management.

Re:I'm not too good for code reviews

[DriedClexler]

So, not enough time to do it right the first time, but plenty of time to do it a second time when the bugs make it unusuable? (Yes I understand this is more of a management issue, where they don't factor in that second time into their estimates.)

Re:I'm not too good for code reviews

[drpentode]

I've had good luck with making it a mandatory part of the process. A developer fixes an issue, and before it can move into the "test" bucket, it has pass through the "code review" bucket. We do round-robin code reviews where a developer just grabs someone who's not too busy, and they take 10-15 minutes to do the review. No need for hour-long reviews since reviews tend to be done on smaller pieces of code. Different processes work for different groups. This is what worked for us.

Re:I'm not too good for code reviews

[Anonymous Coward]

Hey, that's fine.
 
But if you work on FAA traffic control systems, nuclear power plants, non-nuclear power plants, city traffic control systems, waste treatment plants, pharmaceutical manufacturing, food manufacturing systems, electric power routing utilities, natural gas pipeline systems, NASA, JPL, warship control systems, consumer vehicle computer chips, and gee, about 400 other industries.... FIND ANOTHER LINE OF WORK.
 
But if you don't happen to work in any industry where results matter, retain your current attitude. The worst that will happen is you'll be found incompetent and shown the door.

Re:I'm not too good for code reviews

[jeff4747]

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.

You misunderstand. I'm not against them. However, I've yet to encounter management who would allow the time to do them properly.

I'm well aware that time spent planning and reviewing would actually increase output in the long run. But the long run is not what the MBAs running the show care about. They have been taught to only care about next quarter.

It's not surprising though. It took a few millenia for civil engineering to figure out the value of plans and reviews. Software engineering will get there eventually.

Re:I'm not too good for code reviews

[doktor-hladnjak]

A few years ago, I took a training course on something or other (I think it was object-oriented design patterns). The instructor brought up an excellent example from outside the software world--germ theory and hygienic medical practices. Ignaz Semmelweis had shown that doctors washing their hands dramatically reduced mortality of patients in hospitals. At the time, his contemporaries thought he was crazy and many were offended by the implication that their hands were unclean. Even after Pasteur proved the biological mechanism of germ theory many years later, it still took years for handwashing to become a completely accepted medical practice because many doctors felt they just didn't have time for it.

Good software engineering practices like code reviews, source control, bug tracking, unit testing, etc. are generally no different. If applied correctly, they should reduce the overall time to release a product.

Re:I'm not too good for code reviews

[jeff4747]

You misunderstand. I'm not arguing reviews are bad in any way. I'm pointing out that the MBAs running the show don't think anything but banging out code is productive.

Thus they don't like the idea of having the team spend hours just reading other people's code. That helps next year's numbers. Not next quarter's. And next quarter is all that ever matters.

Re:I'm not too good for code reviews

[Anonymous Coward]

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.

A little over a decade ago, I learned the fallacy of statements like this in a very graphic way. I was kayaking a swift, narrow, rocky mountain creek of the type called "pool/drop" -- the creek empties into a pool of water, maybe 20 feet in diameter, then pours over a "drop", or small (one or two feet high) waterfall, rinse, repeat, etc. I had just dropped into a pool, and was trying out some tricks in an eddy behind a rock midway through the pool when I capsized. No big deal -- I know how to roll -- but since the current was flushing me towards the next drop, I needed to roll back upright quickly because you definitely don't want to go through the drop upside down (underwater). If you're lucky, you'll just get banged up on the rocks that make the drop. If you're not lucky, your kayak will get pinned by the current onto the rocks that create the drop, and that has killed kayakers before. So, because I knew I needed to hurry, I didn't bother to take the time to correctly set up for my roll. As a result, I blew the roll and ended up back underwater with even less time to roll upright and set up for the drop.

Way much bad juju, bro.

Underwater again, I decided I didn't have time to blow the roll again, so I took the time to set up properly, rolled upright, set up for the drop, and made it successfully to the next pool.

When you don't have time to do it right, you most certainly don't have time NOT to do it right, whether you are coding or kayaking.

Re:I'm not too good for code reviews

[jeff4747]

. I think it's our job as developers to push back against those crushing deadline and own up to some professional pride. Stop being bossed around.

That's an excellent plan during a period of 10% unemployment and massive offshoring of programming jobs. Perhaps we could practice saying "do you want fries with that?" while on the picket line.

The fundamental problem here is back in the 80s management's view went from a 5-year plan to a next-quarter plan. Code reviews are just one of a million "best practices" that get tossed aside because they will not help next quarter's numbers. Demanding a looser development schedule will not fix this problem. Heck, massive bankruptcies have not fixed this problem.

Until CEOs stop getting rewarded only for short-term gains, short term thinking will continue to dominate business. Doesn't matter how "professional" you are. The guy at the top of the org chart is deciding the course for the entire business, and he gets paid a fortune for tomorrow, and nothing for next year.

Re:I'm not too good for code reviews

[Anonymous Coward]

The problem isn't lack of time for review. There should not have to be a single review ever. Instead you need a good code architect that makes a very simple, isolated and modular approuch. If there is a problem with something, you can isolate the problem very easily to fix it, just like with cars.

Realy? No code reviews? Am I insane?

I'm realistic: nodoby, ever, will create large bugless software. Even with code reviews there were remote holes in OpenBSD.

So the problem isn't lack of review, but lack of easy to analyse and fix code. Simple as that.

Re:I'm not too good for code reviews

[Chelloveck]

I'm too busy for code reviews.

That's actually my biggest gripe with them. Code reviews always feel like an intrusion, like they're preventing you from getting real work done. Like coding. Coding is fun. Reviewing isn't.

You have to convince yourself that reviewing is part of the job. It's not necessarily fun part, but it is a part. Don't schedule yourself so tightly that you don't have time to review. Or, don't let your manager schedule you (and your team) that tightly. Yeah, I've been in this business a long time too, and I know that schedules are imposed by external pressures. And it's tough because at best, you're saving time in the QA cycle, not the engineering cycle. Think of it as an investment, spending a little extra time now to get back (hopefully) time not spent in last-minute debugging.

Re:I'm not too good for code reviews

[Anonymous Coward]

If your boss asks you to do something, are you really going to refuse? Times are tough, jobs are more valuable now than they've ever been.

Re:I'm not too good for code reviews

[rgviza]

The ones that don't and buck management end up losing their house and fucking their family over. Hmmm give me about 20 milliseconds to make that decision...

In the real world, when people depend on you, high falutin ideological development philosophy tends to take a back seat to the reality, unless you are an asshole.

Don't know about you but my family is more important than pissing my manager off because we have a difference of opinion, of which, mine doesn't matter since I'm not the manager.

Might explain why I don't need to advertise myself for development jobs in my forum signature on /.

Re:I'm not too good for code reviews

[Anonymous Coward]

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.

I'm in the same boat, but would like the time to do code reviews. I always learn something or get a refresher course when reviewing the more senior developers' code.

Re:I'm not too good for code reviews

[n8r0n]

That's incredibly myopic. Any system that yields an "ounce of prevention being worth a pound of cure" is one that should be even more important to someone like you who's under tight time constraints. During any given day, you're not just spending time on the particular task at hand, you're spending time as a result of past decisions. If you're adding a new feature to the Widget class, the amount of time that'll take today is directly proportional to how well that class was originally written. If it was originally tossed together, to be just "good enough", but not readable or maintainable, then every feature you add will be more costly to implement.

Similarly, your work today may involve fixing a bug, that would never have surfaced had the code been properly reviewed 6 months ago.

You have to try to envision an alternate universe, where you were taking the time to implement those quality processes that pay future dividends (commensurate with their cost, of course). The question you should be asking is, "would I be further ahead in that alternate universe than I am today?".

It's just bogus to claim "schedule" is the reason you have to implement processes that forgo earlier, cheaper fixes, for later, more expensive ones. I know lazy coders use that excuse all the time, and managers frequently go along with that short-sighted rationale, but it doesn't make it right.

Re:I'm not too good for code reviews

[rmstar]

And each and every single person who buys into this attitude is part of the problem.

As others have told you, reality does not leave much option.

At least not in that particular situation. The actual option is to unionize and kick ass. But people are too scared. And that is the problem.

Re:I'm not too good for code reviews

[Omnifarious]

I have in the past, and I was considered a better employee because I did it.

Re:I'm not too good for code reviews

[shutdown -p now]

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.

That is a management problem. I guess I should consider myself lucky in that in all places I've worked at for the last six years, code reviews were given due time and consideration. One even had a mandatory scheme (each file in the source tree had a designated owner and a backup, and at the very minimum one of those had to sign off for any change to that file, enforced via commit triggers).

Re:I'm not too good for code reviews

[r3x_mundi]

Good software engineering practices like code reviews, source control, bug tracking, unit testing, etc. are generally no different. If applied correctly, they should reduce the overall time to release a product.

I actually agree with you here...but to use your analogy against you...*which practices?* I'm sure medicine is full of examples of practices that were thought to be beneficial, but that later went out of fashion. Software engineering is the same. Tell me in what context a practice is good, how it should be applied, and what benefits it gives, what pitfalls should be avoided, and proof for these claims. A lot of the "practices" we do in software development is driven by trends e.g. the latest technology or the latest methodology, or politics e.g. excessive reviews, meetings, procedures etc.

Re:I'm not too good for code reviews

[SnowZero]

I work at a company with mandatory line-by-line pre-commit code reviews, and reasonably high standards for test coverage.
I have a motto that I think matches well with your explanation:
    I write feature code for free; I'm paid to write tests, review and document that code.

Re:I'm not too good for code reviews

[The Dawn Of Time]

No, the real option is to adapt and stop expecting other people to take care of you, and definitely stop expecting progress to cease just because you don't want to adapt.

Re:I'm not too good for code reviews

[Anonymous Coward]

Awww, does snookums think the world owes him a living? That's precious.

Re:I'm not too good for code reviews

[Anonymous Coward]

Bullshit, basically. You're just kicking the blame upstairs, which is exactly what I expect from this crowd of entitled whiners.

Casting pearls before swine

[Anonymous Coward]

I'm a C++ programmer surrounded by C programmers (pretending to be C++ programmers). Criticism from them is like Neanderthals criticizing Cro-magnon. These people still use FILE* x = fopen.....

I don't know how else to put it.

Re:Casting pearls before swine

[etresoft]

That is perfectly valid C++ code. If most of your colleagues are C programmers, then it would be in your best interests to code closer to what the rest of your team would expect to see.

Re:Casting pearls before swine

[Anonymous Coward]

Feels like you should drop the C++ and write in C instead. You'd all be much happier.

Re:Casting pearls before swine

[Anonymous Coward]

Crazy. Don't they know they should be using "HANDLE h = CreateFile..." ?

Re:Casting pearls before swine

[gangien]

You should show more respect for your superiors.

Depends on the reviewers

[Bookwyrm]

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 level the reviewer wanted it at just to get things done.

Re:Depends on the reviewers

[Your.Master]

I'm really curious about this. Could you describe an example? I've never seen the code quality go down objectively after a code review -- most objections I hear are that it's not cost-effective enough.

Subjectively, a cross-department change might be subject to differing coding standards, which might make person from organization A think he's making his code crappier to fit the ideas of organization B (where he's committing his code), because, I dunno, maybe organization B favours "goto cleanup" while organization uses a classic triangle error handling pattern. But from the perspective of the maintainers that's still a victory for consistency.

I'm not too good ...

[Anonymous Coward]

... I'm too stubborn. I think people avoid them b/c they don't like arguing or defending their own particular style, not that they think they're infallible.

Also, not to perpetuate a stereotype, but programmers don't always like working with people.

Obvious answer is obvious

[GreyWolf3000]

Because code reviews end up being an enabler for OCD coworkers to tell me how to indent.

Re:Obvious answer is obvious

[syntap]

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 to please comment code, and it never happens.

As long as a team has a documented set of coding standards (naming conventions and such) I'm happy to try it. A bug or two or security issues found during a review that don't make it into production pays for itself easily. But code reviews that devolve into coding philosophy debates aren't code reviews anymore.

Re:Obvious answer is obvious

[quietwalker]

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 effective solution to a problem than either working solo. The problem is scope and zealots.

Re:Obvious answer is obvious

[Anonymous Coward]

As long as a team has a documented set of coding standards (naming conventions and such) I'm happy to try it.

I've had success where style rules are kept to a minimum. For example each function must be documented with purpose/inputs/outputs, and the code must be "readable" - if the reviewer is struggling to navigate the code during the review then its likely to be hard to maintain and equally hard to notice problems. Style is explicitly out of scope for reviews (unless formatting is so mucked up as to make the code unreadable), and performance is only reviewed when there has been an up-front performance requirement.

My team handles a huge amount of legacy code (some inherited) over 100+ projects in multiple languages and IDEs, so rigorously enforcing style standards was never an option. A more relaxed approach to style has allowed us to focus on getting the software to do the right thing, irrespective of which part of the code base we are developing/maintaining.

Re:Obvious answer is obvious

[Oligonicella]

"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 to please comment code, and it never happens."

We had that guy on one of my teams. Third time it happened, he left the team.

Re:Obvious answer is obvious

[Mongoose Disciple]

So this.

In my experience, around 20% of developers are the kind of hyper-anal people who will argue endlessly about the stupidest details. Your chances of having at least one on any given team is extremely high.

In one case I (and everyone on tha team) lost literally 3 hours arguing over whether a particular boolean variable should be named something like 'finished' or something like 'isFinished'. And everyone but the one guy who had a hard-on about it conceded the point within the first fifteen minutes.

In some teams, management is wise enough to tell the hyper-anal guys they're not invited to other people's code reviews anymore. In a lot, they're not, and good lord can you waste ridiculous amounts of time on nothing important or useful that way.

TL;DR: I like code reviews in theory but in practice for most teams they waste much more time than they save.

Re:Obvious answer is obvious

[nitehawk214]

I agree about the overly OCD pedantism and arguments. This is why I prefer online-based review tools and not review meetings. This way people can review code at their own pace, and there and any disagreements are documented and forwarded up the chain of command to a moderator or decision maker for resolution. Also people tend to be less dickwadish when there is direct documentation of the discussions.

But to a bigger concern over "what is ok to bitch about", there should be a group concensus and documentation on what the style should be and direction to the reviewers on what they should be looking for.

And sometimes the code is ok and doesn't need discussion. People need to learn not to find nits to pick when the code is good enough.

Re:Obvious answer is obvious

[Anonymous Coward]

I also tend to think that conflicting methodologies come into play. One guy I worked with was an ex-Java guy. So he was all about "abstraction distraction" as I called it. Put a factory here, an interface an ORM there- we might want to change the backend DB one day! He would sit there and bunk heads with me on stuff like this all the time. Then there was another guy who wanted everything to be a template metaprogram. Or the PHD who wanted to put in a huge piece of revolutionary infrastructure where we would have a gui that we could enter configs into, and processes coming up would connect to, yet we were alloted a little over two weeks to move to xml config files for greater flexibility.. Put these guys in a room, especially with me, who is a "lets just get it done" pragmatist who generally kind of gets why the client wants this out now, and generally tries to find the easiest, most maintainable solution that can be implemented in the time given, and it becomes kind of a shitshow.

In general these things became big pissing matches where people wanted to show off their knowledge. Annoyed the crap out of me. Then again the above scene describes most meetings of more than 2 developers where they are encouraged to voice their opinions.

Re:Obvious answer is obvious

[Hatta]

Can't you just run your code through a pretty printer before you submit it for review? The only thing that really needs to be reviewed is the logic.

Re:Obvious answer is obvious

[Anonymous Coward]

If you had access to a computer and a programming language, you could solve that problem.

Re:Obvious answer is obvious

[Anonymous Coward]

Where I work, the coding standard is so strict it's almost funny. Stuff like comments on a line must begin on column 37 and end on column 69. Block comments must end on column 60. It's all meant to make the code readable on an 80x24 terminal with no syntax highlighting. But no one ever argues about the code standard. We have a script that checks all the formatting bullshit. So during a code review, you take two seconds to run that script. Then you don't waste time on syntax and you get to substance.

Re:Obvious answer is obvious

[Anonymous Coward]

If this is your problem, then the person running the code review is not doing their job. This should be set up front - code reviews are about the algorithms and the specific implementation - not (generally) about the trivial things.

Of course, your organization probably should follow some sort of standard and if the developer is veering from that, address it up front, and then be done with it. Simple. /CAPTCHA = inspects ...haha...

Re:Obvious answer is obvious

[Anonymous Coward]

Because code reviews end up being an enabler for OCD coworkers to tell me how to indent.

Obviously you should get a coding style guide. Adhere to it, whether you like it or not, and you get rid of that issue. You can often even just use an automated code formatter to do the job for you. Really, if you're having an issue with that, maybe the problem is you.

Re:Obvious answer is obvious

[Chelloveck]

Because code reviews end up being an enabler for OCD coworkers to tell me how to indent.

You're doing it wrong.

If you're doing a written code review (diffs posted somewhere, people leave comments) then you can easily ignore Mr. Anal Indenter. If you're doing a in-person everyone-gather-round-the-conference-table review the answer to that is "Noted. Moving on." And that's the end of it. If there's any further discussion on coding style during the review it's a failure on the part of the moderator (or leader, or coder) and on the parts of everyone engaging in the discussion.

And, if this is happening frequently, you need to adopt coding standards. It doesn't matter what they are, just something you can point to when Mr. Anal pipes up, and say "We do it this way." If you can't get the team to agree on a standard than have the manager or architect or lead developer make an executive decision.

And yeah, I've been there. Even after all that we still had Mr. Anal trying to convince people that the standard was wrong. Everyone else just has to ignore him. Boot him out of the review if you have to. I once (in very straight-laced dress-shirt-and-tie type of company) brought in a Nerf gun and hid it under the desk. When Mr. Anal started in, I whipped it out and nailed him. Repeatedly. That shocked him so much we actually never had another problem from that guy. But if the Asshole Indenter persists treat it like any other workplace conflict and bring in management or HR or however you'd deal with a co-worker who's keeping you from getting your job done.

Re:Obvious answer is obvious

[j_l_larson]

Yep. Code reviews work best when there is an agreed upon coding standard and maybe even some sort of Czar to weigh in with ultimate authority. There are some people who will waste everyone's time niggling pettily about spacing before and after the parens and such. Code reviews can also bog down indefinitely as different reviewers weigh in with a demand for a complete refactoring that differs fundamentally from another reviewer's outrageous time consuming refactoring request. You are then stuck in the position of trying to please two parties who are engaged in a philosophical power struggle and not being able to. I have seen code languish in limbo this way for weeks because no one had time to sort it out and no one would approve the code review. Nevertheless despite such occasional abuses, I still believe code reviews are essential.

Re:Obvious answer is obvious

[Anonymous Coward]

Wow, I don't wish your workplace on anyone if throwing items at people's heads in order to humiliate them out of something you don't like is acceptable behavior. If that guy had actually really been an asshole, he would have made it his mission in life to really humiliate you back, and you would have deserved it. That he didn't shows that he was just socially awkward and you sure helped him with that, didn't you:

That shocked him so much we actually never had another problem from that guy.

Re:Obvious answer is obvious

[Anonymous Coward]

Just follow the conventions and use auto formatting.

Re:Obvious answer is obvious

[Anonymous Coward]

Consistency in a code base is key. If you have a coding style in house, one of you is right and either you can tell him to go f*k himself or you should have indented your code correctly. If not, then that would hardly be covered in a code review now would it?

Re:Obvious answer is obvious

[MoriT]

And then that guy comes on Slashdot to complain about how useless code reviews are.

Code reviews are a way to shape code to conform to social norms. They allow for community-owned commits to community-owned code, and the development of a collaborative, shared culture. In the absence of such a society, or the desire to create one, code reviews will be no more constructive than a debate between Glen Beck and Rachel Maddow. Some people don't want to give up agency and independence, and for them code reviews can at best be a smarter compiler check. But at their best they can allow the creation of a program that transcends its individual contributors.

Re:Obvious answer is obvious

[Anonymous Coward]

The nail that sticks out gets hammered down.

Chances are if you aren't following an industry standard, your code stinks.
You are not a snowflake.

Re:Obvious answer is obvious

[Mongoose Disciple]

you can easily ignore Mr. Anal Indenter. If you're doing a in-person everyone-gather-round-the-conference-table review the answer to that is "Noted. Moving on." And that's the end of it.

Several times I've been unfortunate enough to have Mr. Anal Indenter be the boss/manager involved.

Not much salvaging that.

Re:Obvious answer is obvious

[Anonymous Coward]

When Mr. Anal started in, I whipped it out and nailed him. Repeatedly.

Re:Obvious answer is obvious

[JonySuede]

fuck the comment off. I remove them before I read a piece of code since they all lie in some way and code almost never lie.

Re:Obvious answer is obvious

[Anonymous Coward]

Amen. There are a number of reasons not to do code reviews:

* time constraints - both total resourcing and latency (Do I really want to tack on more unscheduled time when I am already working nights and weekends? And: If this bug is so pressing and people are waiting on it, does it really need to sit there waiting for two days for suggestions about how we should rename all those variables and filenames?)
* sense of ownership - parts of programming are personal; how you choose names, how you structure methods, etc. pp. Having someone with a radically different opinion chime in on all of your code distances you from professional pride
* reduction in accountability - if it's my code an my code alone that I check in, I am on spot for anything wrong with it. Submitting it to a large amount of group think absolves me from responsibility more than is good for the quality of the codebase
* standardization to the lowest common denominator - holding all of an organizations code to a standard risks replacing excellence with compliance. Rarely is the average you can get everyone to agree on the best possible solution for all problems

My suggestion is to allow anyone to make changes to any code while being completely responsible for the consequences. You can always review any checkin after the fact (that's what source code history is for) and change whatever you don't like. Just be sure that if I end up spending a day merging with your renaming of my variables, it'll show up in my weekly report and my manager might ask your manager questions about the business value of what you do.

In the perfect world maybe ....

[johnlcallaway]

If I had all the time in the world, I would test more thoroughly, and do more post-production audits too.

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:In the perfect world maybe ....

[JoeMerchant]

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.

Re:In the perfect world maybe ....

[johnlcallaway]

Yes I was .. which was 2pm where I was at. Anyway, I was eating my lunch at my desk because I rarely go out for lunch and didn't get a chance to go out to grab lunch until ... 2pm. And, as you will note, I'm posting this at 3amMST on a Sunday. I've been awake since 12:30am doing an install and things are finally running along.

Surprising, no code problems. Only issues were with operations loading stuff into the schedule. Seems when they migrated it from test they missed a few things, and got a couple others wrong.

So far, it looks like a code review would have been .. a complete waste of time....

But maybe operations could have had someone review the work of the guy on their staff who only has 6 months of experience and put all this stuff in. It does appear that inexperienced people need some type of peer review of their work.

The problem is poor developers...

[rtilghman]

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:The problem is poor developers...

[MichaelKristopeit424]

the problem is executives that know their employees are not qualified to do the job they were hired for, and yet they choose to complain about it on internet web site chat room message boards rather than fire the unqualified employees with cause, and pay for talent.

it's driving me nuts.

consulting execs are just symptomatic of a larger problem, which is a lack of quality and skill.

you're an ignorant hypocrite.

Re:The problem is poor developers...

[Spiflicator]

I would love to work with you. I am also driven insane by a complete disregard for the NEED for regression/unit testing. I'm also uniquely irked when existing tests which I have written begin to fail and go ignored. I'm not sure if the failure is in the education system (I never formally learned about testing, it just seems obviously invaluable) I think the easiest thing to blame is too many business people in direct management roles who have no concept of software quality.

Re:The problem is poor developers...

[JoeMerchant]

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.

Re:The problem is poor developers...

[Anonymous Coward]

I was directed on a project that we would spend x months coding and then, AFTER all the coding was done, y months testing. Management told us not to test the code, just write it, check it in, and leave the testing for the testing phase. How dare we test during the coding phase of the project!

Re:The problem is poor developers...

[Anonymous Coward]

The main problem is managers and companies who refuse to fire bad employees. You'd see less of this if management did their job.

Re:The problem is poor developers...

[Anonymous Coward]

code reviews are just symptomatic of a larger problem, which is a lack of quality and skill.

I think it's a combination of lack of skill and lack of incentive.

The company (aka management) gets what they pay for. Why hire a high priced programmer with an MS from a top school when you can get a general studies major from the local college who will write code for half the salary? Why let developers put time into the schedule for things like code reviews or integration testing when you can have a "program manager" who has never written any software but has a PMP certification challenge them with a aggressive schedule? And why reward developers for releasing a successful product when everybody knows it's the salesmen who deserve the fat commissions?

Re:The problem is poor developers...

[billtom]

I'll have to disagree with your assertion that code reviews have no place in a workplace with diligent, meticulous developers; for the following reasons.

1. There are limits to how much even a diligent, meticulous, skilled person can spot errors in their own work. Yes, quality regression testing can catch a lot of stuff. But you'll still catch a few bugs in code reviews, no matter how good your team is individually. Maybe we can argue about diminishing returns, but that's a different point.

2. Code reviews help to spread familiarity with the code among more people in the team. So that when another team member has to fix something in the code Joe wrote (because Joe is on vacation this week), there's a good chance that this isn't the first time he's ever seen the code.

3. Code reviews spread good practices and knowledge. Your team is unlikely to be all equally knowledgeable in all areas, so code reviews are a good place to spread ideas around.

So, even if you have good regression testing and your programmers are diligent testers themselves, I'd still suggest looking at what well done code reviews can do for you.

Re:The problem is poor developers...

[Anonymous Coward]

Good management is essential. When the only criterion by which people are judged is hitting a date, then they submit junk and spend weeks or months debugging.

Re:The problem is poor developers...

[Anonymous Coward]

"Higher demand = more crappy resources"

This, but the demand is the stuff your company is putting on people, and the resources are the time and money available, not the people.

Just remember one thing: treat your staff like adults who enjoy doing good work. If you bear that in mind as a constant, the real variables will reveal themselves.

Re:The problem is poor developers...

[Anonymous Coward]

I think your comment should go something like:
The problem is <consulting exec> yap about others<consulting exec/> you should pay a consulting exec to <consulting exec> insert meaningless acronyms <consulting exec/>
and it should be read like
The problem is <reality> consulting exec ignores the most elementan thruths about sw engineering<reality/> you should pay a consulting exec to <reality> stay out of the way from real engineering <reality/>

Re:The problem is poor developers...

[Anonymous Coward]

Lets translate the above message to language which is easier to understand:

Yes, the real problem is that the coworkers are idiots and not someone who tries his best even after roadblock after roadblock are placed in their path by the more experienced developers. The untested code coming from other developers is a big problem, but my own code is perfect. Developers should do their own testing, so that QA will be unnecessary. If other people do not do things exactly the way I've been doing for last 20 years with notepad, they should be fired for incompetence and there would be no reason to even ask them why they did it in this way? It's probably university professors who caused this misery. Or the people who are building free programming languages. They caused all this! It just drives me nuts! This was all about code reviews so just fix that, and everything will be fine. We're still making all the money and developers are just submitting broken code.

Re:The problem is poor developers...

[Darinbob]

Maybe some management would help. Just set down the law "no check ins until you've tested it once". If a developer starts the cheat the rules it will affect their performance reviews. I'm the laxest and laziest programmer out there but I'm going to take some pride in my work when I check something in; if I get to the point where I think "what the hell I'll submit it and let someone else figure out if it works" then I deserve to be fired.

Re:The problem is poor developers...

[HellYeahAutomaton]

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?

There are a lot of possibilities as to what is driving this idea. Below is my attempt at addressing them.

1. Software development has its various schools of thought including (but not limited to) ad-hoc and hero style programmers, waterfall, Agile, Scrum, TDD, and iterative. For the most part, real world software development processes are not taught in schools, and in some ways are similar to fad diets. There are two very different mindsets involved for coding the software and testing the software -- and if you test the same code you write, you are more likely to miss bugs. This is precisely why it is good to have other people test the code (and review it) than those who are writing it. There will always be blind spots. While some developers may be familiar with the basics of a debugger, most probably don't know how to use disassemblers, decompilers, and static analysis tools. There is no One True Standard Way to do things.

2. Going the distance with solid code, including unit, peer, and system level testing requires a level of discipline that most are not familiar with and not trained/taught at schools. Schools do not turn out software artisans, they merely turn out logical thinkers. There's an adage in software development that you can get it fast, good, or cheap: pick two. Most developers want to do it good, but that is often a luxury that conflicts with stated business goals. Most commercial software starts off as a badly coded prototype, and most developers are expected to shine that turd so long as it makes the business money. Most of the time the codebase is held together like chicken wire and bubblegum.

3. Perhaps there are interpersonal/social problems at play with your team, and the groups they interact with.
      - Do you treat your developers as professionals, or do you treat them as technicians, fix-it people, and common laborers?
      - Do your developers have clear expectations on how they are expected to work with QA?
      - Do your developers have any experience working with UE and QA?
      - Are your UE and QA people confrontational?
      - Does your team have a clear picture of your expectations?

It sounds like you are seeing the culmination of multiple problems, not entirely the fault of your team. I encourage you to look at these areas described.

Re:The problem is poor developers...

[Anonymous Coward]

Posting as anon to save myself the same burn you got, but man I think your point needed making and it was the GP that was the troll.

You pay for quality, you will get it. If you are not getting it, chances are you are not paying for it. And guess what, developers are not the ones with control of the budget.

The GP sounds like someone ranting about how he isn't getting Michelangelo's David from a team of monkeys. The problem isn't the monkeys, its the guy who thought he should only pay in peanuts. The GP sounds like someone who was hired by the monkey wrangler which is even more mind-blowing.

Still, I don't have to deal with the GP or the monkey's he works with. Funny thing is once you prove you can do quality work, there are quality managers out there who will pay top dollar to hire you.

Re:The problem is poor developers...

[Anonymous Coward]

I think you're missing the point, and have instituted improper reporting requirements. If you require a module to be written, integrated and tested in 10 days, at the end of the 10 days, you get what's ready, and if it doesn't pass Q/A, the devloper gets more time.

Line the salesmen up and take their computers, phones and badges as the file out the door. Or conversely, line up the managers/resource schedulers and do the same.

If neither of these help, you should have hired a new set of developers too.

Re:The problem is poor developers...

[Anonymous Coward]

Code reviews aren't meant only for catching bugs but also for learning new things and discovering alternative solutions to problems. Bugs should typically be caught in unit and integration testing. Code reviews are completely different and, in my experience, happen less frequently and shouldn't be relied on as a method of testing/bug-finding.

Re:The problem is poor developers...

[Anonymous Coward]

My best guess is that you hit the nail on the head with higher demand = more crappy resources.

In university I could either study for 2 hours a week and get 80%+ in tests, or I could study 40 hours a week and maybe get 95%+, there is a curve there, and there is a curve here.

If software works, functionally producing some amount of value (quantifiable or not), what does the quality matter to share/stakeholders? If I can higher a crappy developer to produce crap that works most of the time producing value for $40k/year why would I go out of pocket an extra $30-40k to get another 15-20% value out of a products lifetime. It makes no good business sense to create quality software, it hasn't for a long time if ever. This notion is just far more understood and accepted these days. Which is why you are finding so much more crappy code. Crappy code has a better ROI than quality code.

It sucks but this notion isn't anything new or outlandish to any industry let alone software.

I would also guess that less formal CS training, the notion that the users will test software for free (as long as it works most the time), also play some form of a role in all this.

Re:The problem is poor developers...

[Anonymous Coward]

I've been on the development side when things have been given for QA that don't work. Typically it's because management has committed to a project, and promised they can have it in 6 weeks. Without talking to any of the people who will be doing the work. When they finally give the information to the people doing the work, the response is: "No way in hell can this be cone in 6 weeks, it'll take 3 months!". At which point, management says: "Well we're giving it to them in 6 weeks anyway." Then, when QA reports nothing is working, they have to push out to the 3 months that it was actually going to take. But, by that time, so many hacks and shortcuts have been taken to get "something" working - it all has to be ripped out, and re-coded and there's certainly no time for code reviews since everyone is scrambling just to get the damn thing working.

My own experience

[MobileTatsu-NJG]

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.

I don't always test my code, but when I do....

[FictionPimp]

http://troll.me/images/the-most-interesting-man-in-the-world/i-dont-always-test-my-code-but-when-i-do-i-do-it-in-production.jpg That says it all for me.

Re:I don't always test my code, but when I do....

[pixelpusher220]

Made. Of. Win.

Awesome!

See monkey do

[bidule]

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:See monkey do

[gnasher719]

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 graphics device driver if the machine awoke from being put to sleep, at a time when no debugger was present yet. And due to some interesting circumstances, it only crashed the machine if another major code change that another programmer checked in a week later was present. And only if the machine was put to sleep for some time between 35 and 40 seconds.

I could have killed him.

Re:See monkey do

[nitehawk214]

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.

In my experience the people who really are too good to need code reviews are the ones that insist upon them. Managers will insist upon them so that more than one person has seen all of the code in the system. It just makes sense.

Re:See monkey do

[Darinbob]

If you are too good for code reviews, you are too good to work with mere mortals.

Re:See monkey do

[bidule]

I am sorry, what's the punchline?

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.

I suppose he also did "++p" and "p += q". I've seen "if (! p)" everywhere, from beginners to advanced users, how does that demonstrate "superior intelligence"?

That line of code was only ever executed in a graphics device driver if the machine awoke from being put to sleep, at a time when no debugger was present yet. And due to some interesting circumstances, it only crashed the machine if another major code change that another programmer checked in a week later was present.

Why was it not the other programmer's fault?

How did "p == NULL" differ from "! p", was NULL defined to non-zero for that driver, was there some C++ operator overloading or was there something unusual about the processor?

Code review as a method for knowledge passing

[Anonymous Coward]

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.

Re:Code review as a method for knowledge passing

[Nerdfest]

That's actually one of the big benefits ... it forces at least a bit knowledge distribution so that you can avoid the "We can't fix it because Bob got hit by a bus problem". Having a low good "bus factor" is an important thing in ensuring that a system is maintainable.

"formal" code reviews: a waste of time

[spectro]

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.

Re:"formal" code reviews: a waste of time

[ChronoFish]

"...whole concept of everybody getting together and put somebody's work to shame..."

Yes that would be a problem if that is the attitude your team goes into a code review with. Our team uses collaboration software to perform the code review - so it evolves a bit like a wiki over a two-three day period. Usually we are asking why things are being done - not "you shouldn't do this or that" or worse "Your code sucks ass - WTF!".

Style changes are stated as "Next time define your constants in the config file" or "Try using a Case statement here" or "You've initialized your variable, but after this logic it will never contain the default value. Is this what you intended?"

There may be good reasons why the developer chose to go the route he did - letting the team know why code was written a certain way will allow the team to utilize the knowledge in future iterations.

-CF

Re:"formal" code reviews: a waste of time

[Anonymous Coward]

Bad habits are perpetuated by a lack of code reviews. A code review is of the code, not the developer, and if the code is not up to par, then a review will make it so.

If things don't get rectified by the next time around if it was agreed upon that changes should be made, then the lazy bastard should be taken to task.

Allowing people to "pick up a task involving work on somebody else's code" breeds a hostile atmosphere, because you need face time to explain and agree upon changes, instead of relying on a cowboy coder mentality whereby every change is for the better regardless of who does it.

Re:"formal" code reviews: a waste of time

[Anonymous Coward]

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.

Therein lies the problem. A good code review is one where people honestly critique the product, find possible areas for improvements and suggest good strategies.

A bad code review is one where management criticises the programmer/designer with the intent of keeping him/her in place so that he/she won't expect decent raises and bonuses. Of course the programmer also is likely to put less effort going forward, but in shops like that, the programmers are a disposable commodity anyway and that just gives one more excuse to churn the team.

The first kind of code review tends to happen anyway on a project where multiple people are working with the same codebase. It may not be done in formal meetings, but people pass information back and forth at the water cooler. The second kind of code review is a pre-planned ambush.

Re:"formal" code reviews: a waste of time

[Eil]

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.

There are two main kinds of code review processes: good and bad. This is the bad one. It's a huge waste of time for everyone to sit in a meeting, read code together, bikeshed the fuck out of it, and get nothing done. I can't imagine a worse development methodology than coding by committee.

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.

This is the good one. We use an open source product called Review Board to do just this. Before anything is committed to trunk, it has to be approved by at least one other developer first. Our defect rate from QA has dropped to 1/3 of that from the previous development cycle since we started doing code reviews and a few other cherry-picked "agile" techniques.

Re:"formal" code reviews: a waste of time

[Chirs]

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.

That only works if everyone is capable in every area. I'm a pretty good kernel developer and nitty-gritty POSIX guy. I'm not nearly as good at system engineering, Markov chains, fault progression modelling, build system management, database maintenance, etc. Not everyone is interchangeable.

Re:"formal" code reviews: a waste of time

[ShanghaiBill]

... but this whole concept of everybody getting together ...

If your code reviews involve "getting everyone together", then you are doing it wrong.

You should read up on industry best practices. Look into tools like CodeCollaborator.

Re:"formal" code reviews: a waste of time

[trout007]

I'm a mechanical engineer but we have design reviews. The most important question is why have the review? We have project reviews but those are for project management as far as schedules and budgets. We have peer design reviews where we get other engineers, machinists, and welders to take a half hour to look over a design a few times during the design process. These are set up for the designers benefit. They are in charge and it is up to them to take any comments or advice. I find these reviews very productive because it never turns into criticism where you ask why didn't you do this or that.

this

[DeadCatX2]

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

[Spiflicator]

This sounds exactly like what a manager would say. I'm sure it varies from situation to situation, but I'm 200% confident that my group would be more productive long term if there was more time spent ensuring quality, and educating each other, up front.

Re:this

[DeadCatX2]

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 broke, don't fix it.

Re:this

[Lunix Nutcase]

Yes, fixing separate bugs that could have been caught earlier due to a code review...

Re:this

[DeadCatX2]

QA testers are supposed to find bugs and report those results back to the developers.

Re:this

[cjhuitt]

In the time we all spend reviewing my code, we could have each fixed separate bugs in the software or completed a new feature.

Look at your last 50 or 100 bug fixes. How many of them could have been caught with a simple review? Take that data and apply it to your statement here: perhaps in the time you spend having your code reviewed, you and your coworkers will fix more bugs than you would have separately. It's just that bugs not submitted aren't as visible to anyone outside those people.

Try it. Keep some metrics on "bugs" found. You might be surprised.

Re:this

[Omnifarious]

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:this

[Omnifarious]

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:this

[sconeu]

And it's been shown that the earlier a defect is found, the cheaper it is to correct.

Re:this

[mad.frog]

No, that is 100% wrong.

The purpose of QA isn't to *find* bugs; the purpose of QA is to *verify* that *there are no bugs*.

If you are giving something to QA with unknown deficiencies, and expect them to find them for you, you should be looking for another line of work.

(Known deficiencies of a work-in-progress are a different story, of course...)

Re:this

[drpentode]

Finding a bug in your own code costs one of unit of work. QA finding a bug in your work costs 10 units of work. Having a customer find a bug in your code costs 100 units of work. Doing code reviews increases efficiency. Plus, you get to both teach and learn from your peers.

Re:this

[Anonymous Coward]

Bullllllshit dawg. If you're a good tester and coder then you'll have the shit ironed out, without the need for a code review. It's called not being a pussyass coder.

Re:this

[Anonymous Coward]

Well lookie here, another arsehole dev that expects QA to clean up his messes instead of making an effort not to create a mess in the first place. How generous of you to provide job security to your QA team.

Re:this

[Entrope]

Amen. One can rely on developer-written regression tests and QA to detect bugs, but those two approaches will never find all the bugs. Your customers spend years or decades stumbling over all the bugs that QA missed. A lot of companies and customers are going to accept that, but that acceptance is almost unique to software. If any other kind of designers routinely delivered the kind of flawed products that software houses pump out, those other designers would be sued out of business by angry customers and regulators.

Re:this

[Omnifarious]

No coder is that good, except, maybe, DJB.

And this bug would've randomly cropped up 1 in 256 times a specific feature was used in a specific way. And it was data dependent. The testers would've had to have magically chosen just the right set of data to test the feature with. It would never have happened in test. At least, not until I came up with a data set that was purposely designed to exercise the bug to prove that it was there, and gone after it was fixed.

Re:this

[6Yankee]

A bug you didn't mean to be there, sure. But leaving some you know about in there, or planting some deliberate ones, isn't a terrible way to A the Q of your QA. They're human too, after all.

Re:this

[Anonymous Coward]

Your statement is true, if you care about the end user. If you just care about getting paid by finishing your work on time so that its good enough to sell the product and make money, then your statement is no longer true.

If the company creating the product does care not about long term satisfaction, they may just want to ship a "good enough" product that people will buy. In that case code reviews are out the window. I'm pretty sure that's what jeff4747 was getting at.

It's unfortunate that any company thinks that way, but it is the case pretty often.

Re:this

[Anonymous Coward]

And no matter how many times you tell management this, they will continue to prefer to pay the higher cost later.

Re:this

[profplump]

On the other hand, if we built software to be as reliable as planes we'd still be running System5. "Other designers" don't put out entirely new products over a period of a few months -- they make incremental tweaks to a product that's been in production for years. And when they do build something actually new it can take years to get the first model out the door.

It's not so much that people accept errors in software, it's that they'd rather have cheap, quickly-produced software than waiting 5 years and paying 10 time as much to get a perfect version. If it were as easy to re-bore your engine block as it is to apply a software update you'd probably see the same thing cars.

And there are plenty of customers that demand good software for certain applications, and they often get it. But they pay and wait for it too.

Re:this

[Comrade Ogilvy]

Much better. But that is not quite correct either.

The job of QA is demonstrate, with concrete evidence, that a product brings value to customers in an understandable and predictable manner. It is a business decision whether the right customers will be happy enough.

As a practical matter, testing is important. But theoretically speaking, whether QA tests a little, a lot, or at all should be determined on a case by case basis.

I do agree wholehearted with your sentiment. An engineer who throws code over the wall in order to find out if his work doesn't suck is an engineer who is pathologically disinterested in being competent.

Re:this

[leenks]

If that is the case you are doing it wrong.

The code review process we use is pretty lightweight yet thorough (it is built around gerrit and hudson), and we've caught lots of issues before they made their way into the codebase. I'm sure we've stopped more bugs going into the repository than we could have fixed if we hadn't been doing reviews. We also generally have cleaner code going into the repository, because now everything is being reviewed the team are striving for clarity.

Admittedly this is a small project - 500k semicons, around 8 developers (typically around 5 very experienced, 3 junior) at any one time - but I don't see how overall productivity could do anything other than go up. We also have poor test coverage (but better than the majority of projects I've seen) - about 35% overall, but close to 75% on the critical core parts. The GUI is currently lacking any significant tests, but thats partly a byproduct of having rapidly changing customer requirements for the GUI - it is also where the majority of our regressions have historically been.

Re:this

[rmstar]

Finding a bug in your own code costs one of unit of work. QA finding a bug in your work costs 10 units of work. Having a customer find a bug in your code costs 100 units of work. Doing code reviews increases efficiency. Plus, you get to both teach and learn from your peers.

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:this

[Darinbob]

I think spending half your time on a code review is too much time. Code reviews should be quick. Sometimes a once-over, usually slightly more detailed with enough looking to at least figure out what the changes are doing, if the changes do what's required, and understanding how the changes affect other team members. No more than an hour or two a week on average, probably less.

Re:this

[Your.Master]

If the break is subtle and isn't detected, you could have a major system overhaul on your hands. This is particularly and tragically true of software where security is an issue.

To give an idea where I'm coming from, most of my experience is in very large pieces of widely-deployed systems software, where performance and security are both critical. These rules may well not apply if you're working on small to medium projects, or projects only for internal use in a company where all the users already have full read/write access.

I'm going to throw out a couple rules:

1. If you're not code reviewing all of your security-related code, you're not writing secure code.
2. All code is security-related until proven otherwise.

The same goes for maintainability, by the way. Only fixing things when they break and not looking at the things everybody else is doing is an almost certain way of propagating redundant slightly incompatible systems and creating a spaghetti dependency graph in the long run, at least for large software projects. Eventually you descend into a place where the product is almost always broken. To some extent, these things happen anyway with code reviews; they are just a tool to reduce the spread of the problem and even turn it back at times.

The "worse is better" mentality can work for other areas, notably performance. Developers do love to do premature optimizations at times. Even then I'd recommend at least one other person spare a thought of the high-level impact of nontrivial changes so nobody accidentally misses a fundamental flaw.

Your last sentence I actually agree with. That's why you'd do code reviews. Once the "good enough" code is in there, you'll have trouble scraping it out until it's too late. Until it's in there, it doesn't work. If you're going to take the regression risk of making a change, you might as well make the right change. Otherwise, you run that risk again when you fix the originally broken code.

Re:this

[oursland]

Every bug is a condition that the programmer didn't think about. Not thinking about the total problem should be viewed akin failing to do your job as a programmer. Each bug found is another bit of evidence to the fact that the programmer isn't thinking about the problem and how his actions affect the systems.

Re:this

[MadKeithV]

Or, the old simple adage: "Do you want it good? Or do you want it thursday?".
(Which I actually picked up from a musical composer, not a software developer....)

Re:this

[MadKeithV]

And no matter how many times you tell management this, they will continue to prefer to pay the higher cost later.

Because the earlier ship date may make them more money by the extra sales and potentially cornering the market early than the "higher" cost to fix the remaining bugs later.

Short answer: Yes

[JoeMerchant]

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 code review in the "as needed" category, they come up once every year or so, and it feels like more of a flogging for the coder who is being reviewed than a productive forum for learning. Apparently, in 20 years of coding, my management hasn't ever felt the need to review my code. Regulatory agencies have demanded verification and validation, but that is an entirely different thing from programmer to programmer review of how code works.

When I have been in the management chair, I pushed to have some kind of meaningful code review at least every 3 months. That's not often enough, but the places where I was in management were usually of a cultural mindset that code review was a waste of time.

That belief that code reviews are a waste of time seems to be most dearly held by the people who either a) are not confident programmers, b) confident programmers who aren't really that good, c) have (or take) no responsibility for quality and maintenance of a significant code base, or d) don't understand programming at all.

Re:Short answer: Yes

[Anonymous Coward]

I think reviews are only good in situations where you have other skilled coders.

Where I work, I'm the only one in a room full of non enthusiastic "this-is-my-day-job-not-my-hobby" people. Where as I'm dreaming of code and working on my side projects at home in my spare time.

Our code reviews involve lame attempts at correcting my code, where I waste the rest of the meeting explaining why they are wrong and teaching nubile programmers how basic patterns work.

These are more educational "how to program" reviews than actually getting quality feedback about my code. I'd *love* real code reviews, but alas I'm the only one who isn't an idiot in the room.

Re:Short answer: Yes

[tp_xyzzy]

While your answer was mostly ok, the last paragraph ruined it. There are no people who are not confident programmers, because everyone is trying their best. Everyone is different. For some people or teams code reviews are useful. But sometimes it just doesnt work. This is not because the developer was inexperienced or not very good, but instead because code reviews just are not suitable for everyone. Big mistake is trying to forcefeed your own conventions and practises to other people. Not everyone needs code reviews. Or there are other problems. Assign the problems to the responsibility of the process. Never blame other people. People are not idiots, Programmers especially. Everyone is still trying their best.

Re:Short answer: Yes

[JoeMerchant]

While your answer was mostly ok, the last paragraph ruined it. There are no people who are not confident programmers, because everyone is trying their best.

In the ideal touchy-feely world where code reviews benefit everyone, you are absolutely right.

Back in my life, I had a coder who was having a terrible time getting a routine to run quickly enough. He resisted over the shoulder help. He ignored outline sketches of how to do it. When it came time for code review, he pulled me aside and stated outright that he preferred one on one help - the same help he had been resisting for weeks. Turns out that the reason his code was so slow was because he had an unnecessary level of loop nesting, slowing things down by a factor of about 100. He couldn't, or wouldn't, fix it after several of his peers outlined the solution for him. When I finally rewrote it for him, he added comments to "see me about this part of the code" because he just didn't understand it. He had a Ph.D. in Physics and years of teaching experience.

Of course, you may be right, he might not have had a confidence problem, he asked for a 30% raise after that. There are some people who seek jobs in programming who are clearly better suited to other endeavors. Sadly, if they are experienced enough, they can say all the right things for several months - but the truth does come out eventually.

as long as you have useful reviewers

[Anonymous Coward]

Having other intelligent and experienced developers look at code is always helpful. Heck, this is one of the principles behind open-source.

However, sitting in a meeting with a bunch of dullard cow-orkers struggling to grasp basic ideas is a waste of everyone's time. The good ones could be off doing something useful, and the rest could be off eating Cheetos, watching "Two and a Half Men", picking nits out of their hair, or whatever.

Why yes, I am glad I don't work at my previous job anymore.

code review rocks

[Anonymous Coward]

Well, I can speak from experience as a QA manager for a major publishing company, once we instituted a formal unit testing and code review program, the defect count went way down. The apps came to us in much better shape from the start.

Code Reviews Don't Find Bugs

[ShavedOrangutan]

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, so finding out that a developer isn't following design standards doesn't matter because it's too late to do anything about it.

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:Code Reviews Don't Find Bugs

[halivar]

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:Code Reviews Don't Find Bugs

[JoeMerchant]

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?

Re:Code Reviews Don't Find Bugs

[Nerdfest]

I'm pretty sure that actual studies have been done on the number of bugs found/hour by code reviews. Really, you can't afford the time *not* to do code reviews.

Re:Code Reviews Don't Find Bugs

[ChronoFish]

Then you're doing your review wrong.

A formal sit-down where someone is walking everyone through the code is destined to fail in the way you mention. Using collaboration software where the team can go through the code on their own pace, by reviewing a highlighted diff of code and comments are tacked to the lines of code in question is the way to go.

In our environment we ARE able and DO catch bugs. The reviewers need to know what to look for - and they need to be engaged in the process.

Set ground rules :
- don't comment on tabbing, braces, etc UNLESS it's truly unruly.
- Ask questions - don't assume the code is wrong.
- Look for the obvious: Memory has been allocated before use, variables have been defined and initialized, exceptions are handled, "else" statements are reachable, HardCoded values are meaningful or set in a Constant
- Look for the warnings: Compiler/Script warning suppression - Is this really necessary or is the developer being lazy. What cases does it miss? Are the hard-coded values really necessary - or are they arbitrary? If an array is set to a certain size, are there checks to prevent it from being over-run?
- Look for the edge cases: Is there code that will never get executed? If parameters aren't checked in a function call, is there risk of down-stream affects?

Just doing this checklist alone will find bugs and help keep code clean. The team lead/senior developers should be coaching, not criticizing the other developers. This is a great opportunity to improve the team knowledge base and experience levels. Not holding reviews, or holding reviews that are rubber stamps should put your team lead in a seriously compromised situation (I.E. He needs to be held accountable for code that is not properly reviewed before it enters production.)

-CF

Re:Code Reviews Don't Find Bugs

[TheRaven64]

I've seen code reviews find bugs, but that's not what they're for. Code reviews tell you now, when you can still remember how the code works, which bits of it are going to be hard for you to understand in six months time when you revisit the code and can't remember. That can save a lot of time overall.

Nope.

[Anonymous Coward]

I've been a programmer for over a quarter of a century; but despite all the experience, I still find myself making simple mistakes once in a while. Maybe I'm just not brain-wired for coding as well as those uber-graybeards or teenage geniuses who can write a rock solid kernel by themselves in a fortnight....but even if I were....then all it proves is it works *under my interpretation of the specifications and my knowledge of development*. My co-worker John Doe who is a savvy C# developer could review my code and say "Check out this MSDN article; it shows a better, more robust way to do what you're doing."

And I agree with Joe_Dragon. We have non-developers, rookie developers and senior developers all reviewing each others' software changes and code reviewing where applicable. I can safely say that every permutation of "[non-dev/rookie/senior] finding a bug in [rookie/senior]'s code" has been met.

metrics

[Anonymous Coward]

wtfs per second.

The only valid measure of the quality of code during a code review.

automated testing can let stuff fail but still pas

[Joe_Dragon]

automated testing can let stuff fail but still pass the auto tests and it still misses the GUI / user experience testing.

Could have linked to the original article

[Anonymous Coward]

Since the interview was with Dr. Dobbs, why not link to the Dr. Dobbs blog?
http://drdobbs.com/open-source/229502480

The reality is ...

[Anonymous Coward]

except for regulated industries(where fines and penalties can mount up pretty darn fast) and life and death control systems(where people die if it's wrong) ... code reviews aren't cost effective. It's usually cheaper to find a bug in production than to spend 500 hours ( 5 reviewers x 10 hrs) reviewing the code and looking for bugs that probably aren't there. In an ideal world everything would be reviewed but in this one the dollar(pound/ drachma whatever) ... the "bottom line" rules.
-MondoGordo

It depends how they're used

[Anonymous Coward]

Code reviews are good at finding certain types of problems such as coding standards violations. That being said, they are not a substitute for real testing. Don't fire your testing team because you think all validation can be done with code reviews.

or shift the solution elsewhere

[gbjbaanb]

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 (as a cursory look through to spot the glaringly obvious problems isn't too much use).

So why do we spend so much programmer time checking and rechecking, when we can test the system like we used to in the old days. Not only can you get dedicated testers (who have fresh eyes on the problem), but they test all the other aspects that code review and unit test would miss.

I know there are places where unit tests are very good - regression testing a library for example, and code reviews are good for the blindingly obvious errors, but too much emphasis are given to these for almost every part of software nowadays. For example, I read today (on stackoverflow) that someone decided to use TDD for his new project and wrote 50 tests per module and it took him ages to get 100% test coverage, and now he can't bring himself to write a new module because it takes so long and is so unproductive. The answers were generally of "you're writing too many tests, 80% coverage is really good", but then.. if you only unit test 80% of your system, you're going to have to 'system test' the whole thing anyway!

So, I think they have their place, but they're not magic bullets of software quality, no matter how much my boss (who's been reading these blogs on the internet again) thinks they will be.

Re:or shift the solution elsewhere

[marcosdumay]

Software becomes more and more complex when you go from design to implementation to behaviour. That is why people try very hard to catch the errors at the earlier phases.

Yes, that couples very badly with economical behaviour. We try very hard to pull costs backward on time expecting it to pay at the long run, but those pulled costs may make the long run simply not exist (as in e.g. bankrupting a company). That is one of the reasons managers seem to always be in conflict with developers.

Code Reviews are never done right.

[140Mandak262Jamuna]

Management usually feels code review is a waste of time. If we could make that programmer produce more new code instead of reviewing (an apparently working) code it is more efficient right?

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 another tack taken by managers.

Even when the top management insists on code review, the middle managers implement it in the most asinine manner. Comment lines to code line ratio. Length of functions. Number of spaces per tab or braces closing style etc instead of truly insightful code review of algorithms and failure modes and consequences.

Finally in my neck of the woods, (scientific and engineering analysis software development) competent peers are rarely found. Even a top company employing hundreds of developers for circuit simulation, say, would have just two guys in any super super specialty like eye-diagram creation, and the work load, locations, skill sets etc are not likely to permit easy code reviews.

Re:Code Reviews are never done right.

[Nerdfest]

competent peers are rarely found

When you do a code review (an informal one, as I prefer), have both a competent senior programmer and an new or less competent programmer included in the review. It's a pretty good way to improve the abilities of the more junior person.

Yes

[Anonymous Coward]

Yes, Yes I am.

I don't like looking at other people's code.

[wcrowe]

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.

Re:I don't like looking at other people's code.

[Nerdfest]

Working and maintainable are two vastly different things. Your approach works up until *you* get stuck adding features to someone else's code.

Re:I don't like looking at other people's code.

[b4dc0d3r]

I've never understood this. If you're not reading code, you're missing probably the greatest learning opportunity there is in the field. Dry examples or tutorials rarely show complex use cases, and most examples I've seen use poor style, or are completely insecure. Reading other people's code gives you a real-world, (hopefully) working example. And if it's something you never had to do, or never thought of doing, you might learn something.

Re:I don't like looking at other people's code.

[wcrowe]

Yes, but I see other people's code all the time when I'm maintaining applications -- which is at least 50% of my job. Looking at other people's code, just to scrutinize it, is tedious and boring. 99% of it is mundane stuff that is not particularly innovative or interesting -- probably because 99% of the problems being solved are not unique or challenging.

At my company we do have time set aside every week for "code review". We call it that in order to satisfy management. We actually use the time to share interesting problems or solutions. That is more along the lines of what you are talking about, except it is more direct and less tedious. Just looking over someone's shoulder, hoping for the miniscule chance of seeing something interesting, is not for me. I don't have time for that.
 

Well...

[AngryDeuce]

While I'm not a programmer myself, I have friends that are/were, and based on their descriptions and stories, it's always seemed like code reviews end up an antagonistic process instead of the collaborative one it's supposed to be. Maybe it's just the luck of the draw in finding a good group, but I've heard of some serious snark going on between programmers that disagree with each others styles of coding. Something as simple as indents driving people to the point of blows, etc...

Then again, maybe those stories were notable because of the antagonism and the good experiences are forgotten? Everyone seems to remember the negative, human nature 101...

Impostor syndrome

[CHJacobsen]

I believe there are multiple reasons, but impostor syndrome could be one.

I am a pretty good programmer. I work as a consultant, roughly the 2-3rd most senior developer in a company of 35, and i usually end up at our toughest and most important clients, and the feedback i get tends to be very good. However, i'm constantly haunted by a feeling that "this could use some refactoring" or "there might be a better way to do this, and i feel stupid about not finding it". Code review really shows these things, and it can be intimidating.

Can anyone else relate to this?

Re:Impostor syndrome

[calmofthestorm]

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.

Re:Impostor syndrome

[geekoid]

Yes. But I have learned how to address the intimidation / fear issue.

As a consultant, you probably have the ear in someone farther up the food chain. Maybe you should design code review meeting? remember, small chunks, and the meeting should be about 30 minutes, and never extend past an hour.

Re:Impostor syndrome

[shutdown -p now]

there might be a better way to do this, and i feel stupid about not finding it

Don't be. Human brain is a peculiar thing - once set on its track, it is very reluctant to find a different one. That's why having someone else look at your change can make them spot obvious things that you wouldn't have seen even if you stared at it for hours.

But if you do code reviews for others, you'll quickly find out that this works both ways, and even the most bright and experienced programmers are not exempt from this.

Absolutely Nobody

[Archeopteryx]

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.

Code review opportunity to demonstrate good coding

[SiteLinker]

If your ego is so big that you believe you don't need your code to be reviewed to make it better, maybe you should approach it as an opportunity to share with the rest of your staff the practices that you use. Code review can work both ways. Help clean up weaker code and learn from great code. Having the code review keeps the egotists on their toes ;)

Garbage in Garbage Out

[stwf]

In the end code reviews are only as good as the person doing the reviews. SO yes, having your best coder look over everyones code is a very beneficial thing. But I've never worked anywhere code reviews worked because no manager wants to lose his best programmer to reviews. So he has some underling do it and you get useless feedback.

Re:Garbage in Garbage Out

[geekoid]

Code reviews are more then that. In fact, they can be run by someone who is just good at running meeting, and still be productive.
Also, the Sr. Coder should really push to be involved, it will save him time in the long run.

If yo think code review is just seeing if the code works, then you should read up on the subject.

Waterfall Vs. Agile

[SirLurksAlot]

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 gotta say the need for code reviews seems a lot stronger in your typical Waterfall shop than it it in the Agile shop.

In the waterfall scenario my experience was everyone worked on their own module and there wasn't much in the way of oversight or another pair of eyes until a formal code review was held. You could get away with writing some really bad code and unless a code review was held on a regular enough basis (and they weren't) it would ship as long as it worked.

In the paired-programming scenario you always have a second pair of eyes to check the code being written, and if your pairing partner is effective they'll stop you when they see a better way of solving the current problem and vice versa. Paired-programming is essentially a continuous code review if it is done properly. Couple this with switching partners often enough and you get a team that thoroughly knows the codebase and can generally make more informed design decisions. Add in TDD and you'll (hopefully) have enough code coverage and quality tests to keep your code clean and maintainable.

Now, I'm not trying to say practicing Agile completely eliminates the need for code reviews, but it lessens that need greatly. It is still entirely possible for a lot of terrible code to get written in an Agile environment, especially if two clueless developers are pairing, but if you're working with clueless developers you'll have that problem no matter if you're on a Waterfall or Agile project.

Re:Waterfall Vs. Agile

[dkleinsc]

Sounds like what you're saying is that agile methods push you towards the same goal as code reviews, just a different way. That makes sense - if I'm paired up to develop, I've got different eyes, different design perspectives, etc.

What TFA is talking about are the kinds of programmers who demand that their code go live without anyone else ever looking at it. That just seems stupid and arrogant at best.

Re:Waterfall Vs. Agile

[MoriT]

Pair-programming is the "extreme" version of code review, so that makes sense. Google certainly doesn't have universal pair-programming. In general, I would hazard a guess that pair-programming is the least commonly employed Agile practice.

Re:Waterfall Vs. Agile

[SnowZero]

FYI: At Google almost everything is developed using mandatory pre-commit line-by-line reviews, but they are of the "informal" style where it's normally just one other developer. Unit tests and regression testing is also pushed pretty hard, though coverage there varies depending on the project.

After seeing reviews catch various horrific bugs, I would probably not work anywhere that didn't have mandatory code reviews for all production code. It also radically improves internal code documentation, and makes sure at least two people have seen any given part of the code.

It depends

[ichthus]

The value of code reviews depends on the motivation behind it. If it's for mutual education, quality control, style conformity, etc., then it's a worthwhile venture. If, however, it's sole purpose is for the jackass-with-a-Doctor's-degree to tell me that I should abandon C and instead do my embedded development in C++, making everything object oriented (among other bloated, touchy-feely bullshit)... you can kiss my ass.

Re:It depends

[JoeMerchant]

the jackass-with-a-Doctor's-degree

I ran a shop with 2 B.S. graduates and 2 Ph.D.s. The main difference between them was that the B.S. graduates could understand and execute simple instructions like "put your name on the top of this form where it says "Name________"". The Ph.D.s pretty much embodied the picture from the Wizard of OZ's degree in "Thinkology" - they'd take a long time to work on a small problem and 90% of the time come up with nothing better than was obvious in the first 5 minutes, often they'd miss the obvious easy kill and end up taking a long time to do something the hard way.

That said, one of the Ph.D.s was pretty sharp, and once in awhile he'd come out with something pretty cool - but it was a long time between gems.

Judging by...

[smooth wombat]

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.

Re:automated testing can let stuff fail but still

[pixelpusher220]

automated testing is primarily for regression testing anyway. GUI / user experience 'testing' is usually after that point, since 'GUI' is just the interface and not the back end logic. Working but looking bad is inherently preferable to looking good but not working....

Departmental standards

[Anonymous Coward]

Heh...we as a department/development group generated a set of coding standards we would all adhere to. It makes reading other people's code easier, and it helps us all to be able to step in and address issues regardless of which module it is, without having to adjust mindset to understand some different set of standards. Yes, newcomers have to adapt to the departmental standard, but it's for the best. After all, all this code is work for hire, not our own personal property.

And no, I had nothing to do with setting the standard, but I heartily endorse it. Most code I encounter freely available on the Internet is pretty poorly formatted, making it so much harder to read and enhance than cleanly and consistently formatted code.

Yes, we sometimes have difficulty with new hires when they get upset that the mandate is to replace their precious tabs with 3 spaces per indentation, but the end result is that the code looks right regardless of the code viewer and platform. This is a significant win when developing across multiple OS platform, using different tools with different default setting for tab stops.

From a time before software tools

[petes_PoV]

Code reviews come from a time when there were few (or no) alternatives for checking that code does what you think it should, as opposed to merely compiling which was often as far as software testing went. It also comes from a time when budgets were huge, software was EXPECTED to take forever to develop and nobody minded (too much) when it did.

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 impractical. The chances are they won't really understand it anyway - so the chances of finding any real, subtle bugs is pretty low. Add onto that code reviews evolved in a time when software always had a printed form: you'd get a line-printer dump, start at the top and work down - whereas with IDEs today, that is rarely the case. In OO coding there are many, many places to "hide" methods and inherited features/functions that for one person to try to disentangle the mess that another has produced is almost impossible.

So code reviews belong to an age that passed about 20 years ago. I recall doing code reviews and having people feel under pressure to find something - just to prove they'd read the code. Nowadays we have much better automated tools that are much cheaper to employ and can do their work overnight, rather than tying up valuable developer time with duplicated effort.

Re:From a time before software tools

[JoeMerchant]

To expect people to find the time in their schedules to try and understand the sofwtare that one of their colleagues has written is impractical.

Funny thing I find: some programmers can get more accomplished in a 30 hour work week than others can in a 200 hour month.

If you spend 5 hours a week developing the social rapport required to actually work "together" instead of in silos, and another 5 hours a week transferring knowledge from the good programmers to the not so good ones (and "good" is not a simple scale, even the "best" programmers have things they can learn from others who might not be as "good" overall), holy crap, you've "wasted" 25% of your standard 40 hour work week.

If you succeed in transfer of knowledge, cross pollination, and all that other crap that many programmers believe they are too good to benefit from, that 25% "waste" of the good guy should more than pay off in improvement of his skills, and the skills of those who are learning from him.

And, the guy that takes 7x as many hours to accomplish the same task, is usually also producing an inferior product in the end. Even if code review just gets his product quality level up without speeding anything up in the future, if the code was really worth writing, the improvement in quality will be worth the 25% additional investment (fewer post delivery bug fixes, earlier opportunity for new business, etc.)

Re:From a time before software tools

[petes_PoV]

Unfortunately it doesn't work like that.

holy crap, you've "wasted" 25% of your standard 40 hour work week

Few people have the luxury to spend 100% of their work-time writing code. They have other duties, such as attending meetings, answering email/administrivia, office compliance (timesheets etc.) reporting progress and updating project documentation, budgets, timescales and a horde of other non-core, but required responsibilities, such as inputting to the next project's design, tracking bugs in previous work and keeping up to date with developments.

Once all of those are taken care of, you're lucky to spend 50% of your working time in front of a screen developing code. Once you require people to do code reviews, then not only do you have all the overheads you mention for THEM to review YOUR code, but you have the reciprocal overheads of YOU reviewing THEIR code. Add on to that, that one single reviewer is not enough - like having 2 clocks: which one is correct?, you need a third leg for the stool, so add another 10 unproductive hours a week.

Bingo! Your 20 productive hours have now become 0. Although by making all your developers too busy with distractions to produce any code, I suppose you do remove the need for code reviews.

Code review pros and cons

[quietwalker]

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 every time he does it. Keep in mind that many aspects of software development are subjective; how to 'best' write a function, or how to architect a component. That means the criticism isn't constrained to bug fixes (valid) or coding style (valid, even if you dislike them) - it's about personal choices.

Also, there's an issue with ownership. We, as developers, often grow attached to a piece of code and no matter how poorly written or convoluted it is, there's still a certain amount of hurt associated with someone else wanting to change it. This is the case even if we just get assigned to maintain some piece of garbage code we don't even want to deal with.

Then add deadlines. We all know that developing bulletproof code is largely a matter of time. Time to analyze, time to review, time spent peer programming instead of solo, time writing documentation, time writing and running tests, etc. There's good reason that functionality comes first and docs come last in most environments, and it's out of the developer's hands. That's another source of stress since you're tripling the load for each submission: first you have to write it, then you have to go over it with another person in fine detail comparable to writing it. That's assuming you believe you write perfect code to begin with, and don't spend longer than normal reformatting and rewriting your code to make the reviewer happy.

Take those added stresses and add them over and over, and tell me that won't change the coder's behavior in some way. In most places I've worked, this usually results in developers picking 'safe' reviewers - people who don't review in depth, people who don't have the time to do a good job, or people who will defer to the original developer due to perceived differences in experience or rank. In some cases, it means that large rewrites are avoided, and hacks put in place instead, because it's easier to get a 30 line hacked-in function successfully reviewed than a 30 file change. It also appears to limit the creation of new functionality to some extent - if you're breaking ground, everyone has a chance to critique your methods. Most seem to find a way to route around 'damage' in the process, especially if you have any zealots on your team.

Personally, I prefer the more objective code analysis and coverage tools, test driven development, and most important, consensus among developers on questions of architecture prior to implementation. Code can be accessed by those wishing to review it after the fact (which, like refactoring, almost never happens), and it can be handled on an informal level if necessary.

As far as ROI is concerned, this seems to be the most effective and efficient mechanism. Perhaps if the user communities did not so easily accept computer/software failures, this would not be the case.

Re:Code review pros and cons

[JoeMerchant]

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 I see that particular form of commenting and bracket placement, I know who to go to for an explanation of "what the hell were you thinking here?"

Re:Code review pros and cons

[Is0m0rph]

Writing software in the semiconductor industry for almost 2 decades I have found reviews to be a waste of time mostly. I never worked in environments where multiple people were working at the same piece of code at the same time. It would have more value in that setting. When you are working by yourself on code and you have to do code reviews I found most of the time is explaining what the code is doing to the manager. The other developers don't care, they have their work to do and really could careless what you are doing unless it affects what they are doing.

Re:Code review pros and cons

[petes_PoV]

Add to that the problem when people are judged by the quality of product (code) they produce. When that quality is measured solely by the other members of their "team" (and by team, I mean direct competitors for promotions, pay rises, plum assignments, cushy training courses, recognition and the corner cubicle) you will very soon have a war on your hands.

You find a bug in MY code - I'll find two in yours. You criticise my variable naming style, I'll tear apart your class design choices. You cause me to work late on a friday night, I'll make you come in at the weekend.

Alternatively, you can get the opposite: I'll scratch your back ....

Code reviews are all very well when there's an infinite amount of time and no pressure to deliver, but today? No ore.

s/busy/lazy/g

[Anonymous Coward]

There, fixed.

Seriously, code reviews don't need to be long, laborious nor overly formal. Code reviews aren't even necessarily about making that particular piece of code better. They're about education and communication. Education as to things that could have been done better (and will be next time), communication as to how and why the code works the way it does (so that more than one person understands it).

If your development team is good, you won't be finding stuff that just doesn't work.

Re:automated testing can let stuff fail but still

[Pieroxy]

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.

I'm not against them

[emuls]

I'm not against them, but find it REALLY hard to find time for them. Code reviewers are busy too, if I'm ready to check in that doesn't mean someone else is ready to code review.

Code reviews need people intimate with the code

[scamper_22]

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 be useful.

Does your company have the excess labor to have skilled people duplicating responsibility looking at code? I hope so... but not so in many companies.

I'm not too good for reviews. But I think they're a waste of time when the people doing the review don't know anything about the code, when timelines aren't taken into consideration...

The secret to effective code reviews

[spiffmastercow]

The secret to code reviewing is very simple: Don't be a Dick (tm). When you use Don't be a Dick (tm) when reviewing code, the reviewee will be more inclined to respect your opinion, to see his code objectively, and to incorporate your advice in the future! Of course, and entire team using Don't be a Dick (tm) will be even more effective! Get Don't be a Dick (tm) now, and start not being a dick today!

Re:The secret to effective code reviews

[JoeMerchant]

The secret to code reviewing is very simple: Don't be a Dick (tm).

So very true, but since virtually all programmers are male....

Re:The secret to effective code reviews

[kakyoin01]

At the same time though, both parties ought to keep an open mind to criticism. The word 'criticism' is too often seen in a negative light (like how 'benefit' is usually seen in a positive light, even though there can be positive and negative benefits).

Re:The secret to effective code reviews

[SETIGuy]

Actually, the urge to be a dick is indicative of a structural problem that affects some software shops. Managers sometimes forget that software should be designed before it is written. And in the design to release process there should be design reviews. Any software problem that makes you want to be a dick should have been fixed in one of the design reviews, before a rookie programmer decides that he's going to write a new associative container in C++ based upon a bubble sort rather than using a standard container.

Re:The secret to effective code reviews

[Anonymous Coward]

Actually, the urge to be a dick is indicative of a structural problem that affects some software shops.

Try: most.

Re:The secret to effective code reviews

[spiffmastercow]

Actually, the urge to be a dick is indicative of a structural problem that affects some software shops. Managers sometimes forget that software should be designed before it is written. And in the design to release process there should be design reviews. Any software problem that makes you want to be a dick should have been fixed in one of the design reviews, before a rookie programmer decides that he's going to write a new associative container in C++ based upon a bubble sort rather than using a standard container.

I should point out that not being a dick is actually a learned skill. It took me years to get it, and I'm still working on it. Second, we've all made the mistake at some point of trying to use our own data structure libraries that we were so proud of in school, and we've all used sub-optimal algorithms at some point. Give the kid a break, calmly explain why he was wrong (on both counts), and get on with your life sans ulcer.

I love code reviews

[JoshDM]

Unfortunately, we stopped having them due to a reduced group size. Also, last time we had it, the arrogant snob being reviewed fought every single optimization suggestion from the team members and didn't put in any. Then-again, due to the eventual group size reduction, I eventually implemented all the optimizations and his code runs much better now!

The problem is...

[Drewmon]

not the coder, but the company. Or at least that's my experience. So many organizations still persist in viewing technology as a black hole of expense and yet, expect miracles to pop out of it with minimal investment. Then when code or systems go live with screwy results, the bitch fest begins that morphs into a witch hunt on "who to blame for this error that's costing us money." Rolling back to the beginning, if the company would: - Entrust those hired to do the job to do their job - Hire a TEAM to make the project effective and less costly (even if a temp hire from a external org) - Manage the business and projects therein properly (no draconian deadlines and unobtainable goals) - Invest in the proper tools to allow teamwork & quality to prosper - Require accountability and teamwork to promote employee & product development Looking at my current employer as an example, we have no reliable & functioning source control (and the developers typically code directly against live servers - no better test than a live one right?), no QA tools or test processing cycle, and no team or project management. The coders want to do the right thing, which is admirable - but they need better tools to deliver the goods. Takes money to invest on the infrastructure. Management isn't willing to go there as it requires money. It's miserable, but when I look out into the market place this is not an uncommon story. Organizations expect IT to deliver and all too often people want to take on the challenge only to realize they are effectively an army of one, ill equipped to do the right thing and forced/expected to deliver. In our environment, if coders could rely on teams equipped with decent tools, the review would be easy & effective. And our products would reach the market faster with better results... But instead, they write & review their own work.

The only real argument against reviews is...

[unwesen]

... time. And it is a good argument in many cases. Code reviews are great, but they make sense only if you provide good feedback from your review, and the original author has time to revisit their code and make changes according to the feedback. I've seen many work environments were such things were considered too expensive (in terms of time).

Re:automated testing can let stuff fail but still

[mcvos]

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.

About Code review

[geekoid]

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 off.
j) It prevents that coders who is in over his head from panicking and being afraid to do anything.
k) It give some level of control to developers
l) It prevents lone coder syndrome.

Since I do a lot of work alone on special projects, I dont' ahve a team. So I go out of my way to find people to review my code.

Because even though I have been doing this for decades, can write circles around most of my peers, I know I make mistakes, I know that being too close means I will over look errors, I understand the tremendus long term benefit of having a coder bring up a styule ot technique I may not be using.

I have seen death march projects get turned around and become successful because of code reviews, I have seen better and more accurate data about scop and time allow coders to get more time.

I have seen people get pissed because other people will be looking at their code. Why? well in my experiences its:
1) They are a lone coder and want control and job security.
2) They don't produce near as much as they imply
3) They don't really grok coding, and their work is a patch work of leaky crap.

Code reviews need to be done correctly. The person running them should be at every meeting, they should understand how to deal with people, then need to maintain a positive meeting.
They also need to teach the coders how to do a positive meeting, and the importance of phrasing questions in a way that isn't negative.

If you through a bunch of coders, force them to come up with changes just to make it 'seem' worth while it wont' work. They're coders, not managers. It's not very likely their skills are very sharp in the organized meeting dept.

If someone starts going on about spacing, tabs, or indenting they had better have a god damn good reason.
A format decisions should be made, but it should be about comments, casing, and naming. Which is important because it need to be constant to aid in maintains, and to aid in discussion about code with people who are new.
One the decision is made. Sat your editor up and the shut up about it.

Code reviews are NOT QA.

ReviewBoard

[Mr. McGibby]

If you use some software, then the process is MUCH easier. http://www.reviewboard.org/

Re:ReviewBoard

[Chelloveck]

This. It's not perfect and I'd certainly like to see some other products in this arena, but Review Board gets the job done without introducing a lot of extra pain just fighting the tool.

At my current gig we review most significant changes before committing them to trunk. We do that informally via Review Board. Reviewers read the code at their convenience and leave comments; we don't need to schedule time when everyone can get together. The comments are usually pretty useful and range from "Did you consider...?" to "You missed this corner case." to "OMG, that's going to completely break the interaction with this other module!" There's no question, it's worthwhile.

I've been at places doing big, formal, gather-everyone-in-the-conference-room reviews. One place we sat down with printouts and read through it line by line. ALL comments were written down and classified, and we had to get pen-on-paper signatures from all reviewers okaying the changes before the code could be checked in. That's a serious waste of time.

Re:ReviewBoard

[Anonymous Coward]

Code Collaborator by Smart Bear Software (http://smartbear.com/) completely rocks.

Everybody's code I review has the same flaw

[paulsnx2]

I didn't write it.

Good, the Bad and the Ugly

[npsimons]

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.

Re:Good, the Bad and the Ugly

[Yogs]

Knuth lost his wager how many times?
Of course there should be reviews.

And yes, in the short term, it would benefit the reviewer more than the reviewed. Does that make it a bad thing?
Think about organizations with highly centralized expertise and trust vs. ones where it's more distributed.
Which are better places to work, keep better people and do better over time?
If Google doesn't they should start making him do code reviews.

Re:Good, the Bad and the Ugly

[Anonymous Coward]

How very mature. Fuck that. I'll keep my ego. I earned it by not writing shitty buggy code. I test the fuck out of everything I do until there's nothing left. These twinklenuts code reviewers can suck my dick.

Re:Good, the Bad and the Ugly

[snowgirl]

Knuth lost his wager how many times?

Wikipedia says TeX is on version 3.1415926, he released two version before moving on to pi, so 7 plus 2, 9 bug fixes in TeX...

So, yes, even the God of Programming needs code reviews, and in fact, understood the benefits of code review so much that he put money forward to get people to find them. :)

Customers don't demand quality

[t'mbert]

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.

I've reflected on this problem quite a bit, and I can't seem to get past "Customers are getting what they are demanding," which is to say "not much."

None of the contracts or RFP's I see are demanding performance measures, quality measures, detailed functional requirements, etc. Nor do I see customers diving into the product and really trying it out before they buy, or comparing the competing products hands-on. Our customers purchase based on limited demos run by the vendors themselves, and those making the purchasing decisions don't have enough experience to thoroughly evaluate a product, nor do they seek the expertise of those who can. In the end, purchase decisions are made with very weak product knowledge, and create contracts that are weak on details (but strong on delivery dates).

This results in frustration all around. Vendors have to deliver what the customer wants, but can't figure out what they want until after they buy it, try it, and then complain it doesn't work they way they want. Customers unwittingly purchase vaporware from vendors. The development teams are asked to deliver functionality with little or no guidelines. Target dates for delivery are off by months. In my opinion, this occurs simply because the customer wasn't clear on what they wanted, didn't make those demands clear in their contract, and didn't make a knowledgeable purchase decision.

Which leads to no time for testing, compressed timelines, no ammunition to do it right, no clear budget or tracking of the costs, and on and on.

At least in my field.

Victimization, vulnerability, trust issues

[dpbsmith]

I had a manager who stated that he was introducing regular code reviews. Well, actually, he stated that he wanted to conduct regular code reviews. Not knowing too much about them, I dug up a copy of Freedman and Weinberg's "The Handbook of Walkthroughs, Inspections, and Technical Reviews," and saw that they spell out--from memory, but I'm sure others will correct me if I'm mistaken--that management should not participate in code reviews and that code reviews should never be used for employee evaluation. I pointed this out to him and he said coldly that he was going to conduct code reviews. (In the event, of course, it didn't mattered. We had one meeting, we wasted some time in unproductive discussions of different people expressing their personal coding tastes and the manager stating his approval/disapproval of same, and somehow the manager never found the time to conduct another).

Any employee should be justifiably leery of walking into a "code review" situation, much as any employee should be cautious in taking advantage of HR "Employee Assistance Programs."

I've been in other "code review" situations that deteriorated into edgy negative criticism. Unfortunately, the degree of collegiality in any situation is that of its least collegial member. It takes an awfully good team to have the degree of companionship and trust to do something like that properly.

It's all well and good to say "well, your bad experiences weren't truly code reviews," and my recollection is that Freedman and Weinberg offer numerous suggestions to make them work... but I can only report my own experience.

And while I've been in good teams, teams that I think could have conducted productive code reviews, the parts of the project we were working on were so different--different computer languages and different development systems--that I doubt we could have given all that much help to each other. If we'd been given the time. Which we weren't.

Test at earliest possible moment

[perpenso]

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:Test at earliest possible moment

[maxwell demon]

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.

"OK, so we have now our first code review session. So far we have written only

int main()
{
}

Any comments on this?"

Re:Test at earliest possible moment

[perpenso]

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.

"OK, so we have now our first code review session. So far we have written only

int main() { }

Any comments on this?"

Yes, you forgot the exit code. See, code reviews help. :-)

Re:Test at earliest possible moment

[maxwell demon]

I was using C++. In C++, main has an implicit return 0;.

Re:Test at earliest possible moment

[perpenso]

I was using C++. In C++, main has an implicit return 0;.

Which should eventually be replaced with an explicit return indicating the exit state of the program. So I think the code review holds. :-)

Re:Depends on the people

[b4dc0d3r]

What code reviews miss is, only the coder gave real thought to the solution / implementation. Code reviews can miss very important things, because the readers are not (always) considering the same things the author did.

I see generalizations all over the place when it really depends on the people. When you have a weak link, code reviews catch problems. But only if you have someone good enough to *read* code, if not write it. When you have an all-star team, everyone gets to bring the others up to speed on the code changes, but an all-star team also should be able to read the code for themselves without hand-holding.

Instead of code reviews, I'm developing the idea that two people should write code independently, and then review each other's results. What one person misses might be found with another take on the implementation. Even if they come up with the same thing, you've validated that the solution appears to be correct.

This is not team programming, it is not code review, it is duplication of effort, which is expensive. But if you want good results, expensive once is better than cheap twice or more. This eliminates the major issues behind code reviews:

1) Code writers are not always good readers
2) A given solution may miss edge cases
3) Reviewers don't dedicate as much thought as writers

sometimes a good thing

[Jodka]

Code reviews should be your lowest priority. They are the least efficient way of improving the quality of your software, in terms of hours invested vs. improvement in user experience. You are way better off entering feedback from actual customers, testers and UI designers, into an issue tracking system, prioritizing the issues, and working them down in order or priority.

Some supporters of code reviews advocate that they be run as a background task, during what is otherwise programmer idle time. If you are going to do them, that's probably the right way.

You can do good code reviews all day long and your software can remain unusable junk. You need to close the loop, examine how the the actual software works in the field and feed that back into work on the source code. Code reviews short-circuit the process by ignoring the actual performance of the software, instead using the quality of the source code as the measure of correctness. Users don't care about the correctness of your source code as gauged by comparison to a specification. They care about whether the software does stuff that they want it to do and is easy to use.

Code reviews are sometimes only a way for managers to cover their assess. In many organizations managerial competence is defined as adherence to procedures as documented by completed forms. When something blows up, the only artifacts investigators concern themselves with are documents defining procedures and written records documenting that the procedures defined in those documents were followed. Intelligence, good judgement, and actually producing a usable product are irrelevant in such environments. Records of code reviews are an artifact that managers can point to demonstrating their success in following a procedure. Code reviews are really big in regulated environments and especially on government funded projects. Unlike business which needs to sell stuff to actual people to survive, government projects are not required to ever furnish usable products and often do not.

All that said, code reviews do have some advantages. Programmers learn a lot from looking at each other's code and sometimes you have to push them to do that. And it does catch bugs, though the work in finding those is not prioritized by their impact on users, as it should be and is if you prioritize according to user feedback. And if you need highly reliable code, then you need to employ every possible resource for finding bugs, not just the fastest and most efficient, and code reviews are another one of those tools in your kit and you should use them.

Re:sometimes a good thing

[shutdown -p now]

Programmers learn a lot from looking at each other's code and sometimes you have to push them to do that.

Arguably, that is actually the main point of code reviews, not catching bugs. On large projects, people do tend to "burrow down" in their own area responsibility and rarely peek out; if you don't force some change there, you end up with one or two people being the only ones that understand a particular component or area of code. And then, when they go on vacation or leave or change teams or whatever, you get blocked when something needs to be done there.

Re:sometimes a good thing

[Your.Master]

You are way better off entering feedback from actual customers, testers and UI designers, into an issue tracking system, prioritizing the issues, and working them down in order or priority.

By the time your customers are telling you your code is crap, a fix is usually several orders of magnitude more difficult -- and it's already burned your customer! I'm curious what sort of software you write that gives you this opinion -- maybe you ship broadly on a daily basis?

Testers are a valid way to find bugs, but they essentially work on a black box (if they aren't, then they are code reviewers...), which can make it difficult to see certain classes of problems.

As for UI designers, they are pretty much orthogonal to the concept of code reviews.

Code reviews short-circuit the process by ignoring the actual performance of the software

If your code reviews ignore the actual performance of the software, you're doing poor code reviews. It's almost never a good idea to do something poorly; the question of whether code reviews should be used should be read as, should decent code reviews be used. Of course you could argue that decent code reviews are impossible, but you don't seem to be arguing that.

instead using the quality of the source code as the measure of correctness.

The quality of the source code has a very obvious relationship to maintainability. Security is also something that just can't be put off until a user finds it. I can see your argument about robustness (crashes and hangs), that code reviews prioritize the issues you can see whether or not they are common issues -- but really this is the time when fixing them is easiest.

See, priority shouldn't just be about user impact of each bug. It should be user impact divided by the effort required to fix it -- this allows you to give the greatest user impact in aggregate per unit time. But of course, that's difficult to measure.

At the one extreme, bugs from users are prioritized by user impact alone because before investigation you don't even know how much effort is required to fix it (you can figure that out by sinking some time into investigating it).

Code reviews are at the opposite extreme -- it's prioritizing what's easy to fix when it's easiest to fix it, shrinking the denominator. However the user impact is somewhat hypothetical at this point. That said, you can often figure out that something is obviously going to be a frequent problem, eg. a narrow race condition that leads to heap corruption will be a pain in the ass to track down, probably relatively low hitting, with relatively high consequences including security problems. Similarly, a flaw in how text is localized will effect every customer who speaks languages A, B, C, & D.

Users don't care about the correctness of your source code as gauged by comparison to a specification.

You're talking about UI design again, it looks like. That's not the main use case for code reviews. Unless you're talking about a spec as in W3C or OpenDocument or something, in which case, your customers most definitely care.

What goes around, comes around

[seniorcoder]

I regard it that the reviewer is trying to do me a favor. As such, it is my job to return the favor by reviewing some of his/her code. This role reversal typically keeps reviews sensible without too much arrogance on the part of the reviewee and without too much intransigence on the part of the reviewer.

Yes, I am!

[Anonymous Coward]

Time should be used for more productive things than looking at the already perfect code I wrote before.

Code Reviews tend to focus on trivia...

[paulsnx2]

I used to have code reviews at Microsoft. All we did was argue about the proper Hungarian notation. I understand they have given up on that (Thank Heavens) but this illustrates why code reviews can be so bad. They can degenerate into a group search down a checklist for "technical" flaws in the way code is written:

Where did you put your braces? Where are your spaces? Why did you name that variable that?

It is much harder to get (especially less geeky) people to think through the algorithms, and suggest better approaches. I have looked at code (passing code reviews) that performed quick sorts on small arrays so they could just pick up the Minimum value or Maximum value as the first element in the array. How utterly stupid is that? One pass through the array can pick up a Min or Max value O(n) as opposed to Quicksort O(n^2). A code review should pan that sort of behavior in a minute, and yet....

The more geeky among us have their own list of flaws they expect to see in the code of others. Finding these might be *more* useful, but at times also blinds the review for seeing other bigger, more difficult and subtle issues.

Sadly,formating rules, variable names, number of comments, comment formats, etc. etc. and checklists of pet issues are the easiest things to spot in a group scan of the code. The urge to find something and find many things in a group sometimes overrides the need to look past these little things and understand the code at hand, and offer feedback that is really targeted to the problem the code is trying to solve.

Re:Code Reviews tend to focus on trivia...

[Javagator]

Sadly,formating rules, variable names, number of comments, comment formats, etc. etc. and checklists of pet issues are the easiest things to spot in a group scan of the code.

I had the same experience. No one was willing to spend the substantial amount of time necessary to do an in depth code review, so the only things we found were minor syntactic things.

Yes, I AM too good for code reviews!

[Theovon]

It's not that I mind people looking at my code. In fact, I'd love to show off my skills, if they could appreciate my cleverness. And I wouldn't mind some feedback, if other people understood what I was doing well enough to make creative comments. It's not that other people aren't as smart as me; it's just that we're good at different things, and coding seems to be an area where I excel, better than a lot of engineers I know, and better than the vast majority of academics who specialize in theory instead of practical coding. When I have had people look at my code, typically, I get no feedback at all, and occasionally, I'll get someone making stupid remarks about not enough commenting. In fact, I know that I don't comment enough, and I DO NOT believe that it's easy to make self-documenting code. It's just that their remarks would be vague or demonstrate a general misunderstanding of something more fundamental than the lines of code I write. (E.g. that my nlogn algorithm is bigger and harder to read than an n-squared algorithm.)

You're going to get resistance to code reviews from both ends of the spectrum. The lousy coders know they suck and don't want to be caught. It amazes me how some people can't even freaking indent their code, just to make it half-way readable. Their code looks like viagra spam, with random line breaks, extraneous spacing, and everything. The really good coders don't want to be reviewed because it goes over the heads of the typical reviewers.

This is WHY they resist. But the fact is, I think that if we had a culture of code review, it would be good for everyone. The people at the top would get used to explaining themselves better, with the side-effect that other people might learn something. The coders at the bottom would be forced to at least format their code well, and it would force them to think harder about what they write, because stupidity is going to be caught quickly.

And coders of all skill levels are guilty of just hacking crap together sometimes. Just hacking isn't good design practice. Maybe, for someone with an IQ of 150, it's not such a big deal to make solid working code without advance planning, but good design will make it possible for some mere mortal to maintain that code later, even if it's less clever than you could do.

Something that MOST engineers fail to realize is that the world does not revolve around their code. They are doing a job, which contributes to some larger product, and that product is what fills their paycheck. The lines of code you write are almost valueless in and of themselves, because if you weren't there, someone else would do it. If you're a top, well-respected engineer, it's not because you write clever code. It's because you're reliable at churning out CORRECT code that completes products. You are judged on how well you contribute to the bottom line and how many bugs you introduce into end products. Now, maybe you get some satisfaction out of making this or that more efficient or better designed. (I'll do some clever coding tricks because it's less boring, but mostly I spend my brain cycles coming up with better organization that makes bugs harder to introduce, which is actually useful.) Unfortuantely, any competitive edge clever coding leads to will be marginal at best. With today's computers, there are only certain places where cleverness and efficiency really matter, and those are limited to system software and compute-intensive algorithms. Linux and Photoshop have to be smartly coded. Microsoft Word just has to work correctly. Why do you think Apple took so damn long to make Finder snappy? Because it just doesn't matter more than 1% of the time. They spent their resources in more important areas.

I WANT code reviews

[cervo]

I have always worked in places that do not work as a team and do not do code reviews. No one cares about software quality, they just want it done and to go home. Code reviews would be a great way to learn things from more experienced developers. IT would be a great opportunity to learn small tips for improving the overall code quality as well as pointing out flaws so I would know what to work on.....

Instead, I had it once and mostly the person just used it to complain about capitalization in SQL Code and a few other punctuation things. No tips for improving the maintenance, making things cleaner, etc. were given. On actual object oriented code I have never been reviewed. Which is sad really because there is much that could be learned about object oriented design. After all if no one is there to say this code is total crap, or fundamentally flawed....it ends up as production code....sorry.

My experience: not worth the time

[Anonymous Coward]

At my previous job we used to do a lot of code reviews, and they didn't catch squat. Features are usually sufficiently complex that the only thing code reviews did catch were stylistic things, and haggling over minute approaches to problems that ultimately had no impact on the project; there were several instances where we found a bug after launch and it was in a piece of code specifically covered in a code review. Also usually when we had a one or two hour code review we'd have at least three or four other software developers in the room - think of how much time that code review took up. If you believe in code reviews, then a more sane approach is pair programming.

Re:My experience: not worth the time

[Todd Knarr]

That's the same thing I've found: to be useful you have to review such a big chunk of code that you can't do it in a reasonable amount of time. So the review ends up focusing on either stylistic crud like the exact conventions for whitespace and naming, or it focuses on things like should you use a for or a while loop there or whether you should use indexing or an iterator on a container, stuff like that. The big philosophical things that'd make a huge difference, like how functionality should be factored out, what support functions ought to be present in various classes and is all the groundwork needed for future expansion present, all get ignored because they start dragging in too much code and it takes too long for people to get their heads around why it was done the way it was in the first place (which you've got to grok before you can say whether it's right or not).

IMO the better way to do it is to team people up to do the work, and shuffle the teams around so everyone gets to work on different code with different people for different assignments. Problem here is again management, who don't like needing two programmers working on one bit of functionality when it only takes one and the other could be producing something else.

Expense is the thing...

[SETIGuy]

I don't see the programmers as the impediment to code reviews. Projects are often not structured in a way that code reviews are easy to do. Suppose you a small project with 5 programmers. Programmer A has a big chunk of code to be reviewed, so B-E spend a day looking it over, and then half a day in a meeting to discuss changes and fixes. That's 6.5 person days. Multiply by 5 for reviewing the other guys code, and you've got 32.5 person days. A bit more than a man month spent reviewing code. $10k. Now how many times a year do you need to do it? To the average PHB, reviewing code is unproductive time. "Bugs will be found in beta, or after we ship" he'll say. "It's not going to kill anyone. Now get back to work on the brake system software."

Now try a cross project review. What does your boss say when you tell him another project wants you to spend a day and a half reviewing code?

Re:automated testing can let stuff fail but still

[Anonymous Coward]

it is a good place to put no regression: as user and tester discover ui bugs, capture the flow in code and make it a test case.

after a while, when more and more subtler bugs are discovered, you'll develop a nice suite of automated no regression tests.

also, if you can, make your analysts write some test case about the effect of the stuff they want (this mostly applies to largish projects)

automated test is pretty useless if done by the same programmers that write the code, but extremely valuable if done right

Re:automated testing can let stuff fail but still

[Surt]

Why don't your automated tests test the GUI? Not testing the UI with automation is crazy.

Automated testing is important

[perpenso]

... automatic testing is a joke, it just test something you already prove works in the automatic setting and generates totally, totally useless reports which tell nothing of the state of the actual project ...

Sometimes changes inadvertently break previously working code. Some testing is beyond boring and is best automated if possible. For example I have a calculator application, Perpenso Calc for iPhone, and its regression testing checks the results of all operations, formatting options, etc. A human can only stand so much verification of 64 bit bitwise operations, 20 digit complex number operations, time value of money calculations, etc. The human time is better spent exercising the user interface in this case, and letting the machine do most (not all) of the numerical accuracy checks.

OK that was a pretty special case where automation is an obvious fit but automation works in other areas. Gaming for example, consider a real time strategy game's AI. An automated test might create a squad on squad battle, repeat 1,000 time, and compare the results to expected or desired results. This sort of thing can be particularly useful when balancing units, where the changes are not in code but rather in data being tweaked by designers. Of course none of this replaces good old fashioned beta testing, but it actually make for more valuable beta testing that focuses on the edge cases and not so much on the basics.

No problem, but who has the time?

[Sloppy]

Open Source is pretty much the only way to get code reviews for free (and even then, it's not guaranteed). I don't have time to review other peoples' code around here either, except when I already have some other reason to believe there's a problem. And even then sometimes I have to mutter "if it ain't broke, don't fix it."

Believe me, code reviews are only one of a number of Good Ideas that turn additional human time into better code, all of which would be no-brainers if only time were free. It's all just a question of whether or not it's worth it, and that often depends on how critical the code is, in the first place.

My website or sales analysis report or whatever, can't justify expenses quite as well as your avionics system. OTOH it might be more important than your flightsim game's avionics.

Samba slowly moving to mandatory code reviews.

[Jeremy Allison - Sam]

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.

Re:automated testing can let stuff fail but still

[dkleinsc]

Working but looking bad is inherently preferable to looking good but not working....

... unless you're dealing with a salesperson, in which case they'll just take the snazzy-looking-but-broken software and present it in a way to hide its brokenness.

Solo developer monoculture

[HeckRuler]

I've more or less been a solo developer since I left college 5 years ago. My internship had two other developers, but I was given my own project. The first job had co-workers, and there was interaction, but by and far I had my projects and they had theirs. Occasionally they interacted and we had to talk about that. And in my most recent job I am the only person with a scrap of programming logic in the entire company.

So I'm kinda worried that I'm developing bad habits by growing in a monoculture of just me. It could be simple mistakes like knowing when to use a certain pattern, or large mistakes like thinking that making VBA tools are a good idea. But mostly I worry about what I just don't know about because I'm only one guy.

I'd really like to have a code review, but I can't release company code out there for anybody to see, and my own personal projects are... well... sloppy to say the least.

Re:Solo developer monoculture

[OddJobBob]

Wanting to get your code reviewed is a good first step. Is you code clean enough to explain to a non-coder? Maybe find someone in the company who while not a coder has expressed an interest in it.

For personal projects it is easy to not apply the same practices you would at work and become sloppy, just do it as you would a professional project. Get unit testing in right from the start, learn about refactoring, take a while off from it and look at it again afresh, if you feel a pattern is wrong or not applicable you're probably right so strip it out and use another. Maybe publish your work somewhere, get involved in forums where you can get advice and also give it. Learn to accept criticism - this is a tough one for most people - me included.

If all your developers were Ken Thompson...

[Above]

I came here to say this another way around, if all of your developers were Ken Thompson level folks, you wouldn't need formal code reviews.

Excellent developers know what they are doing, research alternatives, run tests, simulations, and performance analysis, and submit awesome code. Someone reviewing it is unlikely to make improvements by reading it, but rather would have to study the problem in depth to improve a solution.

If your reviewer can simply read the code and point out errors or problems you probably should fire the person who wrote it. If they aren't good enough, and can't do a complete enough job to pass a visual inspection you have no chance of turning out high quality software.

Code reviews should be less frequent, and more in depth. Have a programmer present on a section of code after he's figured out all the details. Describe the alternatives he investigated, and why this worked so well. After an hour presentation then his peers might be up to speed enough to make substantial comments on the code.

Re:If all your developers were Ken Thompson...

[rokkaku]

Meh, folks should also realize that Ken Thompson is almost certainly writing proof-of-concept research code, where it would be pointless to get a code review. He's trying to get a demo working to show that his idea works, then he'll throw it over the wall and move on. He'll get his code review when somebody else builds on his code to make a production-quality product.

Re:If all your developers were Ken Thompson...

[MoriT]

Ken Thompson didn't need source control either.

How often do you release code? My projects take anywhere from half a day to three weeks, and I am expected to have self-documenting code. "Presenting" code would probably have taken nearly as long as some whole projects, and an hour is far longer than most of us ever spend on a code review.

I'm assuming part of the issue is how we are defining "code review". Day-long, or even hour-long, in-person conferences would never fit into our workflow. Online, collaborative code reviews do, and can be done well in maker's time.

If code reviews are good...

[hsthompson69]

...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.

Re:If code reviews are good...

[Anonymous Coward]

Hating paired programming has nothing to do with being antisocial. It is simply a retarded waste of time. Code review are a must, but if your coders need someone else sitting there holding their hand, fire them.

Re:If code reviews are good...

[Anonymous Coward]

I'm struggling with "asymmetry" in pair programming. How long should it take before my peers don't suck so much?

Re:If code reviews are good...

[Anonymous Coward]

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 ...

... bad advice.

"I don't know anything about the problem domain or the customers, so here's my solution that completely destroys scalability and makes it completely unsuitable for processes A B and C all of which I don't know anything about"

"I don't know anything about the problem domain or the solution space, which is why I wasn't assigned to work on it, and you were instead, but here's my completely useless advice anyway" or almost equally bad "We're just stamping license plates here, looks like you used the standard solution and got the standard results, now lets pretend I/we have any concern with the standard solution and/or pretend I/we have any voice in the advantages or disadvantages of the standard solution."

Another awesome one is turning the reviews into a reality TV show. You are a friend of my enemy, so I say your code sucks. You are not part of my social group, so your code sucks. And of course the opposite, that WoW raid we went on last night was awesome, I'm sure this is great code.

All you need to do if find the Venn diagram of someone close enough to be useful and far enough away not to socially matter, in other words, pretty much no one.

Re:If code reviews are good...

[Anonymous Coward]

On my current project the team worked really well together and we incidentally happened to start doing a fair amount of pair programming -- not a ton, maybe 10-15% of the time. There's no mandate about it, but when someone gets coder's block their first instinct is to ask someone else to sit down and help get through the problem. It has worked very well. On those tough sections, having two pairs of eyes results in much better code, and often educates both parties.

Re:If code reviews are good...

[hsthompson69]

Well, it takes, what, about 10,000 hours to become a master at something? Some of us started programming before hitting our teens, so 10,000 hours happened a long time ago, but figure someone fresh out of college? 5 years if they dedicate 40 hours a week to it, figure more realistic 20 years given that they'll only spend 10 hours a week doing it with any sort of focus juggling meetings and status reports.

Re:If code reviews are good...

[TheTyrannyOfForcedRe]

...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).

What the hell is wrong with people in the software industry? In any other industry you'd be laughed out of the room for promoting this hokum. If pair programming is such a great idea then why not pair lawyering, pair bus driving, pair accounting, etc. Before anyone mentions that lawyers and accounts often do work in teams, remember that working in teams and the "pair" concept are entirely different beasts.

Re:If code reviews are good...

[Anonymous Coward]

Pair programming is certainly great. It's a great way to lose many very skilled people who are too self-conscious to deal with having someone see their every typing mistake or on-screen scratch-padding. It's a great way to turn every coding session into a bicker-fest. It's a great way to kill employee morale because half of your developers have nothing to do but watch someone else work.

Re:If code reviews are good...

[Anonymous Coward]

The only thing pair programming accomplishes is to put two people on an inherently one-person task.

Re:If code reviews are good...

[Anonymous Coward]

If [eating, sleeping, pooping] is good... ...then continuous [eating, sleeping, pooping] is great?

Cost/benefit functions are very rarely linear.

Re:If code reviews are good...

[n8r0n]

Why are continuous code reviews great? Is a flood great because water is good?

People who don't like pair programming aren't merely antisocial, as you contend. There are very legitimate reasons why pair programming may be a bad idea. For one, logistics. Having pairs requires people to work the same hours, sit in very close proximity to one another, get interrupted when someone else needs to use the bathroom, it works poorly with telecommuting, etc, etc. There's something uniquely human about needing some space to work and think, that pair programming does not allow.

It's also pretty established fact that knowledge work (for many people) requires concentration, and the absence of interruption. Pair programming is horrible from this standpoint. You are constantly having thoughts, and then getting interrupted when your pair has a thought they want to share. You'll basically be coding, and trying to hold a conversation at the same time. Concurrency like that isn't helpful for many computer programs, and it's even less helpful for most human brains. It's like context switching on the order of seconds, rather than having an hour or so to work in peace and quiet, and then conversing with a coworker only on demand.

Personality also has an impact. If you have a dominant personality (driver), you'll often have them doing most of the actual coding, with the other person just serving as their typist (which is slower than if the person thinking just typed it themselves). The passive personality may be having trouble coming up with good ideas on their own, because their mind is being constantly re-routed by the aggressive personality.

You can argue all you want that impracticalities like the ones I've mentioned are just indications that the pair programmers need to get better at pair programming. But, that's crap. A good system is one that capitalizes on the way people actually are, not one that forces people to work in unnatural ways.

Re:If code reviews are good...

[hsthompson69]

Having pairs requires people to work the same hours, sit in very close proximity to one another, get interrupted when someone else needs to use the bathroom, it works poorly with telecommuting, etc, etc. There's something uniquely human about needing some space to work and think, that pair programming does not allow.

All of those seem like textbook anti-social issues :)

Look, I know about the anti-social programming zone. I get it. I'm good at it myself. But to assert that pair-programming is bad because it is somehow "unnatural" seems somehow out of place when we're talking about probably the most unnatural practice in the history of human kind, working with technology that is as unnatural as one can possibly be :)

Now maybe pair programming is too hard for you. Maybe it will *always* be too hard for you. I get it. Not everyone can be a rock star, and not everyone can be a brain surgeon either. Maybe you've got enough chops to make up for it. But in terms of code review, a continuous code review, during programming, is going to be more useful and efficient than a drive-by "curly braces should be on the same line" formal code review.

Re:If code reviews are good...

[hsthompson69]

Well, suppose you only ate, slept or pooped once every quarter at the same time you had your formal code review :)

I'd much rather do it every day, what about you? :) Perhaps not technically "continuous", but you get the point :)

Re:If code reviews are good...

[hsthompson69]

And a code review of 5 people on one persons code puts 6 people on an inherently one-person task. I'll go for the cheaper alternative, thank you :)

Re:If code reviews are good...

[hsthompson69]

There's always a wrong way to do everything, even pair programming :)

While there are going to be the anti-social archetypes that can't stand it, it's a valuable practice to learn, and a useful tool to have in your bag of tricks. In any case, it's better than your drive-by formal code review when it comes to actually helping improve code.

Re:If code reviews are good...

[hsthompson69]

Industries, since time immemorial, have had master, journeyman and apprentice roles, which involved a *bunch* of pair work on all kinds of stuff. To think that the concept of pair programming is so offensive as to be laughable shows a lack of imagination, really.

Would you want to fly in a plane that didn't have paired pilots? :)

what about maintainability?

[Chirs]

The fact that the code passes the tests says absolutely nothing about the quality of the code or how maintainable it is. If it passes the tests but is a huge mess of spaghetti then it likely needs to be rewritten properly.

Maybe it's a modification to an upstream project and you want to pass back the changes...then you need to make sure that the changes are written in a manner compatible with the uptream project. You need a code inspection for that.

Maybe there's a corner case that was missed in the testing but is visible in the code inspection.

Maybe there's a race condition that wasn't hit in testing but will be visible in the field once you're running in a thousand sites.

Re:what about maintainability?

[Anonymous Coward]

The fact that the code passes the tests says absolutely nothing about the quality of the code or how maintainable it is. If it passes the tests but is a huge mess of spaghetti then it likely needs to be rewritten properly.

The huge mess of spaghetti is a concern because the tests are likely missing some flaws, but if the tests are adequate and the specs are not changing, I say let the spaghetti fossilize until the specs do change.

If the code really is that bad, when the new specs roll around, it might be better to chuck it and start over - code reviews are also a good place to determine that.

I have also worked on quite a few projects where the final code freeze really was final, no sense making something maintainable if the hardware is going to get completely revamped during the next product cycle.

Reviews serve some purpose

[shellster_dude]

Where I work, each project gets a formal code review, and after that a security review. While it definitely serves as a good thing for new developers, and a once-in-a-while review to keep developers honest, it's a waste of time to do one on every project. It takes time and money and after the first couple reviews, the developer should be learning from their mistakes and not making them anymore.

Shitty code can come up with the right answer

[perpenso]

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 related uses. Again, perhaps you are the exception but there is a strong correlation between mediocre code and the perspective offered.

Similar to the above, there is a high correlation with delusional overconfidence and statements like "I've never come to appreciate something as ridiculous as 'company-wide' ANYTHING. You don't hire me for my ability to conform!". I've had the pleasure of working on teams with incredibly talented and experienced professionals and their attitude is quite the opposite. For example they understood the need to have an agreed upon coding standard to make things more readable to the group, to minimize diffs that can be bloated by personal formatting tastes, etc. Personally preferences were prevalent when coming up with the coding standard for the team, blood was nearly spilled :-), but once negotiated and agreed upon everyone went with it. As for code reviews these people liked them. Highly talented people often like to learn from other highly talented people. Again, perhaps you are the exception, but the attitude offered is more commonly associated with those of lesser talents.

In short, shitty code can come up with the right answer. So arguing that your code comes up with the right answer is not necessarily informative. Once again, having not reviewed your code I can't speak towards your specific talents. I think myself and the other poster are saying that your comments seem to fit a pattern that we and many others have seen before. If you are truly an exception you might want to rephrase your comments to not fit this well established pattern.

Re:Shitty code can come up with the right answer

[Doctor Faustus]

they understood the need to have an agreed upon coding standard to make things more readable to the group

I've taken over maintenance of an awful lot of old programs, and I've never had a case where I thought I would have been helped by anything I've ever seen in any coding standards.

Reviews, on the other hand, might catch things like variable names that clash with what's actually going on, not trusting error or transaction systems, writing code that duplicates database constraints, wrong comments, etc.

Re:Shitty code can come up with the right answer

[perpenso]

they understood the need to have an agreed upon coding standard to make things more readable to the group

I've taken over maintenance of an awful lot of old programs, and I've never had a case where I thought I would have been helped by anything I've ever seen in any coding standards.

The coding standard was more of a help during development. The idea was the dev team's. After two multiyear projects without any standards they decided they needed some for the next multiyear project.

Reviews, on the other hand, might catch things like variable names that clash with what's actually going on, not trusting error or transaction systems, writing code that duplicates database constraints, wrong comments, etc.

For this team part of the review process involved looking at diffs from the last release version. Their experience from earlier projects where anything goes led to bloated diffs with cosmetic changes, hiding real changes among a bunch of cosmetic changes. Just one of various reasons the *team* decided it needed a standard the next time around.

Re:Shitty code can come up with the right answer

[frank_adrian314159]

Amen. The folks on my team range from seven to twenty-five years of experience. They argue about some process practices, but code review is not one of them. They see the value of making sure that code they might well be working on someday is rigorously checked and they've found enough logical errors (usually of omission) to understand that this is a worthwhile practice.

Asynchronous reviewing systems help

[pumpkinheadgiant]

Asynchronous reviewing software systems really help. I've found that when things get busy, they get busy for different people at different times, and it can be hard to get the full value of somebody's attention when they're in the pressure cooker.

I really enjoyed using Code Collaborator on a recent project, as the flow was more compatible with everybody's schedule throughout the project. I could do a commit, select a reviewer (or reviewers), and an email would be generated with a link. When the reviewer HAS TIME, they can log in to the system, see what the review queue has for them, and let them get to the meat of the differences, leaving comments/questions in the system as meta-data (NOT the all-too-standard in-the-code /* what is this never initialized?!?! */ comment). When I in turn log in (when I HAVE TIME), I can see this feedback, and if I comment further, the dialog is scoped right at the change. If I do a follow-up commit, Code Collaborator was good at maintaining the dialog for the new differences...

In any case, I found the system to be valuable, and a lot of it seemed to be the asynchronous nature of it. Not having to schedule meetings was key.

Re:Asynchronous reviewing systems help

[shutdown -p now]

Hm, I haven't even realized that someone could consider code reviews as "having to schedule meetings" and all that. All my experience has been either with automated tools (I also had some experience with CC, and it was indeed a good one; these days, I use a internal tool which is broadly similar feature-wise), or at least emails with diffs, and it was never something with hard schedules and such. I can understand why people would push back on code reviewing if they have to schedule time specifically for that in the middle of their workday, rather than whenever they have some free cycles to spare.

have to disagree

[Chirs]

I've found race conditions, synchronization bugs, corner cases, opportunities for performance improvement, etc. while reviewing other people's code. Occasionally others have found the same for my code as well.

While they *can* become nitpick sessions, they can also be very useful.

Code review is like...

[BaldBass]

A) taking you out naked in the public and publicly discussing the shortcomings of your body.

B) tasting the dish you've just cooked with a group of friends.

C) flossing your teeth after each meal.

Pick one.

Re:Code review is like...

[Anonymous Coward]

You should have used numbers instead of letters for your options.

You should have capitalized the first letter of each option.

gd&r

The attitude of too many developers...

[Anonymous Coward]

"Nobody is too good for code reviews... except me".

Re:And there is always the guy who never comments

[Anonymous Coward]

if the code is so difficult to determine what it is doing without comments - then that is a good sign you are doing it wrong. It isn't unheard of for people to read the comments and that blinds them to code doing something stupid.

Comments should be telling you something you couldn't easily workout from the code. Comments telling you how/what the code is doing means you have to read comments, compare the comments and the code, finally check the code - not comments usually you just check the code

Good when done right

[Luveno]

But most code reviews I have seen have been reduced to smaller items and standards enforcement (spacing, declaration, naming conventions, etc.) and not around anything that I consider significant enough to make the app "better." Usually those types of things are more in the application architecture, and a code review is much too late for that. As an aside, we used to semi-jokingly refer to code reviews as the "revolving vendetta" in my previous company, as certain individuals would pick on certain others with more zest.

Yup

[Anonymous Coward]

I once seen a guy go bat-shit crazy because I told him in a review of his code that a two letter macro in a header of a public interface was not a good idea. He insisted that NO ONE else would ever choose to use an identifier of the same name.

Re:automated testing can let stuff fail but still

[marnues]

Not true. A broken database is a mess to fix. A broken webpage is easy to fix. A broken database with a working interface appears to work to the customer, while a broken webpage with a functional database just doesn't work. Fix the broken webpage and you're free and clear. Of course, let's hope you didn't tie the user interface into the logic. That get's you hosed.

Re:automated testing can let stuff fail but still

[marnues]

Which is exactly why you never show them a working interface that doesn't have a functional backend. Salespeople notoriously create headaches for engineers and customers alike. Never give them an interface without a functional backend.

Code reviews, wisdom of crowd.

[aleckais]

Code reviews make truth/right ad-hoc techniques a non-accumulation point for programmers' commits. This, in proportion of how much members of the community look at each others' work. Adding values on top of this is still worse, because that bias only makes the downfall faster. This is another occurrence of that law concerned with the bad effect of horizontal exchanges on the rightness of the whole (cf. also Wikipedia). There is a comment of Ritchie somewhat according to this view, namely the one about his being thankful for not having developed the popular operating system using the advices of an overcrowded horde. Also, consider Perelman's isolation and its good intellectual effects.

Code Reviews != Bughunt

[Anonymous Coward]

If you think code reviews are about identifying bugs, that is a reason why they are ineffective.
The primary purpose of code reviews should be to share the code. If you can get other programmers in your organization to understand what you've written, then your code review is a success. Otherwise, you might as well skip them. I am surprised more organizations don't understand this.

It's all about HOW you review

[Anonymous Coward]

What's a code review? A long boring meeting where the whole teams sits around a conference table and reads the source line by line? A second set of eyes on the code while it's being written (pair programming)? Something in between?

The key to doing a good code review is getting additional people deeply engaged in the code. Very few real-world "code review" processes accomplish that.

The "meeting" style of review engages a lot of people, but the engagement is shallow. Responsibility is diffuse, so nobody's especially motivated to find bugs. And reading code off a page, in a linear fashion, is dull.

Reading code as someone writes it catches shallow bugs, like typos. But you are stuck reading at the same pace as the coder. Sometimes you're bored as they type in the obvious. Other times, they speed ahead when you need to stop and reflect.

The ideal process creates incentives, provides support tools, and allows ample time for analysis. How do you do that?

To manage incentives, have clear responsibility for verification. For each code module, one person is responsible for writing the code. One or more people are responsible for verifying it. There's a bounty for finding bugs, and shame for coding them. Keep score and publish it. Reward quality stars and fire the bad coders. If code ships, and you find bugs in the field, penalize the people who were supposed to verify that code. Keep score, and act accordingly.

What tools do you need? Good editors/IDEs with tags support, debuggers, and testing infrastructure. Reviewers don't just read code. They run it. Step through it. Step into functions. Watch variables. Try different inputs. They don't need a full blown testing environment. But they do need a way to browse and explore that's richer than just linear line-by-line reading.

How much time do you need? Maybe less than you already spend on code reviews. Suppose you have 20 modules. That's 20 1-hour meetings where 20 people sit around a table and read code. 400 man hours. Instead, assign 2 people to review each module. Give them a work day each. 20*2*8 = 320 man hours total. I guarantee, 2 people beating up code for a day is more effective than 20 people giving it a half-assed readthrough. More eyes is better, but only if they're paying attention.

Have the right people systems in place and your code reviewing will be less onerous and more effective. Do it wrong, and you'll achieve nothing except wasted time.

Yes

[sustik]

To answer the editor's question: Yes, I am too good for code reviews.
Other programmers need code reviews, because they are not good enough or outright suck.

I am also much better at self praise than the rest of you losers and I have a much higher self esteem as a
result.

P.S.:I also think that there is no other worthwhile response to flame baiting (title) than humor.

Re:automated testing can let stuff fail but still

[bberens]

IOW the automated tests only tests what the test was written for.

300 of 263 comments loaded

[Cletus87408]

Seems the answer for whomever developed the /. page is a definitive yes!

No, you are not.

[twocows]

Short answer: no.
Long answer: nooooooooooooooooooooooooooo.
Seriously, everyone knows code reviews are a good thing unless your ego is friggen huge. The problem is convincing the higher-ups that it's worth taking an extra X days off the release schedule.

Re:No, you are not.

[Dwonis]

Seriously, everyone knows code reviews are a good thing unless your ego is friggen huge.

Wrong. My ego is friggen huge, and I still know that code reviews are a good thing.

I can't think w/ someone looking over my shoulder

[moorster]

It just doesn't work to have two guys crammed in a cubicle huddled around a monitor. It's just like the human eye that sees by quickly moving about. The person who is 'driving' is the only one with any ability to understand the code. The other person quickly zones out. Meanwhile, the person who is driving gets self-conscious about how fast/slow he's going so he stops thinking about the code too. If either person says anything out loud it completely destroys the other's train of thought. It devolves into a dysfunctional group-think where each person says "I'm OK with it if you are...". For me it works much better to monitor the SVN commits and take my time to look at every line of code and work through it at my own pace.

Advisory code reviews?

[Anonymous Coward]

Code reviews are a giant and totally necessary pain. The problem is that the usual code review process gives complete control to the reviewer: at some point a person trusted to write code has to be able to take responsibility and say it's good enough.

How about:
- Reviews are mandatory and must be documented (Mondrian or email thread, whatever works.)
- Reviewers log their opinion, same as always, and original reviewers must see any changes.
- The coder is free to checkin in reviewed code without changes (they should log their opinion.)

Coders remain responsible for their product as always: if they're comfortable with 'won't fix', they need only say so.
Reviewers document problems they see and cover themselves; they get humiliated for what they miss (as always.)

I'd probably take exactly the same action, but the stress and annoyance would be much less.

Not just coders.

[pavon]

One of our customers upped the quality assurance requirements for our projects with them, which means that we get to have a bunch of non-technical (or worse ex-technical) management review everything we do at every stage of the process: design, implementation, change request, test plan, close-out. These people are worse than the coders about debating stupid nitpicking ideas.

There is just something about putting a bunch of people in a room for the purpose of performing a review that eliminates any sense of perspective that they might have.

I could argue.

[Anonymous Coward]

"keep it fresh"? having a bunch of want-to-be's review my code when they can't even write their names on their paper back lunches?

"On my toes"? So I don't step in their shit-arse arguments that don't hold water as the reviewers spent less than 1 hour to read the specs for a major coding project.

"Ensure continuity"? Sure. I can produce shit like the Indian dumb arses or I can write good code. Give me a break. A review by inferior fools who can't even speak my language has wasted more of my time than you will ever know.

Bah...

[liquidweaver]

We don't use code reviews here. In fact, we barely introduce new features (only if we really have to, which is sometimes in the span of a decade).
We simply dominate certain segments of the market. We found it is not only cheaper, but more predictable to use our market position to influence other companies and sometimes governments to keep our favorable position. In fact, we often times will announce features but never even implement them! What's crazy, is that shit works - well! Our stock goes up every time because our consumers and shareholders love us so damn much!

As a software company, we would like to proudly announce to the world that a solid understanding of how to manipulate to market, your employees, and public perception at large is far more effective then code review, features, ability, or even competence in general. You should learn something from us, spending so much time on trite details like these.

Re:Bah...

[Anonymous Coward]

Is your company owned and run by Jewish people?

Too busy?

[Anonymous Coward]

I'm too busy for code reviews.

But apparently not too busy to read and post to slashdot. A curious definition of "too busy".

Re:automated testing can let stuff fail but still

[turbidostato]

"IOW the automated tests only tests what the test was written for."

And that's only if the test is itself bug-free.

Dogma

[lolococo]

I've read a lot of posts in this thread with "you should", "you must", "you have to", you're not", "absolutely" etc. This all sounds like dogma to me, and as such, I view it with a suspicious eye.

C'mon guys, with all the technologies and programming languages, there's an infinity of things that be done very well. Surely, within that infinity, there's room for people who think differently?

In my experience, sticking to any kind of dogma or absolute is one major factor of failures in software development. Keep your mind and your options open. Be ready to adopt code reviews and to abandon them. Best practices are indeed just that. They're not the only practices, and more often than not they are in reality good practices. There's no one-size fits-all, not in the software industry, so please let's calm down and consider things outside our comfort zone, they have merit too.

My experience

[Rene S. Hollan]

Perhaps one in ten reviews of my code have managed to find a bug. The rest usually fall into the following category:

1) Bona-fide bugs were not found. This is the vast majority.

2) Someone complains about style... where style is either ill-defined, or has the guide has changed since that block of code was written. This is particularly bad when modifying GPL code that does not adhere to internal style.

3) Someone doesn't understand the programming language and complains about a bug that isn't.

4) Someone doesn't like the implementation but can't offer a valid reason for rejecting it (performance, etc.)

5) There are no bugs, well none that weren't found yet.

Code reviews are useful, but you require good reviewers of two different types: 1) those who don't know the semantics of the components and pick out syntactical errors that are valid language constructs. and 2) those that do know the semantics and can pick out logical errors.

 

Nobody wants code reviews

[Sarusa]

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.

Re:Nobody wants code reviews

[snowgirl]

This. 100% this.

In a corporate world where money/budget is all, once there is money being spent with no direct product, there is a strong desire to avoid it as much as possible. (I've actually never attended a code-review meeting actually, but we would basically code through the code and then write an email with concerns and issues.)

Coming from the world of F/OSS, I was all prepared for the honest and detailed code reviews I got from all my patches, yet never got any of it. In many ways I was ill suited to corporate programming, because to me priority 1 was get it done right, meanwhile everyone else's first priority was to do as little non-productive work as possible, and if that meant rubber stamping code reviews, then so be it.

Actually, my best example of code review / pair programming comes from a programming competition. I had a team member sitting there watching me iterate through a char * properly, when I had declared it an int *, surprise surprise the char * never seemed to be more than one character long. We ran out of time, and never solved the problem. Later he mentioned that he had seen me typo the declaration as an int *, and just figured I knew what I was doing. I looked at him stunned and said, "dude. Don't assume that."

Comment removed

[account_deleted]

Comment removed based on user account deletion

a couple of issues..

[samantha]

1) The problem of finding actual peers. Not everyone on a team is capable of meaningfully, much less efficiently, reviewing the code of everyone else.

2) Our tools for spelunking code and thus for its presentation to another person and ad hoc exploration are generally woefully inadequate. This makes code reviews much much less efficient than they could otherwise be. This is a general complaint with the state of tools. In Symbolics and in Smalltalk in the 80s I had better code walking tools than I have on the supposed state of the art IDEs today. It is quite possible to make much better code exploration tools but it never seems to happen.

3) I have had even very good people miss glaring errors so I am not a big believer in this process for elimination of many errors. It has other useful features but this in my experience is not one of them.

All of that said, review with a true peer can be very useful. It is most useful when reviewing bug fixes as compared to initial code in my experience.

Forget "good", let the fittest survive

[mightyhe]

I generally avoid the whole debate of review or not to review. My rule of thumb is, let them use their own method, be it with peer review or not; if they're shipping faster and profiting more, I have something to learn. So what about the manager that wants to promote best practices? My answer is, it is hypothetically possible to data mine for best practices. Of course, you aren't any good data mining, and you don't understand productivity yourself, so your observations about best practices are either anecdotal or hopelessly biased; in short, you're doing more distraction than good. Case in point, the latest greatest plan to make the programmers productive never does ship more software. Oh sure, it produces lots of great stories for which the boss can never cough up data, but never more software.

Re:Depends on the people

[profplump]

This has been tried before. Boeing put a lot of money into a program just like what you're suggesting.

But as it turns out, even in independent implementations, non-trivial bugs tend to pop up in the same places, so both are broken in the same way. That's because non-trivial bugs come from things like unclear specifications, missed edge cases, and other issues related to the programer's understanding of the problem and the types of solutions that people with a given system of programming tools are likely to find for those problems. The only thing independent implementations can reliably fix are coding errors, and they're probably an expensive way to get an extra set of eyes on the problem, particularly given the complications in reconciling the differences found between the two incompatible systems.

Yes!

[CyberDog3K]

Next question!

Re:automated testing can let stuff fail but still

[Anonymous Coward]

Not if you're in marketing.

Re:automated testing can let stuff fail but still

[marcosdumay]

That is why you write some scripts to test the automated tests.

Whatever you do, don't do what we do...

[StevenMaurer]

...at my current company, a software behemoth which got that way by buying out the VC stakeholders in innovative little companies and cannibalizing them.

We have a process in which you need to submit any change to the source base to "Code Collaborator", a barely functioning web-service tool which, besides needing babysitting by the administrators to work correctly, makes it an absolute requirement that some other engineer approves of your changes. Invariably, this means I'm trying to explain how a multithreaded algorithms work to a kid who just graduated from Bangalore University, and loves the power trip of being able to not actually do anything and get paid for it. I've had code on hold in our system waiting for a solid year to be reviewed (368 days) because of this kind of crap. (And invariably, when the code the junior engineers have all mutually approved for each other fails to work, they call up me to try to fix it.)

But buying out small companies and squeezing the life out of their software is profitable. Can't complain about the salary too much. Truly my company is still a place where software goes to die.

Don't forget to do self reviews

[roman_mir]

How about going over your own code once in a while, looking at it a month or 2 or 3 after it was written, certainly in that time period the understanding of the business problem has expanded or changed or was clarified somehow and it makes sense to go over business logic intensive parts to clean up, refactor, remove useless and restructure the useful.

What's driving this.....

[Anonymous Coward]

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?

I'll tell you what's driving this:

. Tighter budgets. Less money equals two things: (1) What is the least amount I can spend and still do some development? As an IT manager I can't spend a lot of money on a programmer, so I am forced to hire the cheapest one who meets my minimum specifications, instead of hiring a good, experienced programmer for $20k more. (2) My development team has less time to do the appropriate QA and testing tasks. I have a huge backlog of maintenance / support activities, and not enough bodies to do the work. The CFO's response? "Work smarter". Really? Smarter would be to invest up front, instead of on the back end.

. The 'evolution' of crappy methods. Agile? XP? IMHO these are just excuses for not gathering and agreeing upon requirements. No requirements = crap coding. No requirements = no ability to do meaningful QA and testing. Gather requirements up front, keep them in mind in the design, development, testing and deployment, and good things will happen. I'm not advocating for waterfall, but I have seen a tremendous decline in both the quality of requirements documentation, as well as the skills of business analysts to document requirements.

It depends on who's doing the reviewing ...

[golodh]

Whether a code review is useful all depends on who's doing it.

A nice caricature can be found here: http://www.jsquared.co.uk/jennyl/verity.htm

Don't believe that his sort of response during code reviews is all that uncommon.

Re:automated testing can let stuff fail but still

[maxwell demon]

But who tests those scripts?

Re:automated testing can let stuff fail but still

[Hal_Porter]

I think it's scripts all the way down.

Too good for code reviews?

[Larryish]

Are you too good for code reviews?

The short answer: YES

The long answer: ALMOST CERTAINLY

feedback loops

[epine]

What Thompson should have said is that among the guys he worked with, poor quality work would have been detected faster than Tony Soprano receiving a thin envelope from a deadbeat nephew with a drug problem.

I don't recall Furio being debriefed ever. Performance norms were adequately enforced by the custom of passing thick envelopes.

On the Firefox team entering into year five of dribblegate, you can make a case for additional code review. The performance norms pertaining to background memory leak are a lot softer and benefit more from up front scrutiny. Also, a better architecture might be considered. But failing that, code review is an excellent second line.

Impossible to hit schedule

[Anonymous Coward]

There is no way most software teams can consistently hit schedule without code reviews. What a team is saying when they do not have code reviews is: "We are unable to access the risk of the work we are about to integrate, and yes we are pushing the quality problems on to the next team... QA, It's not our problem".

Re:Impossible to hit schedule

[Javagator]

There is no way most software teams can consistently hit schedule without code reviews.

I agree 100%. Of course, there is no way most software teams can consistently hit schedule with code reviews.

Communication

[Dwonis]

We use code reviews to communicate about changes with the rest of the team. When we make significant changes, we plop them into our review tool so that everyone gets a chance to see how the code is evolving, to learn from each other, and to provide feedback.

It's also a great way to identify when team members don't know how the code works. If somebody says, "I don't understand any of this at all", then we know that it's time to sit down and explain things, and maybe add some documentation.

Coding Standards colapse too easily

[IBitOBear]

Real world example from a previous employer regarding a C++ project:

(element X) All class member variables shall be accessed vit standard format "getters" and "setters".

(element Y) All class member functions shall be defined within the separate compilation unit associated with the class.

These two elements were far apart from each other in the text. The combination of the two meant that most simple accesses of atomic data types (say integers) went from being an integer load (mov EAX,address) to a far call into a separate object file. When I complained I was told to STFU. When I ignored these two requirements and my code was the only code that ran in the time projected and allotted, everybody acted all surprised.

Most coding standards are good for keeping NPEs (non-practicing entities) straight out of school "in line", but they function on the "interchangeable morons" school of programmer management. Thing is, if your code is constrained to allow for the interchange of morons, then your code will be moronic.

goto(s) are bad, but "break" and "continue" are fine. Sure. Except that every now and again, a goto is _exactly_ what you need to make the logic clear, and sure you could skip the goto with purely local throw catch to get out of a nested loop, but that exception isn't all that exceptional any more.

I am all for "thou shalt not"

if (test)
    {
    commands; //this is not indented more than the braces
    }

when it is preventing something that unreadable, but that's more of a "call the idiot into your office and smack him upside the head" transaction IMHO. Meanwhile I bet someone is going to jump up and call that the epitome of good code layout.

Even Microsoft has abandoned that stupid thing where the type of the variable is prefixed to the name, which seemed like a good idea to have sValue for a short value, until someone decided that maybe it should be a long, but the docs had all been printed and everybody had already written most of the code, so you end up with just the declarations changed "long sValue;" because a global search and replace would have scooped up a bunch of "psValue" entries, and "short sValue" declarations in functions where it didn't change. We generally _ban_ "lptrszActualUsefulName" in favor of "ActualUsefulName" since it's the _compiler's_ job to deal with the type. Evidence shows that a programmer can deduce "FileName" is a string, and if he cannot, there is usually a declaration right there to tell him what it is if he cannot remember.

Coding standards are like military plans. The DO NOT SURVIVE first contact with the enemy, that being programmers trying to solve real world problems in non-retarded ways.

There are only two hard standards: Your code must be readable, your code must not suck.

But just like comparing Finnegan's Wake to Star Trek Slash Fic, the argument of what constitutes "Readable" is unbounded.

Re:Coding Standards colapse too easily

[GigaHurtsMyRobot]

Well said, and glad you decided to post... interesting example.

Re:I can't think w/ someone looking over my should

[Your.Master]

I think people have different ideas of what code reviews are. I think it's strange that we're talking about working with computers and everybody is assuming we're doing a face to face meeting with a single projected image.*

In my experience, people basically give me a source code change pack. I get around to looking at the diff when I get the time -- usually within the same day, next business day at latest. I read it myself, make my comments, ask questions, raise objections, and tell them I'm done with the current version. Then, when they get around to it, they send me a new change pack and I take a diff between the changes. Usually the second round is much quicker unless there was a major problem in the first round. Eventually we converge, and the final change is committed. Often for small changes I have no issues and it just goes in the first time. Big changes usually require the second round, occasionally three, very rarely more.

In some cases another reviewer or other reviewers will receive the same packs and do the same thing in parallel -- usually if the change crosses several components with different people familiar with each area. Each reviewer will probably focus on their own area and only make more cursory passes over the parts the other reviewer covers.

* Mind you, we do the projector thing where I work after we release a beta / release candidate, to increase the chances that the next one will be the last. We also do this when we're going to distribute a security patch. In both cases, the projector code review happens only after the more typical code review.

Get business knowledge besides coding practices

[Anonymous Coward]

It depends on who the reviewers are, and if people have enough time to review other people's code. Besides coding practices as stated by others, code reviews also give us an idea about the business functionality that's being implemented (if the reviewer doesn't know the business functionality), make sure that the person who has coded doesn't miss any business scenario (if the reviewer already has good knowledge about the functionality), helps everybody informed as to what others are doing such that when needed people can quickly pick up each other's tasks.

Code Reviews are essential

[lsatenstein]

Aside from looking at code that is of poor design, I look at code that uses an incorrect approach. For example, We had an application that used an ordered entry table of dynamic size (number of rows could grow or shrink). The programmer tested with a small population, and therefore programmed a sequential search. However the table grew from single digit numbers to almost 100 entries. The search algorithm was changed to do a sequential search for 10 or fewer items, and a binary search for larger number of entries.

CPU time dropped dramatically in the production environment. Other code reviews include looking for copyrighted sources that are pasted into the code without suitable permissions.

I happen to be part time programmer, having started with mainframe (ibm 360) and 8088/6502 assembly languages.

Re:automated testing can let stuff fail but still

[turbidostato]

"I think it's scripts all the way down."

Gosh, no! I've been said there's an elephant at the bottom supporting everything else.

I hate to nitpick...

[roguer]

Nah actually I love to nitpick.

In any case Ken Thompson was not the originator of C (the programming language), he was the originator of UNIX (the operating system). So it would have been more accurate (and slightly more impressive) to have called him Ken 'UNIX' Thompson.

Ken's Bell Labs colleague Dennis Ritchie was the originator of the C programming language.

Answer : "No, you're not too good..."

[RockDoctor]

If you're the weird recombinant mutant programmer offspring of Ada Lovelace, Donald Knuth, Linus Torvalds, with gene snippets from the Ken'n'Denis'n'Bjarne ... you're still not good enough to not benefit from code review. Even if the code is perfect and really, really gnarly, the documentation could almost always be clearer. It's really really difficult to see how someone new to the code would need to read the documentation that you've written while writing the code. "Clean" eyes on the documentation are essential.

When I'm writing code at work (and as the only repeat only person in the company who ever uses the documentation system) I struggle to get someone else to review my code before putting it out to customers and colleagues.

Tried Both Ways

[Anonymous Coward]

I've worked on projects with formal code reviews, and without formal code reviews. And I've seen them work, and I've seen them fail.

One project was with a group of skilled developers all working in the same area. The only rule was "All Code Must Compile". When someone did an update, and the code failed to run, they'd debug it, and most of the time this would entail stepping through someone else's code. If a developer had done something stupid, the developer debugging the runtime error would say "What is going on at $line in $file?" and everyone would immediately update their working directories and check out the problem. The developer who'd done something stupid would then own up to having been stupid, a brief discussion would be had, and everyone would move on.

This was pretty much the ideal. It is not, however, the norm.

Another project had formal code reviews, scheduled for tuesday afternoons. Tuesday mornings the team would spend reviewing the code to be reviewed that afternoon (So, basically, 10-20% of our time was spent on code reviews). One of the developers looked at the various approaches the team had to writing up their code review, and provided a simple five-column form to bring everything into a somewhat consistent structure. We caught a truly abysmal piece of code with this method, and sent the developer in question off to fix the code base. At the followup review, the code had gotten *worse*, as the developer in question had introduced more bugs with the fixes (and had failed to fix the original problems to boot).

The team lead -- not a developer, but a respected process expert -- directed that the code be checked into the mainline anyway, bugs and all, in order that the team would not fall farther behind schedule. And then to help us "streamline" our working review process, gave us an "improved" code review form -- this one fifteen columns, with 57 "codes" printed on the back of the form that we were supposed to use instead of the plain old boring text description of a problem. Code reviews, now fully CMMI-compliant, were effectively meaningless after that.

And then there's the OTHER team that didn't do code reviews. Their code is entirely undocumented, always misformatted, with poor variable names, no modularization, and incoherent organization. There are no unit tests, no regression tests, no sample data generators. The code is fragile, and when it fails, it emits incomprehensible errors, or produces strange data, so that only the original developer would have a chance of finding the error in less than a day. Asking for formal code reviews results in howls of protest -- followed by scheming to form agreements with the other developers to sign off on each other's code. They think they're like the very first group, but they're not.

So I've worked with developers skilled enough not to need code reviews. I've worked on projects where code reviews were very beneficial. I've worked on projects where code reviews were a waste of time. And I've seen a ton of developers who aren't nearly as good as they think they are.

And pretty much everyone here who's saying that THEIR code doesn't need code reviews fall into that last category.

Software process is for

[Anonymous Coward]

teams of mediocre developers that where created in the pre-dot.com bust era when people abandoned fine arts and science degrees because they could make gobs more money writing code. You need software process when you have teams of developers where using logic and common sense is not instinctive, instead these people learned how to program through DeVry like -get rich quick- programs where after only 6 months of "training" they were unleashed into the workforce and expected to write Windows or Linux, or whatever.

There is a much smaller sub-set of coders such as myself where coding was something I grew up doing. I learned how to program after reading an old TRS-80 basic programming guide when I was 8 years old. Every computer I got or was able to use had code written for it. For me I can model any real world data set as classes and objects and write logical code that, for the most part, does not fail.

I am not above code reviews, but I don't like them imposed on me. I know the difference between when I need a code review ( working in some new technology area I am not familiar with, want a second opinion on some complicated bit of logic, etc ), and I know when I can check in a months worth of work and know it is flawless, because 20+ years of programming experience has developed those skills. I definitely do not need a peer review from a DeVry graduate who is constantly fixing bugs they have committed over and over again.

On the flip side, I do have to give a lot of code reviews which has a tendency to waste my time. It is necessary when dealing with weaker developers, to ensure they are not checking in some massive cluster f*ck of unmanageable code. But usually I want to know that these people are gaining experience and learning so that I do not have to review every line of code they commit for the rest of time. Personally someone that requires a code review, and errors are found every time, should consider an alternative career, and I think more companies need to grow a pair of balls and lay off bad developers rather then trying to pander to them by imposing Draconian software processes that hinder skilled and unskilled developers alike.

I think software process fails when for every 1 hour of code development requires > 1 hour of process. I work in an environment like that and I am quickly looking for work elsewhere. The managers think this process overhead will deliver quality code, the reality is the code base has stagnated innovation wise while Agile processes and code reviews give a false sense of quality control.

I would rather work on a small team of skilled developers who can trust each other's work and know when to peer program or code review rather then a large team of mediocre developers hiding behind software processes and forcing code reviews for all which just waste everyone's time.