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.
