Oh god, this is so true. Albeit I’m still not out of university, having dealt mostly with very strange assignment requirements, I noticed how the amount of crap that got past the radar greatly reduced itself when my classmates and I decided to give each other’s code a look, to the point that our professor (one of the absolute best programmers I’ve ever known) asked us how we were doing it, and once we explained, started encouraging peer review in the other subjects he was teaching.
All well and good…as long as you work in a shop with more than one programmer. I am a one-man shop. I know AT LEAST three other people in the same predicament. And don’t just suggest to use each other as code reviewers…How would your manglement feel about you shipping intellectual property around the 'net to people in other shops, in some cases, actual competitors?
Cautionary tale: Being the lone wolf sounds cool, until you’re the one howling by yourself in the moonlight.
Like many things, I think open-source projects have been doing this for years out of necessity even when it wasn’t as popular in corporate environments.
I can’t really imagine a project without some form of code review, now.
There’s an interesting side-effect to getting reviews, though, which is that when I know I have to go through one, I often am a little less vigilant about my own code than I am on projects that don’t involve review (like personal projects). I won’t test quite as much, because the reviewer is likely going to test. I’ll still usually look over my code before I submit it for review, but sometimes not as thoroughly as I would if it isn’t going up for review.
Basically, a slight amount of the responsibility gets transferred to the reviewer that would normally be on me.
They’re still great things to have, though, and overall I think they really do improve things. Reviewers often see things that I never would have, even if the reviewer has less experience coding on the project than I do.
@Max that’s strange re lowering your standards. In my experience it has always been the opposite, knowing that people are going to review my work makes me less likely to be lazy because I know I’ll be picked up on any slackness.
I wonder what other people’s experience is? Maybe I’m a special case because I work on a code review tool
You left out Statler and Waldorf. Seriously though… I have had the opportunity to work closely with some really cool people the last year or two and it helped greatly to be sharing ideas and looking at the code to implement the ideas together. Talking through what you’re thinking about with someone else who’s head is in the same space is always a good idea
As a lone gunman for 10 of my 11 years in programming I have found peer reviews to be frivolous and counterproductive. I have learned nothing in a peer review that I didn’t learn by myself eventually. The problem with peer reviews is that the peer review gives management ammunition to fire you if you don’t live up to the expectations of even one of your peers.
The problem with peer reviews is that the peer review gives management ammunition to fire you if you don’t live up to the expectations of even one of your peers.
the point of peer review is not to give ammunition but feedback to you. At a sane organization, anyway.
I advocate picking your own peer review buddy, for the most part, so you enjoy the experience.
I like the term coding buddy. Having multiple eyes on code works for Lixus, why wouldn’t it work for your code?
The next questions is, why after plenty of evidence that code reviews create value, improve quality, and increase velocity, do we avoid them? This is a human problem, not a procedural one.
A rose is a rose by any other name? Isn’t this just a form of editing? Works for writers, academic research papers and a whole host of other constructive disciplines. Why do programmers think that can just bang it out once, and have it declared perfect? Particularly when – by most definitions – it’s the first time they ever wrote something like this.
It’s a strange occupation, often growing stranger.
I’m currently self-employed so I’m a single developer business here. I find I can successfully review my own code by reviewing it first thing the next day.
I am the only person in my company that does Ruby on Rails development. Are their any place online to find people to review your code. Like a stackoverflow.com for code review?
@Chad Moran:
I’m with you in this one, i do the exact same thing, start codding all day long, and in the next morning review my old code, i have to say it works great for me, i guess my duo is me and mini-me. I don’t know if can do that with another employee at my work, they are not bat programmers, but there isn’t chemistry and i don’t admire them, but i realize that if I can find my perfect buddy who can successfully review my code the productivity and energy would be awesome.
At last. The false assumption that someone can be self-critical is over. You might beleive you can be self-critical, but how can you actually be self-critical if you are never fully self-aware.
Just by explaining something to someone you become aware of problems in your code. The same is true of personal skills, interpersonal skills and management skills. People don’t have a great gague on what they know if they don’t know what they know. People don’t know they are treating others badly if they don’t realise they are doing it!
People don’t know how bad their management skills are because they simply aren’t aware. If they had to stop and explain why they mange people and things the way they do - they would have the same reaction as you have explained above.
Now if you can just extend your idea out a little and accept peer driven critical (positive and negative) feedback (or better still - 360 degree feedback), THEN you can truely be self-critical in all areas. Your code and company will benefit.
@PaulG
The problem with peer reviews is that the peer review gives management ammunition to fire you if you don’t live up to the expectations of even one of your peers.
Management should not be involved with peer review meetings - only peers should be. Not even the project manager should be part of the peer review. That way there is no holding back and no grudges held. There is also no wasted time of non-technical managers listening to technical options and tricks.
if Management are at a peer review - it isn’t a peer review.
Hm… I’d like to nominate today as coding buddy day. Imagine receiving Will you be my coding buddy? every feb 26.
Seriously though - sharing code with another pair of eyes does wonders. You get embarrassed by your wtfs and you get a sense of pride out of clean and readable code. My current choice of coding buddy is pretty limited as there’s only one other developer working here
Perhaps there’s a niche market in this? www.findyourcodingbuddy.com?
Seriously, peer reviewing would greatly improve the code quality where I am currently working. As I’m not an employee, just a contractor, I wonder what the best method of changing the working practices of the people I work with.
Unfortunately, the people I work with don’t read programming blogs so they are a tough bunch to improve to start with.
My manager just mentioned about exactly the same thing days ago and I couldn’t really agree with him as he didn’t illustrate the benefits of doing so good enough, but now I see the light of it
Nice one Jeff!
I am pretty much the only coder in our company right now, so I don’t have a coding buddy. But we will probably hire another coder in a few days, so I hope he can become something like that for me:)
What to do if you are the best programmer in the company and everyone wants to peer with you so they get their code checked but none of them is good enough to check yours?
That probably wont happen, you can always learn something. Noone knows everything. They can at least check the readibility of your code and they will learn from your code.
Many years ago there was a story going around - which may have been wildly apocryphal - that went something like this.
A software house noticed a steep decline in standards and productivity and managed to trace its roots to the sacking of a cleaner. When the programmers were asked what he used to do their replies all boiled down to:
He asked us ‘why’ we were doing it that way and he listened carefully to our explanations.
Needless to say, the cleaner was rehired.
It’s not the review that improves your skills, it’s explaining clearly what you are doing and and why you are doing it that way. If it’s clear in your mind it’s clear in the code.
I.e. the Wikipedia theory is that criticism from a not-so-expert is, on the whole, better than not having them around at all.
But my own experience tells me this isn’t a universal rule. For one thing, having people of comparable abilities play together (e.g. chess) generally works better (i.e. advances their level more) than having people with very dissimilar abilities work together, while the latter is obviously better when the ones with higher abilities specifically focus on trying to teach rather than trying to perform themselves.
As long as this is in moderation. Buddy’s can be abused by newbies looking for free teaching lessons and free bug testing. Too many times have I spoken to a Jr. programmer and convinced him that code reviews are not QA debug sessions.
In the classic book Think and Grow Rich, Napolean Hill describes that when you combine the minds of two people, the benefit is not 1 + 1, but rather 1 + 1 + 1 as the two minds combine to create a third - in the form of the ideas and thought patterns that are stimulated by an additional person and that would not have occured alone. I have found this to be true as well.
I’ve been imployed by a big, international company for many years, where I had to work alone. Now I’m a one-man shop. I simulate code reviewing by inspecting my own code not on the same day I wrote it, but, if possible, several days after. This is possible without wasting much time, because my software usually has enough separated parts, through which I can do a roundtrip. Additionally I have trained myself being able to attain a different attitude in the different phases. It works.
I would love to do peer reviews of my code. Try convincing my boss that we should budget time in the project for it! We are almost always in a this is what they would pay and it covers cost situation.
Like many things, I think open-source projects have been doing this for years out of necessity even when it wasn’t as popular in corporate environments.
Not really. The real value of doing one of these peer reviews is in having you EXPLAIN it to someone else. It’s amazing how many of your own mistakes you find when you actually have to go back and explain to someone how it works.
Even individual programmers can benefit from reviewing their own code by writing up a document explaining the code as if you were explaining it to someone else. Heck, if she REALLY loves you your wife may be willing to lend an ear (although she won’t understand it).
I don’t know of a single process that has improved quality for us more than simple peer reviews. The type that Jeff is advocating. Physically sit down at the programmer’s computer and have him/her explain what they did. It is invaluable.
I truly wish I had a peer to review my code, but I am the only developer here. I started a blog to share my code and to get comments on it, but I am not allowed to share my work code, so that is difficult as well.
The most productive peer reviews I have had came about not necessarily with the peers that I admired most, they were due to the relationship between the reviewer and the review-ee. Say one developer on a project wrote a web service to , and another developer writes a client that uses the service. Well now, you have a producer/consumer relationship from which a peer review on either end will benefit both developers.
I have long practiced something similar in nature. Even if you can’t find someone to review your code (for whatever reason), a great way to learn is to review the code of others.
Find a programmer that is better than you, or that you at least respect greatly. This is particularly effective in areas that you aren’t already well versed in. You can often learn some great things. You will often get straight to the meat, so to speak, with them having already figured out some tricks and overcome some issues that you would have to if you were to approach this from scratch.
Don’t fret if you don’t have someone like that. Fire up Reflector (if you don’t have it, GET IT!), and disassemble some of the .NET Framework itself. There’s some great stuff in there.
The problem with peer reviews is that the peer review gives management ammunition to fire you if you don’t live up to the expectations of even one of your peers.
Here’s the truth. They decide to fire you and then look for a reason.
In the words of Doctor Cox, Now, 'course, if you are lazy and incompetent, then, yes, that will buy you a one-way ticket out of here.
But let’s be honest, they’ll work that out without a code review.
I hate faddish programming trends (of which Jeff posts one per week or so), but pair programming was something I fell upon by accident at my university, and loved it – when working on a tricky bit of code.
I wouldn’t want someone looking over my shoulder while I wrote some trivial bit of code (I’m good enough to not write bugs in routine code), but when I was writing a neural net spam filter with some really hairy math in it, it was beyond essential having another guy there working with me on every line of code, making sure the thing was correct.
We’ve been experimenting with something similar at work. Our plan was to move to a two week sprint model, and at the end of every spring the ‘code czar’ for that sprint would examine an SVN diff of every change between that sprint and the prior one. That sounds horrible, but we have four very experienced developers and use jalopy religiously to keep the format consistent, so it really wasn’t that bad. The czar’s job was just to look for any WTFs and bring it to the attention of the group in a final sprint review.
It worked well for the first few sprints. We only fell off it because our scrum master screwed up and let some additional tasks creep into the sprint, then again, and then yet again, and suddenly the two week sprint was over a month and we were staring down the release deadline. But we’ve all agreed to resume the practice in the next release cycle.
Here’s one trick I found, don’t call it code review call it code proofreading. Many developers have had bad experiences with code review at past jobs, so you’ll get resistance. And many non-technical managers either won’t get the idea or think you’re wasting time.
But everyone understands the need to proof read written stuff; so making the jump to code is easier. To the developers, proof reading sounds less threatening than review. And managers will understand the need to proof read stuff that is being sent to clients.
Yes Jeff, I totally agree.
Its always good to have someone looking over your code.
Especially if you have to work on several projects on the same time.
Sometimes it makes even sense to speak about your code with someone who isnt a developer but has a good logical understanding, maybe you see then yourself where you can archive some optimizations.
On the last projects (like the one i have submitted at Website:) i often speak with the other developer about the code i was going to check in.
Currently im working all alone on some projects. So who wants to be my Coding Buddy?
What about the ladies? You forgot the girl geeks! We have coding buddies, too, you know! I work with another woman programmer, and we continually check up on each other’s code. Here’s a few you could have included:
Thelma and Louise
Lucy and Ethel
Cagney and Lacey
Wilma and Betty
Laverne and Shirley
At my current work, they just enforced the rule to show the changelists (we use Perforce VCS) to a senior developer before submitting it.
Which often results in changing this and that before.
Even senior developers, even project managers follow this rule.
And changelist descriptions include the initials of the reviewer.
While it doesn’t eliminate all mistakes, it catches a good number of errors, poor designs, forgotten internal rules, etc.
How about those projects which fail before they get to the coding stage? The efference downstream from poor design is usually almost impossible to fix at the implementation level. Perhaps we need design buddies at the cocktail napkin or whiteboard stage. There’s also the allied problem of independent peer-review of management.
the pair programming initiative is a way of using both sides of a human brain, while the one person is coding , mostly the left brain is used to figure out the logic. whilst the person on the viewing side is using a right brain perspective to evaluate the code being created by the left brain.
and like in the article , i find the comparison between pair programming and super hero side kicks a great one. because thats just what happens, when someone is in trouble , the other can help, and vice versa.
i have started working in pair forms for 3 months now, at first it is difficult to be open, and be decisive over an idea. at first i may seem struggling or difficult, or that one person is dragging the other. but thats how it is for the first part , give it one or two weeks. and if it still doesnt work , think about getting a different partner , or try and spend more time with your partner , to better know him/her.
from my experience i can truly say that pair programming is the best way of putting a lot of requirements on it and allowing them to cope under any circumstances , and to be ready for deadline. most SOLO programmers tend to get stuck on a single issue, and waste time.
rather then getting 100% from 1 person , get 1% from 100.
I think it’s important to see there are two ways of peer-reviewing.
Jeff is clearly talking about the right way. One-on-one reviews purely targeted at improving code and sharing knowledge. Its very important to do these type of reviews one-on-one.
The traditional style of reviewing is usually whole-team-against-one. These type of reviews usually don’t improve code, usually cost lots of time, often result in hostilities between team members and are usually done to give management ammo to fire developers.
So be aware. If your code reviews are taking place in a conference room instead of behind a workstation you’re probably in for trouble.
I agree with what you have said and it holds true in non-software related activities also.
For example, the effectiveness of a two person military unit compared to just a one person unit increases more than the expected 50%.
This is no magic formula, it is human nature - 2 brains are better than one.
In case of software writing, you have to decide at which point to apply the principle: during coding, after a code block is written, after it is checked in, etc
Secondly, you have to decide what you mean by ‘better code’: improve your skills, optimisation, functionality, etc
And adhere to this rule when finding a buddy: be the worst musician/guy in the band (from Chad Fowler who read it from guitarist, Pat Metheny)
The maintainability of the reviewed code increases. (I don’t think one can find many bugs by code reviewing.
Sharing of technical knowledge. You always learn something when you review code (for example: some kind of handy method of a library class you don’t use). And you always see things that can be done better. So the reviewed learns from the reviewer and the reviewer learns from the reviewed.
Sharing how a application works. If you have reviewed the code of you colleague it becomes easier to maintain his/her applications when he/she leaves.
Developers who think the way you were explaining …talking in terms of coder should know everything and be perfect should be immediately fired. If they have that kind of negative non-team oriented attitude, put them in a box and ship them to Afghanistan.
What about the ladies? You forgot the girl geeks! We have coding buddies, too, you know! I work with another woman programmer, and we continually check up on each other’s code.
I am truly ashamed that within 20 seconds, the words Strip Code Review flashed across my mind.
Wait…
In my project, we have adopted this practice of pair programming, we implemented it in a way so that when one is focusing on technical aspects the other one focuses on the business rules and in this way we are able to build a full proof usecase and at the same time doing code reviews.
When we started doing this, i thought we are wasting time, but i was totally wrong. This way of working definetly increases productivity and decreases the testing effort.
Thanks for the article Jeff.
Prachi
I have to admit looking back, I have regularly went back to some x-month old code which didn’t live up to the expectations.
I took those occasion to improve it, adding more robustness, more (updated) design, more documentation (as the first few days it’s often painful to look at code made maybe 1 year ago with no clue).
I am often surprised how much my impressions could change in just 6 months. Maybe I should prototype more and play more with the results. Too bad I lack the time and will to do so.
Having someone to review the code for me would likely be a real time-saver. We all know how difficult is to get the rid of that bad-design-that-was-not-considered-harmful after the code grows around it.
Why don’t I get reviews? Because essentially good workforce is impossible to find here. I mean, good enough to understand what I needed to solve in the first place. In a small company I worked with about 5 years ago, I had to spend about 6 months in taking the others from our terrible school level to useful level. By the time they started producing something, we were just missing the milestone by a long shot.
Yes, it sucks SO bad here.
But the benefit of a later-time or second-programmer review is undisputed to me.
The kind of fast and casual code review described in the article sounds great, but there’s also bad code review. I worked in a shop with two mandatory reviews, and it was painful. Most bugs caught weren’t functional bugs, but style and formatting issues: what to name a variable, how to format a comment, that kind of thing. Of course the two reviewers would disagree about the standard…
So if you’re going to do peer code review, you need to be able to let things go… the unimportant things anyway. The comic has it exactly right: if it doesn’t make you think WTF then it’s not even worth mentioning. Trust me, if you dig you’ll find plenty of real bugs…
Being a Quality Assurance Analyst, I think the WTF Quality Measure should be integrated into every SDLC.
To anyone in a One-Man Shop, ask around your group. Chances are you’ll find someone with enough programming experience to ask questions. Present your design and logic to them. If they WTF, then you know where to start.
@Nicolas: if a bridge architect released a beta for testing which repeatedly crashed, he or she would soon be helping you bag your groceries. Coding’s not difficult - thinking is.
I’ve seen lots of failed attempts at code review systems in the seven startups I’ve worked.
In general the very worst way to review is to save up a bunch of changes, call a bunch of people into a conference room and show the code on the wall with a projector. This system absolutely brings out the worst in people and doesn’t work.
The next worst system is to only code review after a bug appears that bothers the management. This makes the code review something akin to a military non-judicial punishment.
At my current shop (five devs) we’ve come up with a review system that works well.
The five of us have agreed that whenever possible we will not check in code until another dev reviews it. The reviews usually consist of the reviewer looking over the writer’s shoulder as he displays the diffs with the current code. Eighty five percent of the time the reviewer just says ‘that’s fine by me’. Five percent of the time the reviewer notices a real problem that is nipped in the bug. The other ten percent of the reviews consist of the author explaining the code as he looks at the diffs, then saying , ‘oh, that’s not right, I’ll change that before I check in’, and continuing the review until the reviewer says ‘that’s fine by me’ and walks away.
When we commit the change we prefix the commit comment with a ‘*’ to show its been reviewed and include the reivewers name in the comment.
If for reasons beyond the dev’s control he can’t find a reviewer (he’s working late everyone is gone) he can still commit, but prefixes the comment with a ! so that everyone knows that this commit is unreviewed.
This system works really well for us. We view these reviews as PEER reviews, not as any kind of evaluation.
Of course if your team hires a jerk then this won’t work, but then nothing will. Working with a jerk on your team, especially a talented jerk, needs a post of its own.
I championed the institution of a permanent check-in partner system at my current place of employment after seeing it used intermittently at my previous one.
Unlike pair programming, it’s extremely easy to get people on board with it. It rarely amounts to more than about 3% programming time investment (assuming a day’s work and a 15-minute review).
The obvious benefit is that bugs get caught before check-in, but I believe that the distribution of knowledge that check-in reviews promote is a more important benefit in the long term. Check-in partners learn good techniques from each other, maintain coding standards, and learn what code is actually going into the project.
So far, at my current shop, everyone who’s been introduced to the idea likes it, which is pretty remarkable for a piece of process.
Pair programming is not necessarily an onerous regime. I think the key is to not make anything a regime.
I personally like the open source development model, with developers more or less submitting patches to the project owners. It’s a good way to keep the code clean and to make the best use of the best people.
There is even a recent ms-research paper on this topic: Pair Programming: What’s in it for Me?
In this paper we report on a longitudinal evaluation of pair programming at Microsoft Corporation. We find from the results of a survey sent to a randomly selected 10% of engineers at Microsoft that 22% pair program or have pair programmed in the past. Using qualitative analysis, we performed a large-scale card sort to group the various benefits and problems of pair programming. The biggest perceived benefits of pair programming were the introduction of fewer bugs, spreading code understanding, and producing overall higher quality code. The top problems were cost-efficiency, (work time) scheduling problems, and personality conflicts. Most engineers preferred a partner who had complementary skills to their own, who was flexible and had good communication skills.
It’s hard to do a code review if there are no published guidelines or standards. Each company should have this document, which itself must be updated and reviewed regularly.
Everyone wants to be code cowboy, but best practices do exist and not everyone on a team can have it their own way. The code belongs to the company, after all.
And, each company needs someone to manage the devs. They don’t need to be the best coder, but they must know how to settle disputes and pick the best path, and be able to tell the coders to shut up, it time to move on.
It’s hard to do a code review if there are no published guidelines or standards. Each company should have this document, which itself must be updated and reviewed regularly.
You’re trolling, right?
If you aren’t, then I’ll just say that code reviews are supposed to be informal - no guidelines beyond look at the code and make sure the guy checking it in can explain what it does.
But implying that all code reviews are informal with no guidelines beyond having someone explain what it does is exactly the opposite of what I’ve seen work.
