Slashdot Mirror
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?'"
And some of that needs to testers who are NOT coders or people who are not mainly doing programming.
Anyone who is anti-code review is an arrogant snot. No one is perfect, and we all have the capacity to overlook things.
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.
Hold up, wait a minute, let me put some pimpin in it
... 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.
Perhaps peer code reviews were less important 30 years ago. The constraints around running code were much tighter. There were fewer layers in the stack where bugs could lurk. Memory was scarce, instruction sets were limited. Computer applications were like my 1973 AMC Gremlin: fewer moving parts, so it was easier to see which parts weren't working correctly and predict when they would fail.
Welcome to the industry, Chris. This might have been the funniest thing I read all week.
I'm 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.
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.
Because code reviews end up being an enabler for OCD coworkers to tell me how to indent.
Slashdot: Where people pretend to be twice as smart as they really are by behaving like children.
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.
I rarely read replies, it's my opinion and if you thought about your opinion a little more, I'm OK with that.
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
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.
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.
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.
HTML is obsolete. It's time for a new, simpler and richer markup language.
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.
:(){
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....
People in cars cause accidents....accidents in cars cause people
If you use some software, then the process is MUCH easier. http://www.reviewboard.org/
Mad Software: Rantings on Developing So
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.
Nathan's blog
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.
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.
...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.
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.
:-), 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.
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
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.
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.
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.
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.
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.