Beta

Slashdot: News for Nerds

×

Welcome to the Slashdot Beta site -- learn more here. Use the link in the footer or click here to return to the Classic version of Slashdot.

Thank you!

Before you choose to head back to the Classic look of the site, we'd appreciate it if you share your thoughts on the Beta; your feedback is what drives our ongoing development.

Beta is different and we value you taking the time to try it out. Please take a look at the changes we've made in Beta and  learn more about it. Thanks for reading, and for making the site better!

Are You Too Good For Code Reviews?

Soulskill posted about 3 years ago | from the what-else-are-end-users-for dept.

Programming 495

theodp writes "Why do some programmers,' asks Chris Hemedinger, 'place little value on code reviews?' This apparently includes even Programming Greats like Ken 'C' Thompson, who quipped, 'we were all pretty good coders' when asked about the importance of code reviews in his work. Hemedinger, on the other hand, subscribes to the school of thought that peer code reviews are Things Everyone Should Do. Not only do reviews keep you on your toes, Hemedinger says, they also 'improve quality, ensure continuity, and keep it fresh. Who can argue against that?'"

cancel ×

495 comments

We need more testers / QA as well (4, Insightful)

Joe_Dragon (2206452) | about 3 years ago | (#36694690)

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 (2)

pixelpusher220 (529617) | about 3 years ago | (#36694780)

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.

automated testing can let stuff fail but still pas (1)

Joe_Dragon (2206452) | about 3 years ago | (#36694964)

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

Re:automated testing can let stuff fail but still (3, Insightful)

pixelpusher220 (529617) | about 3 years ago | (#36695232)

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

Re:automated testing can let stuff fail but still (1)

Pieroxy (222434) | about 3 years ago | (#36695296)

When your input field is overlapped by your nice little logo, there's no way to enter anything in it (for the average Joe). You page may "work" but if users can't figure out how, it it as good as a broken database.

Re:We need more testers / QA as well (1)

memyselfandeye (1849868) | about 3 years ago | (#36695020)

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 (1)

Tsingi (870990) | about 3 years ago | (#36695116)

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 (1)

Z00L00K (682162) | about 3 years ago | (#36694808)

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 (2)

memyselfandeye (1849868) | about 3 years ago | (#36695072)

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 (1)

EraserMouseMan (847479) | about 3 years ago | (#36695234)

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.

I don't always test my code, but when I do.... (2, Funny)

FictionPimp (712802) | about 3 years ago | (#36694882)

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

pixelpusher220 (529617) | about 3 years ago | (#36695278)

Made. Of. Win.

Awesome!

Pure Arrogance (2, Informative)

Anonymous Coward | about 3 years ago | (#36694704)

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 (0)

GigaHurtsMyRobot (1143329) | about 3 years ago | (#36694856)

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 (0)

Anonymous Coward | about 3 years ago | (#36694958)

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 (0, Troll)

GigaHurtsMyRobot (1143329) | about 3 years ago | (#36695148)

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 (3, Insightful)

JoeTalbott (2146840) | about 3 years ago | (#36695000)

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 (1, Troll)

GigaHurtsMyRobot (1143329) | about 3 years ago | (#36695186)

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

I agree 100% (4, Insightful)

jlechem (613317) | about 3 years ago | (#36694710)

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% (5, Insightful)

flooey (695860) | about 3 years ago | (#36694770)

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% (1)

jlechem (613317) | about 3 years ago | (#36694798)

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.

There's a point when... (2)

blahplusplus (757119) | about 3 years ago | (#36694724)

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

Z00L00K (682162) | about 3 years ago | (#36694764)

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

Anonymous Coward | about 3 years ago | (#36694842)

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

bames53 (300438) | about 3 years ago | (#36694910)

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

Remloc (1165839) | about 3 years ago | (#36695058)

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

Anonymous Coward | about 3 years ago | (#36694860)

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

ArsonSmith (13997) | about 3 years ago | (#36694952)

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

Re:There's a point when... (1)

JoeMerchant (803320) | about 3 years ago | (#36694988)

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.

A matter of time... (1)

mdf356 (774923) | about 3 years ago | (#36694726)

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

pixelpusher220 (529617) | about 3 years ago | (#36694892)

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

Anonymous Coward | about 3 years ago | (#36694916)

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

Re:A matter of time... (1)

JoeMerchant (803320) | about 3 years ago | (#36695018)

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.

League of Legends? (-1)

Anonymous Coward | about 3 years ago | (#36694728)

Love being offtopic here, but his last name makes me think of League of Legends ^__^

It's a cost/benefit thing (0)

Anonymous Coward | about 3 years ago | (#36694730)

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 (2)

NorthWestFLNative (973147) | about 3 years ago | (#36694844)

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 (0)

Anonymous Coward | about 3 years ago | (#36695026)

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

Welcome to the industry, Chris. (3)

xxxJonBoyxxx (565205) | about 3 years ago | (#36694738)

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

Synerg1y (2169962) | about 3 years ago | (#36695256)

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.

They're good (0)

Anonymous Coward | about 3 years ago | (#36694754)

Because not everyone is Ken Thompson =)

Your Employer or Client Can Argue (0)

Anonymous Coward | about 3 years ago | (#36694766)

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 (1)

sosume (680416) | about 3 years ago | (#36695124)

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 (5, Insightful)

jeff4747 (256583) | about 3 years ago | (#36694788)

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 (1)

CapuchinSeven (2266542) | about 3 years ago | (#36694888)

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 (0)

Anonymous Coward | about 3 years ago | (#36694894)

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 (1)

codegen (103601) | about 3 years ago | (#36695132)

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

this (2)

DeadCatX2 (950953) | about 3 years ago | (#36694906)

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 (1)

Spiflicator (64611) | about 3 years ago | (#36695062)

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 (1)

DeadCatX2 (950953) | about 3 years ago | (#36695282)

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

OldTroll (970892) | about 3 years ago | (#36694942)

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 (1)

Co0Ps (1539395) | about 3 years ago | (#36695078)

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 (1)

OldTroll (970892) | about 3 years ago | (#36695180)

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 (0)

Anonymous Coward | about 3 years ago | (#36695012)

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

Re:I'm not too good for code reviews (1)

JoeMerchant (803320) | about 3 years ago | (#36695090)

>

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 (1)

mcmonkey (96054) | about 3 years ago | (#36695194)

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.

s/busy/lazy/g (0)

Anonymous Coward | about 3 years ago | (#36695292)

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.

Casting pearls before swine (0, Funny)

Anonymous Coward | about 3 years ago | (#36694802)

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 (1)

etresoft (698962) | about 3 years ago | (#36695008)

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 (0)

Anonymous Coward | about 3 years ago | (#36695044)

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

Re:Casting pearls before swine (0)

Anonymous Coward | about 3 years ago | (#36695102)

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

Depends on the reviewers (1)

Bookwyrm (3535) | about 3 years ago | (#36694820)

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.

I'm not too good ... (0)

Anonymous Coward | about 3 years ago | (#36694836)

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

GreyWolf3000 (468618) | about 3 years ago | (#36694854)

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

Re:Obvious answer is obvious (2)

syntap (242090) | about 3 years ago | (#36695024)

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.

Departmental standards (0)

Anonymous Coward | about 3 years ago | (#36695250)

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.

In the perfect world maybe .... (2)

johnlcallaway (165670) | about 3 years ago | (#36694858)

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

JoeMerchant (803320) | about 3 years ago | (#36695118)

But I don't have all the time in the world, so I do what I can given the time that I've been given to get work done.

So you, like me, are posting on /. at 11AM EST.

The problem is poor developers... (4, Insightful)

rtilghman (736281) | about 3 years ago | (#36694874)

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

MichaelKristopeit424 (2018894) | about 3 years ago | (#36694978)

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

Spiflicator (64611) | about 3 years ago | (#36695146)

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

JoeMerchant (803320) | about 3 years ago | (#36695172)

The problem is that the need for code reviews is driven by lax, sloppy developers who don't see regression testing as a requirement, and who foist crappy, untested code that, in many cases, they haven't even tested.

Code review and regression testing are separate animals.

Code review is an application of the hive mind to the problem at hand, and an exchange of knowledge among the worker bees which can, in an ideal world, lead to better productivity for everyone after the meeting.

Regression testing is an (imperfect) attempt to be sure that requirements are met.

The two are complimentary, both can be misused, abused, and turned into a mockery of what they should be.

My own experience (1)

MobileTatsu-NJG (946591) | about 3 years ago | (#36694876)

Hemedinger says, they also 'improve quality, ensure continuity, and keep it fresh. Who can argue against that?'"

You mean besides the hordes of contrarian Slashdotters looking for the word Insightful to appear next to their posts? Heh.

Well, I wouldn't bother trying to argue with that. I know that I'm more apt to design and implement my scripts a little more thoughtfully when I know somebody else is likely to pick them up and work on them. Unless you suffer from 'code fright' it's difficult to picture a scenario where this wouldn't be preferred.

See monkey do (1)

bidule (173941) | about 3 years ago | (#36694884)

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.

Code review as a method for knowledge passing (4, Insightful)

Anonymous Coward | about 3 years ago | (#36694900)

I tend to use code reviews as a method for showing other team members the work that I have done so they will know about the changes. I work with a large number of geographically disperse people and having 1:1 conversations about every little change can be tiresome and wasteful. Code reviews are an excellent way to get around this problem.

Another bonus for code reviews, along the same lines - is in teaching people new to a platform about the kinds of bugs to expect when working on the platform.

It also keeps me honest, did I just add a debug, or do I also need to add trace? If I forgot the trace, code review will pick that up.

"formal" code reviews: a waste of time (2)

spectro (80839) | about 3 years ago | (#36694902)

Having another pair of eyes on your code is a very good idea but this whole concept of everybody getting together and put somebody's work to shame is bad for team dynamics and a waste of time.

I rather have a team dynamic where any developer can pick up a task involving work on somebody else's code, this way code reviews just happen naturally.

Short answer: Yes (1)

JoeMerchant (803320) | about 3 years ago | (#36694920)

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.

as long as you have useful reviewers (0)

Anonymous Coward | about 3 years ago | (#36694924)

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 (0)

Anonymous Coward | about 3 years ago | (#36694928)

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 (1)

ShavedOrangutan (1930630) | about 3 years ago | (#36694932)

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 (1)

halivar (535827) | about 3 years ago | (#36695094)

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.

Nope. (0)

Anonymous Coward | about 3 years ago | (#36694956)

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 (0)

Anonymous Coward | about 3 years ago | (#36694962)

wtfs per second.

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

Could have linked to the original article (0)

Anonymous Coward | about 3 years ago | (#36694966)

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

The reality is ... (0)

Anonymous Coward | about 3 years ago | (#36695004)

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 (0)

Anonymous Coward | about 3 years ago | (#36695016)

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 (1)

gbjbaanb (229885) | about 3 years ago | (#36695030)

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.

Code Reviews are never done right. (1)

140Mandak262Jamuna (970587) | about 3 years ago | (#36695032)

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.

Yes (0)

Anonymous Coward | about 3 years ago | (#36695048)

Yes, Yes I am.

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

wcrowe (94389) | about 3 years ago | (#36695074)

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.

Well... (1)

AngryDeuce (2205124) | about 3 years ago | (#36695080)

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 (1)

CHJacobsen (1183809) | about 3 years ago | (#36695088)

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 (1)

calmofthestorm (1344385) | about 3 years ago | (#36695208)

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.

Absolutely Nobody (1)

Archeopteryx (4648) | about 3 years ago | (#36695096)

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 (1)

SiteLinker (137132) | about 3 years ago | (#36695106)

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 (1)

stwf (108002) | about 3 years ago | (#36695108)

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.

Waterfall Vs. Agile (1)

SirLurksAlot (1169039) | about 3 years ago | (#36695142)

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.

It depends (1)

ichthus (72442) | about 3 years ago | (#36695214)

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.

Judging by... (1)

smooth wombat (796938) | about 3 years ago | (#36695220)

the amount of craptacular software that is out there, both free and paid, I would say the answer is yes.

Between the exhaustively documented bugs in Civ and AC (to name two games from the same person/group), to the WTF? in many open source packages, coders, as a group, believe they are too good for code review.

From a time before software tools (1)

petes_PoV (912422) | about 3 years ago | (#36695252)

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.

Code review pros and cons (1)

quietwalker (969769) | about 3 years ago | (#36695260)

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.

Load More Comments
Slashdot Account

Need an Account?

Forgot your password?

Don't worry, we never post anything without your permission.

Submission Text Formatting Tips

We support a small subset of HTML, namely these tags:

  • b
  • i
  • p
  • br
  • a
  • ol
  • ul
  • li
  • dl
  • dt
  • dd
  • em
  • strong
  • tt
  • blockquote
  • div
  • quote
  • ecode

"ecode" can be used for code snippets, for example:

<ecode>    while(1) { do_something(); } </ecode>
Create a Slashdot Account

Loading...