As I alluded to in my last post (which I will be correcting shortly), I no longer work for Google. I still haven’t decided quite where I’m going to wind up – I’ve got a couple of excellent offers to choose between. But in the interim, since I’m not technically employed by anyone, I thought I’d do a bit of writing about some professional things that are interesting, but that might have caused tension with coworkers or management.
Google is a really cool company. And they’ve done some really amazing things – both outside the company, where users can see it, and inside the company. There are a couple of things about the inside that aren’t confidential, but which also haven’t been discussed all that widely on the outside. That’s what I want to talk about.
The biggest thing that makes Google’s code so good is simple: code review. That’s not specific to Google – it’s widely recognized as a good idea, and a lot of people do it. But I’ve never seen another large company where it was such a universal. At Google, no code, for any product, for any project, gets checked in until it gets a positive review.
Everyone should do this. And I don’t just mean informally: this should really be a universal rule of serious software development. Not just product code – everything. It’s not that much work, and it makes a huge difference.
What do you get out of code review?
There’s the obvious: having a second set of eyes look over code before it gets checked in catches bugs. This is the most widely cited, widely recognized benefit of code review. But in my experience, it’s the least valuable one. People do find bugs in code review. But the overwhelming majority of bugs that are caught in code review are, frankly, trivial bugs which would have taken the author a couple of minutes to find. The bugs that actually take time to find don’t get caught in review.
The biggest advantage of code review is purely social. If you’re programming and you know that your coworkers are going to look at your code, you program differently. You’ll write code that’s neater, better documented, and better organized — because you’ll know that people who’s opinions you care about will be looking at your code. Without review, you know that people will look at code eventually. But because it’s not immediate, it doesn’t have the same sense of urgency, and it doesn’t have the same feeling of personal judgement.
There’s one more big benefit. Code reviews spread knowledge. In a lot of development groups, each person has a core component that they’re responsible for, and each person is very focused on their own component. As long as their coworkers components don’t break their code, they don’t look at it. The effect of this is that for each component, only one person has any familiarity with the code. If that person takes time off or – god forbid – leaves the company, no one knows anything about it. With code review, you have at least two people who are familiar with code – the author, and the reviewer. The reviewer doesn’t know as much about the code as the author – but they’re familiar with the design and the structure of it, which is incredibly valuable.
Of course, nothing is every completely simple. From my experience, it takes some time before you get good at reviewing code. There are some pitfalls that I’ve seen that cause a lot of trouble – and since they come up particularly frequently among inexperienced reviewers, they give people trying code reviews a bad experience, and so become a major barrier to adopting code review as a practice.
The biggest rule is that the point of code review is to find problems in code before it gets committed – what you’re looking for is correctness. The most common mistake in code review – the mistake that everyone makes when they’re new to it – is judging code by whether it’s what the reviewer would have written.
Given a problem, there are usually a dozen different ways to solve it. Andgiven a solution, there’s a million ways to render it as code. As a reviewer, your job isn’t to make sure that the code is what you would have written – because it won’t be. Your job as a reviewer of a piece of code is to make sure that the code as written by its author is correct. When this rule gets broken, you end up with hard feelings and frustration all around – which isn’t a good thing.
The thing is, this is such a thoroughly natural mistake to make. If you’re a programmer, when you look at a problem, you can see a solution – and you think of what you’ve seen as the solution. But it isn’t – and to be a good reviewer, you need to get that.
The second major pitfall of review is that people feel obligated to say something. You know that the author spent a lot of time and effort working on the code – shouldn’t you say something?
No, you shouldn’t.
There is never anything wrong with just saying “Yup, looks good”. If you constantly go hunting to try to find something to criticize, then all that you accomplish is to wreck your own credibility. When you repeatedly make things to criticize just to find something to say, then the people who’s code you review will learn that when you say something, that you’re just saying it to fill the silence. Your comments won’t be taken seriously.
Third is speed. You shouldn’t rush through a code review – but also, you need to do it promptly. Your coworkers are waiting for you. If you and your coworkers aren’t willing to take the time to get reviews done, and done quickly, then people are going to get frustrated, and code review is just going to cause frustration. It may seem like it’s an interruption to drop things to do a review. It shouldn’t be. You don’t need to drop everything the moment someone asks you to do a review. But within a couple of hours, you will take a break from what you’re doing – to get a drink, to go to the bathroom, to talk a walk. When you get back from that, you can do the review and get it done. If you do, then no one will every be left hanging for a long time waiting on you.
My first job as a newly minted EE back in 1981 was as a circuit card designer. I worked for a military contractor, and we did a detailed design review that was akin to a modern code review. (I later migrated into software engineering, and I’ve reviewed my share of code over the years). This detailed design review shocked, shocked, shocked senior new-hires; NO ONE had ever taken a look at their designs before. They were, on the whole, offended. They complied grudgingly… until they were at the company long enough to realize the benefits of review, which you’ve outlined.
There was one added benefit, given that this was a military contractor: when your hardware didn’t work during system test on graveyard shift, or even worse, during system installation at some godforsaken military base at the end of the world, there was likely to be somebody there who had seen your design and was familiar enough with it to troubleshoot it. There’s much to be said for not getting phone calls in the middle of the night, or being put on a plane to nowhere for some indeterminate amount of time.
Hey Mark, sorry to hear you aren’t at Google anymore (we started on the same day, though you may not remember me at this point).
One point I’d somewhat expand on is that, in addition to attempting to ensure the reviewed code is correct, you’re also attempting to make sure the code is maintainable. The code under review might be correct, but any aesthetic problems it might have could cause future bugs.
The “what you would have written” problem is particularly troublesome in the domain of aesthetics. There’s a difference between commenting that the code doesn’t use the same naming conventions as the surrounding code, all of which was written by you, and commenting that you prefer other names. It’s important to do the former, since that’s likely to prevent bugs, but the latter causes just the problems that you describe with no benefit. I find a reasonable guideline is to try to articulate exactly which bugs might be prevented by a comment; if you can’t do that easily, it’s likely just a personal preference and you shouldn’t bother the coder with it.
I have no idea who you are, but I think that’s because your real name doesn’t show up here. I’m pretty sure I remember everyone from our first day, so I just can’t connect your pseud to your face :-).
I’m sad to not be at Google anymore, but that’s life, and I really can’t talk about it here.
You came across as a jerk. I don’t think there was any need to comment on anything confidential. And there was plenty here otherwise that might have been even a small effort.
Did you actually read the post?
I didn’t comment on anything confidential. In fact, I specifically made a point of saying that I wasn’t commenting on anything confidential! (Aside from the fact that it would be remarkably unprofessional, I’d also gave Google’s lawyers coming after me if I disclosed anything that I wasn’t supposed to!)
“You came across as a jerk.”
Project much?
I find code reviews to be a waste of time unless you are intimately aware of the piece of code being reviewed. I’ll bet more than 90% of people who claim to be doing reviews just quickly glance at it and hit “ok”. It would be interesting to see some stats on how long people actually look at the code being reviewed and how many have any comments whatsoever. If your finding real problems in a code review something else is wrong.
At least at Google, code reviews are definitely taken seriously by everyone I’ve ever interacted with. I get meaningful feedback on the code I send out, and I spend real time looking at code that’s sent to me. That’s mostly organizational, though; if you’re not serious about code reviews at Google, it doesn’t go unnoticed.
I definitely agree that if you’re not actually going to do a thorough review, there’s no point in having the code review facade, that’s just wasting time. But if you commit to them as a group, I think they can definitely be valuable, for precisely the reasons Mark mentioned.
The 90% is the difference between Google engineers and other developers.
That’s certainly not true. We’ve been doing productive, comprehensive code reviews on every project I’ve worked on for the last 14 years. The mentality that Google employs the cream of the crop of developers is somewhat annoying.
Agreed. There are plenty of great programmers who are not at Google, who are probably just as productive, and maybe even do code reviews without Google having told them to do it.
I was doing code reviews at all my companies, years before Google existed.
We’ve done code review the last few years and found it beneficial. We apply the reviewing without tools, where the reviewer walks over to the desk of the reviewee, and the reviewee talks the reviewer through the problem that was solved (if it was not already known to the reviewer which is usually on the same development team), and what were the reasoning behind why the chosen solution was actually chosen. That brings forth a discussion over the code, not just a “Ok”, and we’ve found that many times we end up rewriting an entire module, often as part of the review, because of the better understanding of the problem that has arisen from the review. So can’t do anything than +1 code review.
As an academic, and a low-ranking one at that, I generally work alone on the coding portion of my projects. A real code review is pretty much out of the question. But you can approximate it to some extent: you usually work on only one bit of your model or simulation system at any one time. Other parts may lie fallow for weeks or months; there’s no reason to look at them when they work.
So, make a point of looking over your other code (or a defined part of it) a couple of weeks after you last touched it. You’ve been away from it for long enough that you can look at it with fresh eyes, and read the code as it would look to an outside observer. But it’s still fresh enough in your mind that you know what it’s supposed to be doing and can fix inconsistencies and hard-to-read constructions without having to reverse-engineer it from zero.
It’s not code review, but it’s better than nothing. And it’s surprising how often I find potential bugs (unaccounted corner cases for instance) and clumsy idioms this way. The one drawback is that there’s nobody to push you to actually do it, so it tends to fall by the wayside periodically, especially when you’re busy with the non-coding parts of your job. Which reminds me, it’s really time to look over my reduced model again…
I like your idea of going back to your own code after a set time has passed. I did this recently, not because I wanted to see my code, but because I knew I wrote a clever function to do something I needed to do again and was looking for the thought process behind what I did.
When looking at my old code, I was surprised at some of the things I found. I decided to take 10 minutes of my day and fix the small issues I noticed, added comments, and rearranged small bits of code here and there.
Being in a small development team, I found this to be very helpful.
I have had a similar experience, though in my case its mostly caused by the fact that I was not trained as a programmer at all, and just learned by trial and error (I’ll admit that I am the lowest species of programmer, writing macros in Excel, but several people in my office do a lot of work with my macros).
Whenever I have reason to revisit old code I wrote, I find things that leave flabbergasted, which are simply the result of me not knowing any better at the time. Some of then are indeed maintenance nightmares, but mostly I see a lot of redundancy and inefficiency, which nonetheless horrifies me.
This is an excellent idea. Having worked on a significant project by myself I can definitely see the merits.
It was actually a situation like this that taught me the value of writing maintainable code. I noticed that coming back to a bit of a large project that I hadn’t looked at in months was very much like looking at someone else’s code. The more effort I put into making sure that code was understandable to someone else the more likely I would be able to understand it 6 months later. I can see how the same idea would allow you to do a very unbiased review when no other reviewer is around.
There is no reason why an academic can’t get code reviewed, unless of course you’re a one-man department and there are no graduate students. Otherwise, everybody writes code, and who has a more obvious professional stake in their code being good than an academic? Anyone you’d ask to look over a paper would be a good candidate to review your code. Getting a culture of code review established in an academic department would be a huge win for everyone. Let me also tout the pedagogical advantages of code review if you have a gaggle of grad students working on a project.
I’ve encountered this a couple of times when I’ve reviewed code that I’ve left fallow for too long. The code works, and works beautifully, but I had a hard time identifying exactly why and how. Of course, this is also coupled with a lot of new experience, and how I would write things differently now than then, but obviously I didn’t document the code nearly well enough in the beginning.
I now try to live by that old hack about the next reviewer being an axe murderer with your address, even if that next reviewer is likely to be only myself.
I understand the lack of other people to do code review: I’ve worked in a few places where I was the only “advanced” coder and no one else could read my code even though we were using the same platform (SAS).
One way around this is to create a community within your environment who may be able to review your code. At the University of Wisconsin, I started a SAS user group with this in mind as well as sharing knowledge and working together. I’ve already given and received assistance from the group, although it hasn’t been as detailed as code review.
Good luck!
Have you considered using any sort of tools (as a replacement for a human) to assist with the process? Though it is commercial, Atlassian’s Crucible would be a good example. Obviously there are plenty of others.
Things like Crucible aren’t a replacement for humans; they’re a tool that humans use to do code reviews.
You can’t replace the human. The whole point of code review is that the code is being viewed by someone else who understands what the code is supposed to do. If you could explain to a computer what the code is supposed to do in enough detail, the result would be another program – which would, itself, need to be checked for correctness!
You can use a variety of tools to *help*. There are a ton of nice static analysis tools that can flag likely errors. For example, you can write tools that do a pretty good job of identifying potential memory leaks in C++-like languages; you can write tools that flag access to uninitialized variables, etc. (In fact, I’d argue that type systems in most strongly typed languages are nothing but that: tools that statically analyze the program, and flag errors.)
I’m curious as to the technical implementation of your code reviews. Did you use a vcs that allowed you to share code with other people without merging it with the master/trunk branch (á la git?), did the reviewer physically pull a chair up to the author’s terminal, or was it some other means of wizardry?
Short answer: http://googlecode.blogspot.com/2008/05/guido-van-rossum-releases-mondrian.html
Longer answer: Google uses the commercial vcs perforce, which has a concept of “changelists”, files which are sitting in a client, ready to be committed, but aren’t actually committed yet. Anyone on the same perforce server can see what open changelists exist, what they contain, and where the clients are. (Maybe; there might be permissions things perforce admins need to set up to allow that – I’m not familiar with perforce outside Google) Combine that with an internal tool that can ssh into developer boxes, read files in perforce clients and display the results on the web, and… well, now you should go watch the video linked to in the shorter answer.
(And yes, Google hired the creator of python and then had him work on a purely internal tool)
Facebook has open sourced its code review tools, which integrate with git and svn: http://phabricator.org/
Basically you work locally and when you want to commit, you can send out a diff, which appears as a nice visual diff on a website, with inline comments and so on. Reviewers get emailed, and there is a workflow where they can request changes, reject or approve the diff.
I currently work at a defense contractor, and code reviews are a way of life for us (and for good reason).
As the lead designer/architect for our product, I am the reviewer for every piece of code written by someone else on my team. For my code, there are usually 2 reviewers from our software group. as well as the usual reviewers from QA and the occasional subject matter expert from systems. We also invite the Customer and get a lot of good comments from them.
We largely review for correctness and maintainability (style does creep in here sometimes). I am also responsible for ensuring that the code is consistent with the design and meshes with the rest of the product.
Having our Customer at the review serves both to allow them some visibility early in the project and to give them an appreciation for the review process we use.
Great article. However, it’s difficult to not see the irony that you didn’t have someone review it before you posted it. There are a number of typos in there 🙂
Ironic, perhaps, but this is a different context.
I worked for another big silicon valley company where code review was rampant. I fought against that practice for as long as I could. You see, like religion, reviews are a great idea in theory. And, like religion, they are terrible in practice.
Reviews often degenerate into nitpicks. It takes a lot more effort to understand code and make really productive suggestions and non-obvious bug catches. It does work great the first few times but is highly susceptible to hysteresis losses when done on a continued basis. And as you say, one can’t rush it but where does one find time to do a thorough job of it?
I find brainstorming and design reviews to be more productive. Code reviews as a surrogate to these activities is useless and ceremonial. I would rather have a team which is competent enough in coding and doesn’t need someone else to point out obvious issues. After all, automated style checkers, developer testing coupled with design discussions and reviews, with random code reviews to keep everybody honest, seem to be a better approach.
I could list dozens of anecdotes on how reviews didn’t help. But, I am afraid that this is one of those crutches that people won’t give up. But there are other ways to develop quality software notwithstanding the Google way.
Yes, they can devolve into nitpicks. I am guilty of being such a devolver. Finally after seven years of Google code reviews, I’d like to think I’m on the other side of that and reserve my nit picking for training new employees. Being a good reviewer requires good social skills, and like all good skills, require practice. I’m sorry to hear that you’re so turned off by the process.
Many people rebel against what others consider “best practices”. That doesn’t mean that the practitioners are idiots, but it may indicate that the rebel is insecure in some way.
I’ve done the at-your-desk code reviews before, and for that team they were absolutely the right thing to do. They were a hassle but the benefit outweighed that hassle by a substantial stretch.
If your reviews degenerated into nitpicks, you were doing it wrong.
I may be coming from a different perspective, though, since the projects I work on typically run only around a million lines of code and most of us on the team know large and overlapping areas of the codebase; any of the senior people could review almost any change for appropriateness.
In my experience it’s not about saying “yeah that looks good” or “no that looks bad,” but rather discussing what the change is for, why this approach was taken, etc. Often it seems that it’s the reviewee that notices the issues during the review. Fixing a bug before committing it is much cheaper than fixing it later.
Of course, if you wanted to start talking about pair programming or test driven development, you might find that explicit reviews are less necessary. 😉
By this: “At Google, no code, for any product, for any project, gets checked in until it gets a positive review.” Do you mean pushed to production? Or checked in? Or checked in to trunk? How do you even review code if it isn’t checked in somewhere? That’s not really what you meant, is it?
I am an advocate for code review but our process is post commit, and “looks good, fix this param, add that comment” etc is 90% of it. We tried some tools but nobody ever used them, so we have a code review channel in skype open all the time where we post links to commits. Larger more experimental code changes we know are going to be risky ahead of time are done in a branch or in an experimental directory on trunk and argued about a lot before use. I tend to refer to this as “design review”, which IMHO is more important.
Just getting the code right is relatively easy, getting the overall design/architecture right is hard and simple code review doesn’t really address that because the chunks are too small. How do you know the big picture makes any sense?
Google has a lot of custom tools for doing review. There’s a command-line tool that you use to send code for review. It talks to the SCM system that manages the code, finds what’s changed, bundles up the diff, and sends it to the code review tool.
So really, *every* piece of code gets reviewed before it gets checked in the repository.
As an ex-Googler, that’s not *quite* true. There are a couple of exceptions. There’s a way to do an emergency checkin that will get reviewed later (mostly used for rolling back stuff that’s causing critical problems, but could be used for, e.g., an emergency security patch). There’s also a private developer checkin area intended for stuff that you want to maintain in version control, but don’t intend to be part of a larger project, that doesn’t get reviewed. But as part of the day-to-day working routine, what Mark says is correct.
I entirely disagree, but I don’t suspect it matters. Code Review is one of those BS things that everyone thinks improves work, even though there’s not a shred of evidence to prove it.
There are, however, several negatives to code review:
1. They make people check in code less frequently, both because they don’t want to go through code reviews all the time, and because they realize the more code they have, the faster they can go through it in a review.
2. They provide a false sense of security (the second pair of eyes, as you openly admit, rarely catches errors).
3. They make people waste time ‘polishing’ code for other-consumption so they don’t have to deal with idiot feedback.
4. They are dreaded by less social programmers. Socializing is not the Holy Grail of programming. Many people prefer working by themselves as much as possible, and this works fine. Forcing them into social situations doesn’t help them, and many of them see it as punishment.
5. They open up the floor for code bullies and code morons – the ones who want you to explain your every choice and offer alternatives, or the ones who require hand-holding through the entire procedure or make idiotic suggestions (“It would be faster if you used ?: instead of if/else!”)
Code reviews, like pair programming, work for some and not others. Making them mandatory is just dumb, particularly when you admit that it doesn’t stop errors and is merely a social practice.
Obviously, I disagree with you.
In my opinion, a lot of your points come down to “if you’re working with a bunch of idiots, you’re going to suffer”. But if you’re working with a bunch of idiots, then you’re going to suffer whether you do code review or not.
(#1) is a cultural issue, and it’s easy to fix. If code reviews are done well and done quickly, then there’s no particular reason that people will stall on sending code for a review. In particular, I’ve seen that kind of behavior stop really quickly when a reviewer says “Sorry, there are three different changed mashed together in this review; please separate them”. Once that’s happened once or twice, competent people realize that they need to make each review be an atomic unit.
(#2) isn’t a problem if people understand what code review catches. As I said, the bug catching thing isn’t the best reason for review; the best thing about it is knowledge sharing.
(#3 and #5) share the “if you work with idiots, then you suffer” effect. If you’ve got bullies and morons in your group, then you’re going to have trouble no matter *what* you do. You can’t get around that.
(#3) also isn’t a bad thing. One of the big pros of code review in my experience is precisely that people polish their code more when they know someone is going to look at it. Lots of cheap hacks that would work for now, but cause trouble in the future get committed in non-review environments, because someone just wants to get something done. But in a code review environment, they don’t want to show that kind of sloppy shit to the reviewer, so they fix it. That’s a good thing.
(#4) in particular I think you’re seriously wrong about. I’m painfully, disfunctionally shy, and I’m a walking train wreck in social situations. But code review isn’t a social thing. It’s a technical thing. And unless you’re working on something so small that you’re the only person working on it, working collaboratively with others is absolutely unavoidable. So you’re going to *have* to interact with your coworkers. Code review is less social and less stressful than many of the other interactions that you’re going to have to live with if you want to be successful.
The value of knowledge sharing can’t be overstated. We all have a tendency to end up with spaghetti code if we truly believe our eyes are the only ones that will ever see it. Even when writing code for iOS, which I do entirely on my own, I walk my wife through the code. She enjoys it and it helps me organize my thoughts.
I admit I tend to lean toward less process, but I disagree with several of the “negatives” here:
1. If you check in less frequently to review less frequently, then you are using the wrong CM tool. Distributed version control lets you continue to have fine-grained commitment history, even if you want to fly solo and under the radar at longer intervals.
3. I agree with Mark here. I don’t work with idiots.
4. I read Mark’s point here slightly differently. Although reviews are usually by their nature a “social situation,” perhaps the real benefit is the forum for sharing and learning insight into best coding practice. This is why I think even junior staff reviewing code written by senior staff is a good idea. I often learn from reading other people’s code.
5. See (3).
In short, I think the practical problem that argues against *mandating* code reviews is one of time. I don’t know any coders– and I know a lot of good ones– that are actively opposed to others reviewing their work. But it’s hard to justify stopping to read when you have so much writing to do. I still think it’s a good idea, but it’s hard to convince management of this when the time and cost that code reviews can *save* is by its nature amortized over a long period of (hopefully) bugs that are caught before release, design that is improved and more frequently re-used, etc.
Thank you for displaying your objections in a numbered list; it makes them easy to respond to. I think your attitude is colored by exposure to a highly dysfunctional code review process.
(1) I have found, as Mark alludes to as well, that code review in fact has the opposite effect.
(2) I know I’ve certainly gotten some amazingly dodgy stuff through code review. I’ll point out though that because I have, (2) doesn’t really apply to me anymore. I don’t get a false sense of security from CR, because I don’t get a sense of security from CR in the same way. However, I’ll concede that it’s possible CR gives a false sense of security to people who know the engineers are doing it, but don’t write or review code themselves so don’t have a good sense of what it can and can’t catch.
(3) I’ll admit that there’s been code I’ve cleaned up because of the review process (either before sending it out for review, or after getting feedback) that I wouldn’t have cleaned up if I were writing it only for myself. I find it very hard to classify this time as “wasted”, however, since the end result is that I can pick up the code for anything at Google and have a significantly better chance of being able to read the code easily than I can with some random open source project. Maybe I’m not understanding the kind of idiot feedback you’re talking about.
(4) Ah, I think I see the problem. You mean something different by CR than what happens at Google. You mean the kind of thing where everyone sits in a meeting room and the code author makes a little presentation about their code and you get other people in the room telling the person presenting “X sucks, don’t do Y, and what were you smoking when you wrote Z?”, and maybe there’s someone else taking notes on stuff to be changed; lather, rinse, repeat. Agreed, that’s awful; don’t do that.
CR at Google isn’t like that. First off, it’s mostly not done in person: these days we have this web tool (See the author of the tool talk about it at http://www.youtube.com/watch?v=sMql3Di4Kgc) that automates the process, but even before that CR was done over email, as email threads. It’s about as intimidating to highly non-social types as posting to your team’s mailing list. Secondly, it’s been my experience that many reviewers will use different gradations of comments, saying “nit: don’t do X” instead of “don’t do X” when they (the reviewer) realize that X isn’t actually that much of a big deal and are fine if, having received the advice to not do X, you go ahead and do X anyway. (which is to say, even in the non-confrontational setting occasionally reviewers deliberately make it even less confrontational) Thirdly, and I’m not sure if this is really a difference with your mental model of how CR happens, at Google CR is fundamentally a peer process. If A is reviewing code written by B, B will very likely soon be reviewing code written by A. People who aren’t writing code aren’t reviewing code, and you don’t get a hierarchy of “Well, I’m a level 4 engineer, and you’re only a level 2 engineer, so I can review your code, but you can’t review mine, and I get 2d6 more hit points”. Finally, and this relates back to the fact that it’s done on a web tool or over email, CR happens out of band so you can get to it when you’ve already context-switched out of coding for some other reason and so it isn’t ever the kind of interruption to flow that meetings can be.
(5) I don’t understand what you mean by “the ones who require hand-holding through the entire procedure”; perhaps this relates to the different experience you mean by CR. I agree that it does leave the door open to nitpickers, but one of the nice things about the way CR is done at Google is that everyone being sent the review sees everyone else’s comments, and if other reviewers disagree with what one reviewer is saying, they’ll call them on it. Another nice thing is that the Google corporate programming style guides already cover most of what nit pickers would pick at and are automatically checked, so that’s usually taken care of quickly before sending some change out for review.
This article is one of the most clever and interesting one I have read for a long time.
I’ll keep it warm and spread it around.
Thanks Marc 😉
I personally think pair programming is better than code review!
It’s funny how many times while reading this article I thought about pair programming and how the benefits are the same. It’s almost like code review is a sort of asynchronous low-density pairing.
Which is exactly what’s required. I know it’s heresy to the agile folks, but unless you’re working on an extremely hard problem, pair programming is a waste of time. (Compared to after-the-fact code review)
Do pair when you design, or when you debug hard problems, by all means.
I know, pair programming is just “turning that to 11” – I prefer to turn “not wasting time” to 11 😉
Groby, ya gotta do it for a while to get into the swing of it.
Of course, I’ve been a Xealot for ten years, but I’ve loved my time pair programming with people of all abilities. With junior people or senior people the knowledge transfer alone makes it highly visceral. Maybe though by “hard problems” you mean “not trivial crap,” which I’m not sure if I entirely agree with but I try not to spend too much time doing trivial crap… 😉
This is an interesting discussion. Here you are dealing with the same problems faced by writers in small critique groups when they are part of a writers’ club.
Successful groups spend a little time going over (with new critiquers) what kinds of comments are helpful/desired and why, and what kinds of comments are not helpful/not desired and why. This is immensely helpful in turning the group into a positive and useful experience for all participants.
–Lynne Diligent
Dilemmas of an Expat Tutor
expattutor.wordpress.com
I completely agree that code reviews are very, very useful technique for the reasons you described. In my previous company I came across a problem of some people doing review quickly, and because lack of coding rules and standards it was hard to argue about important points.
Anyway I wanted to ask if you consider running the code as part of the review? I came across developers quickly fixing the code without even checking it for basic test scenarios described in the task, which I find unacceptable. So I ended up running and testing the code – doing the job of the developer and tester. I still think it is worth it, but can be sometimes time consuming.
In my project at Google, we had an automated testing system which ran a test suite over the code when it was sent for review. So the reviewer got a copy of the change, and the results of the test run.
One of the most important rules of code reviews:
NEVER do code reviews in triplets, or anything more than a pair.
It doesn’t work. There are arguments between the two reviewers on the back of the reviewee as he/she waits. There is an ego fight. It’s useless – try it once.
Its a complete disaster and I’ve seen it in many work places.
The way I’ve seen it work, each change is reviewed by *one* person. I think that you’re absolutely right that it’s very important that there be one person responsible for reviewing each change.
And that person should not invite other junior/seniors to “learn and participate” actively on the back of the person being reviewed. Sooner or later religious arguments begin, ego kicks in, and and the poor guy never gets his code reviewed – and all of those excellent tips you’ve shared would be ignored, because its no longer a code review but an ego fight.
I refuse to be reviewed by more than one person, and I refuse to share or participate in other person’s review with another reviewer. If i must be there, i stay silent.
When I used multi-person reviews we didn’t have this ego problem. The purpose of the review meeting was to raise issues, not resolve them. If the discussion about a point lasted more than a minute it was an issue, it was noted on the appropriate issue list and we moved on the the next point. The author took the direct issues and resolved them in his own time and his own way using advice from whoever seemed most appropriate.
We also had a strict time limit. If the meeting took more than an hour we terminated and marked the review as incomplete. It was up to the author to decide whether to continue with the current version or to revise it for the next meeting.
We also started each meeting with once around “What’s the best thing about this document?”
This has not been my experience, at all.
That is, in my group code reviews are routinely sent to multiple people, and CC:ed to a team mailing list. (a different one than the main team mailing list, so people can filter it separately) Despite this, I don’t see reviews getting bogged down in these reviewer fights you’re describing.
I could speculate as to why – maybe the interface Google uses shapes the interaction so that doesn’t happen? – but to be doing anything other than pure speculation I’d really need to observe the pathological case you’re talking about.
I can only agree with the article. At my last company there were NO code reviews at all…which is sad
Next time you review code, consider searching for things to praise as well as things to correct. I find praise broadens my relationship with the person who wrote the code. I become less of a judge, more of a collaborator. Searching for exemplary details changes my attitude toward code reviews, too, making them less of a burden, more of an opportunity for discovery. I consider a review a failure if I can’t find something to commend.
I strongly second this. I’ve performed and received hundreds of code reviews, both inside Google and out. Rarely do people take the time to comment that something was done well and should be repeated. Yet people learn, in general, much better from a mix of positive and negative feedback than from purely negative.
Commend people for good practices. “Hooray for good variable names!” or “Thanks for pulling this into its own module” or “Nice handling of the library returning an error”. It doesn’t take much, and it dramatically changes the flavor of the review.
Interesting article. So the next question is “Does Google have a set of Coding Standards?”. Its been my experience that it is much easier to look at other people’s code if both parties have the same coding style. Of course getting a group of programmers to agree on coding standards is more difficult than getting Republicans and Democrats to agree on a budget!
I think you raise a good point, at google I understand they have Readability tests, that you must pass before you can submit code for code review.
So I think Code review and Coding standards go hand’n’hand together, of course readability doesn’t include looking out for quality design patterns, bad anti-patterns, or limitations on scalability or any other kind of architechtural challenge which go far beyond the initial scope of programming.
Yes. http://code.google.com/p/google-styleguide/
This is a very interesting post. I mostly agree with you, but I’ve also never had a positive experience with code review in the past. I’ve found programmer personalities and egos a major barrier to entry in getting this right.
I’ve found it’s always taken up enthusiastically only to be gradually dropped over the ensuing months. I think it boils down to your point of the method of review being wrong: you should be seeing if the code WORKS, not if you LIKE it, which in my experience is what most programmers battle to do.
I would be interested in knowing how you got over these social issues – and do you believe most people liked them, or did they just pretend to like them because they had to be done?
I also want to know how Google ensures that code reviews are mandatory. Is the culture that powerful? Do programming supervisors require this? Does anyone skip this process? How do you ensure that it is not skipped? My interest is to adopt it in a way that sustains it.
Google uses tools to enforce it.
There’s a script run every time you try to submit a change to the repository. If the change hasn’t been approved by a reviewer, then you can’t submit it.
very good to know. Thank you.
Nice article. I am going to practice it in my day to day work more religiously now.
As I read your article and the responses, one of the most contentious points seems to be about “my way vs the coder’s way” or “mere aesthetics”, “code bullies”, etc… Since there is more than one way to code, how do you prevent these arguments?
Pair programming has many similarities to the style of code review you describe. The way we avoided squabbles was to have objective coding standards that were the guidelines. Note I said “guidelines.” Even then, there were very few mandatory things. There is still judgment. But if you advise all reviewers to check whether code conforms to the guidelines, then the implication is that any other variations are just stylistic differences that you should respect and not argue about. So, for example, if your team agress that all public methods must guard against non-null arguments unless null is valid, then it is fair game to point this out. If that is not your standard, then you may point it out as a reviewer. But if one always points out stuff outside of the guidelines, it suggests the reviewer is being too picky. This approach requires some regular mechanism to suggest improvements in the guidelines.
I strongly agree that 100% coverage code reviews are a must. We’ve been doing this for close to 10 years now. We find bugs faster than we would otherwise, and we find bugs in corner cases that testers might miss.
You don’t need any special tools to do it. With a branching DVCS, just ask another developer to review branch diffs before merging to main. With a VCS like TFS, you can make code review part of the work item workflow and review the changesets linked to the work item.
O to be able to use DVCS, I’m so jealous…
Maybe one day when we have 100s of TB of disk on our local machines… Or hg bfiles is fixed. 😉
I used to be a lead developer at a small company and made code reviews a must for my team. The collobrative environment was excellent and rarely would QA find a problem with our output.
Lo and behold the company hired an outside manager for our team who talked the talk but in practice cared naught for code reviews. The product quality plummeted and political battles broke out. The team was broken up. Needless to say, I no longer work there and literally half the company has been laid off or quit.
I found the major problem in any office environment is the egos. This is pretty much what defines office politics. Code reviews force egos to be set aside a little. Programmers with big egos are cowboy coders and not team players. In my opinion you don’t want these people on your team. Having reviews makes the office environment better, the product better, and is to me a clear reason Google is successful.
I work in the medical field and we bring software solutions to market that directly interact with human patients.
We are required by the FDA to do code reviews. It is amazing how much more robust your code is when you are doing code reviews.
Brandon
Pingback: Quora
Is anybody at Facebook listening?!
This is an inspiring post, thank you. In contrast, I read recently how Ken Thompson (co-creator of Unix) eschewed peer code reviews back in the day because “we were pretty good coders”. I’ve shared my thoughts/response at: http://blogs.sas.com/sasdummy/index.php?/archives/264-Are-you-too-good-for-code-reviews.html.
Maybe if the original Unix team had done code reviews then the sh(1) code wouldn’t have been so hard to read and maintain, at least for non-Algol programmers. 🙂
My first company did code reviews, and that is what I learnt to expect from a software development company.
My current company does no code reviews, and unfortunately the product quality and source code quality suffers. There is no way that I will succeed in getting that kind of quality check introduced unfortunately.
I am all for unit testing and code reviewing. I have worked in companies where it was done, and companies where it was not done and it is easy to tell the difference.
“The biggest advantage of code review is purely social. If you’re programming and you know that your coworkers are going to look at your code, you program differently.”
Man is that the truth.
I have worked on a couple of large industrial process control system upgrades over the last 15 years. The project team was made up of internal company engineers (read that as system engineers maintaining the legacy systems pre upgrade) and a bunch of programmers from an IT department.
It seems that code reviews, that were stressed as so important to the internal engineers (self included) and were strongly enforced when we had performed portions of the code migration, were not equally enforced on the “experienced” enforcers.
As a result, quite a few fundamental coding and code migration errors had been made that had a major impact on the quality of product produced from the process control system once the upgrade was “technically complete”.
For an upgrade that was commissioned in 1998, I am still uncovering the occasional code migration error produced by the “experienced” programmers.
Code reviews only work if they are used consistently across a project team.
Too often code review is just a check mark for I have looked at the code. This does not translate to efficient code or even effective code.
“The biggest advantage of code review is purely social. If you’re programming and you know that your coworkers are going to look at your code, you program differently.”
Not always true.
I think I disagree slightly, Mark, with what you describe as the “biggest advantage”.
Yes, it is a big advantage that I write my code to a higher standard when it’s going to be sent off for review, but I think there’s an even bigger one: doing reviews makes me read the rest of the code in the project more often. This makes me familiar with more of the code than I would be if I were simply reading what I needed to read to make the change I was making, and helps me catch the wider implications of some change.
Also, as a Noogler, CR kicked me into writing better code – not just the feedback on my own stuff, but reading other stuff the team had written, and watching other reviews to see what comments were made, and seeing how code changed in response to those comments (which helped gain an understanding of what kind of comments were useful, and what was painting the bike shed).
Note that if you find yourself working outside Google in a team using git, setting up Gerrit (http://code.google.com/p/gerrit/) to run your central authoritative git repository and setting up Hudson (http://hudson-ci.org/) to run your test suite as soon as a patch is posted to Gerrit for review can get you rather close to the setup Mark was talking about.
You might also want to set up some wrapper scripts to common git commands to make your workflow easier, or grab the set of scripts the android team has released as repo (http://source.android.com/source/using-repo.html).
Two things that nobody brought up (that I saw):
Code reviewers have a different set of the total codebase in their head, and can point you to libraries (or code that should be a library) you had no idea existed, so you can avoid reinventing wheels. I remember a good chunk of my first code review was of this type.
They can also see the forest from the trees a bit better than you and can suggest refactorings that really help clear up the code. Phil Coakley, my mentor when I started here is a master of this.
One more point – Designs. If you don’t know what it’s supposed to do, you can’t decide if it’s correct. I’ve sat in on far too many so called code reviews where I was merely handed a stack of code and told “this is what we’re reviewing today”. That is useless.
Understanding both the context the code needs to run in, and what it’s task is, and what issue it’s supposed to deal with are all critical to make a review work.
Hello Mark,
Excellent post on a very important aspect for us engineers.
I just have a confusion here, you wrote the reviewer needs not to judge the solution but the implementation of the solution selected by the coder. So shall not the reviewer tell the coder, ( in a friendly manner, not commanding manner at all ) about a better solution or a solution he thinks will combine well with the existing code base.
I hope you get time to answer this one.
That’s actually an opening into a fairly subtle but important point: Code review isn’t a panacea, and it’s not the sole important factor in the process used by a development team. A successful team needs to communicate. Code review is one aspect of that. But it isn’t sufficient.
A code review should analyze and critique the implementation as it exists. A code review that says “Oh, you should have done it *this* way” isn’t really a code review. It’s a design review. Design review has its place – but that place isn’t during the code review. You really should be discussing design with your team, and agreeing on a correct design before anyone starts to waste their time coding. Once it’s been coded, that’s not the right time to second-guess the design.
I know that that sounds sort-of petty. But it’s indicative of something very important. When you let people do that kind of retroactive design critique, it will frequently devolve into petty squabbling. The message of a code review shouldn’t be “go throw away your work because you didn’t write it the way that I would have”. When you allow it to be that way, then you’re allowing things to become personal, and that leads to disaster.
While saying “you should have done it this way” is counterproductive, a discussion about “did you consider this alternative approach” can be very useful to both parties. I’ve certainly seen my fair share of “oh you mentioned this thing in a review some time ago, and it looks like I need to look into that further at this time…”
I’m curious how Google handled inspection of new development. Most studies show that inspections work best on small bits of code at a time. How does Google handle inspection large new features which might be thousands or tens of thousands of lines of code? Inspecting all of that in one bite generally doesn’t work.
The usual solution is to split the giant new feature into a series of smaller changes, each of which builds and passes tests but may not do anything interesting on its own (if you’re writing a new feature, you generally want it disabled by default anyway), and send them out individually. For example, if you’re writing a multiple-process pipeline to generate some new shiny data, you can send out one step at a time, or if you’re building some new backend server, you can send out the minimal functionality then throw in additional pieces once that’s checked in.
If you do this early enough in the process (as in, don’t write your whole feature and THEN split it into smaller pieces), if your reviewer wants you to make design changes, you’re not stuck redoing quite as much work. Also, if you do get rolled back, rolling back a small change is much less traumatic for everyone involved.
Great article. I work in Release Management in Yahoo Mail Service Engineering, and we use codereview heavily. One thing I want to point out is that codereview is not just for ‘developers’. Operations people should use it too, for exactly the same reasons.
We use Google’s Review Board heavily for these sorts of operations code reviews. For example, if we are going to push a config file change out to servers, that file change must be submitted to Review Board first. We also have an internal tool that takes subversion diff output on the command line and submits it directly to our Review Board server, for maximum ease of use.
Another reason this is a good thing to do is it helps speed up the training of new hires. You can have people start submitting codereviews as soon as possible, and use codereview as a training exercise.
FYI, Review Board is not from Google. It’s from a couple of engineers who work at VMware (it is not owned by VMware, however).
I miss what is probably the dual most important advantage: when you review code (or for that matter, anything), you LEARN.
A.lot.
Even from obvious mistakes.
Sorry to hear you’ve left Google, but best of luck in whatever you decide to do next! In a strange act of conservation of former Sciencebloggers, I started working at Google a few weeks ago 🙂
A question for software folks who have seen code reviews done both synchronously and asynchronously: Which do you like better?
At my previous company (working on Second Life) all of our code reviews were over-the-shoulder: we would pull up graphical diffs with WinMerge or OpenDiff, then sit side by side and review. Once we opened multiple offices we would screen-share with VNC and talk over Skype. I found this to be an important social tool – it forced people in remote offices to talk to each other, and was an important conduit for people to get to know each other personally. But time zone and work hour differences sometimes made it hard to connect. Code was reviewed fast — often as soon as it was written — at the cost of interrupting another engineer for a synchronous discussion.
At Google, code reviews are asynchronous. You upload diffs to a web tool, then the reviewer adds comments to lines or blocks. Discussions go back and forth via automatically generated emails. It’s technically simple to get a review from someone in another time zone, or who works early or late. Discussion is slower and harder. Your code must be self-explanatory — while it’s technically possible for the author to add comments, few people do. This might be a benefit.
Has anyone else done it both ways? Which do you prefer? Why?
The last two groups I’ve been in have mandatory code review policies. For large or architectural changes, I prefer going over code in person – this helps with the knowledge transfer aspect.
In my previous group I was more experienced and usually felt I was a stronger programmer than most people doing my reviews, so this tended to be helpful for them but I didn’t feel like I learned much from them.
My current group has a code review committee, and it turns out I sit between two of them. They are both strong programmers and more experienced than me in the language my new group mainly uses, so this is quite helpful for me.
For more minor changes I generally commit and ask for a review of the change set.
Small world. I too just started working at Google a month and a half ago, and one reason was a post you wrote about Haskell, and how not to walk into a Google interview saying you know Haskell if you don’t, because there are people there who do.
And I thought, wow, what a company to work for that doesn’t automatically say, what’s Haskell?
Anyway, sorry our paths didn’t cross. I have read most of your category theory blog posts and loved them. Maybe you can post some more details about why you joined Google and why you left, so I can compare them to my own experience there.
Code reviews are important. But I disagree that it’s a good idea to tie them to commits.
As mentioned by someone else tying code reviews to commits discourages people from committing often and promptly. Instead of a lot of incremental changes with full history int the SCM you end up with large batch commits with no idea why things were done that way.
I much prefer the model in the open source world where there’s code review because every patch is posted publicly and commented and discussed (ad nauseum). It might not get discussion until after it’s committed or it might get discussion before it’s ready to commit. There’s code review not because it’s a bureaucratic checkbox that has to be ticked to make any progress but because people actually have things to say.
I know I’ve arrived to this very late—after the event, as it were…—and this isn’t quite on-topic, but a little while back I wrote about using a mix of literate programming and regression testing during coding. (I was disappointed at the lack of comments, esp. compared to what you have drawn here!)
The reason I mention this is that reading your article I realised one aspect of this that I didn’t mention is that effectively the approach effectively includes trying to code as if it were for someone else to read and (re)use from the onset. That, in turn, encourages a number of the things you mentioned in your article.
It does take more work that a ‘just make it work’ approach, but in my (anecdotal) experience it pays off in the longer term, especially for code you re-use or later extend and adapt.
[I haven’t read the comments before writing this – there are just too many! My apologies if something someone else has written pre-empts this late thought.]
Great article, I also am in favor, in general, of code reviews. Your specific experience was probably better than others because of the Google infrastructure and resources. For smaller companies, the resources and infrastructure may not be available, so the CR process is more manual. More manual = more social = more egos. As a reviewer, I try not to nitpick, and provide constructive comments. Also, it is ultimately up to the code author what is committed, so they can choose to ignore the comments from reviewers, if they think it is best.
I think the asynchronous code review, similar to Google, would be better and more agile than scheduled meetings.
As a software writer/reviewer, I have a more complete understanding of the problem only after I’ve reproduced the problem, and tested that it has been fixed. A better understanding of the problem means a better understanding of the solution, which can lead to less nitpicking. Also, I may “pass” some code while reviewing only by reading, but then with few tests of corner cases find a bug. Does Google encourage/require reviewers to test the changes? Also, is the author encouraged/required to also submit unit tests for the review?
Great stuff, but God is a capital “G”. That’s my code review 😉
And here we all thought he was a big guy with a beard up in the sky. It turns out he’s just an uppercase letter.
You learn something new every day.
I shared this blog post on Twitter from @SmartBear a while ago and thought the free eBook, “Best Kept Secrets of Peer Code Review,” may be of value to some of you.
You may download it here: http://smartbear.com/best-kept-secrets-of-peer-code-review/
There are a bunch of other code review related resources that aren’t heavy on marketing as well here. What comes to mind reading through the comments here are:
Improve Quality and Morale: Tips for Managing the Social Effects of Code Review and;
Pros and Cons of Four Types of Review (which discusses pair programming vs. code review, etc.)
http://smartbear.com/support/articles/codecollaborator/
Hope you may find these a nice addition to MarkCC’s informative blog post.
Best,
Alex
SmartBear Community Manager
FWIW, Code Collaborator (the SmartBear product) is a tool that we use for our in-house code reviews.
For the most part, I think it’s been a net positive on our job. It does have its quirks that can drive us crazy, although that may be because of the manner we use it. We tend to have a “review meeting” to discuss any big issues, and to get our Customer involved. However, the Code Collaborator review can stay open for a couple of weeks as the author and reviewers work together resolving any outstanding issues.
In my first weeks at a medical electronics company I introduced a document review process loosely based on the Ethnotech Review Handbook (I think that was the title, it has been many years). Every document we produced had to go through a formal review process, four or more reviewers, a formal review meeting, keep written records. “Document” included source code. The process worked well and was gradually adopted by departments other than the original software engineering where I started it.
Then one engineer, working on his own said, “I’m not going to have my code going through a witch hunt” and management let him get away with it. Before finishing the project he left the company and his work landed on my desk. It was dreadful. Eventually it was discarded. After that, management wouldn’t let anyone get away with it.
Three things made it work. One is that the goal of the review was to add value to the document, not just picking up mistakes. The second is that management was not involved beyond saying, “Thou shalt perform peer review”, they did not attend the reviews nor did they evaluate anyone’s performance on the basis of reviews. The third thing was that it was truly peer review, today I add value to your document, tomorrow you add value to mine.
One difficulty was that we liked to get one reviewer who was not familiar with the project. After a while finding a naive reviewer became difficult, and we would settle for someone who hadn’t reviewed anything in the project for a couple of months. There were enough projects going on for this to work.
The review process was part of the culture of that company and I found out later that when a bunch of people moved to another company they took it with them and it worked just as well in the new place.
Elsewhere I have had the one-person code reviews you talk about. They are certainly better than nothing, but their value depends very much on the quality of individual reviewers. With the multi-person review meetings you get a much higher and more consistent level of review as each reviewer learns by watching the others.
“The biggest rule is that the point of code review is to find problems in code before it gets committed – what you’re looking for is correctness. The most common mistake in code review – the mistake that everyone makes when they’re new to it – is judging code by whether it’s what the reviewer would have written.”
So code quality is not a factor when reviewing code?
Of course quality is a factor. But “quality code” is not the same thing as “code written exactly the way that I would have written it”.
I’d argue that quality is part of correctness. “Correct” doesn’t just mean “gets the right answer on our test cases”. It includes algorithmic soundness, stylistic validity, testability, documentation, maintainability. Code that can’t be understood by the reviewer, even if it generates the right result, is not correct code.
Is this usage common? It’s pretty much impossible to talk about code quality without using words differently than anybody else, but I’d like to argue for ‘correctness’ being reserved to meaning that the code does what it’s supposed to do, leaving the other things you mention under the general term ‘code quality’ (or ‘good code’).
What is the review process used? Without a description of that it is quite difficult to decide if the reviews at google are good or not. Also, are there any measurement available to quantify the improvement in code quality?
Giovanni, the tools (which to a certain extent codify the process) used at Google are mentioned above. Perforce-based projects use an internal tool called “Mondrian”; Git-based projects use a very similar, but open source tool called “Gerrit”. Both are described extensively online, just search for those names.
Τhe code review also serves educational purposes in that which makes…
I would like to add another “thing you should do.” Unit testing. I have been in this business for 30 years, but have never been so humbled until I started unit testing. It is amazing what gets found by your unit testing of even some simple routines.
This being off topic, I will stop there, but I maintain that there are at least 5 great reasons why unit testing makes sense.
Pingback: Things Everyone Should Do: Code Review | 余修忞
Pingback: 谷歌是如何做代码审查的 | Jzg's Blog
The ‘social’ aspect of code reviews you talk about is known as the ‘high school reunion’ effect.
Pingback: 谷歌是如何做代码审查的 | Welcome to home of KyleWong!
Pingback: CodeReview实践 | 编程
Pingback: Code review (inspections) | La Vie est Belle
Pingback: A few day to day tasks of a Development Team Lead | randonom
Pingback: Muhammed Tahiroğlu
Pingback: 【观点】代码审查：大家都应该做的事情（转） | 南龙的小站
Pingback: 【观点】代码审查：大家都应该做的事情 | 南龙的小站
1) Just want to help increase your credibility; you used “every” instead of “ever” in two sentences:
“Of course, nothing is *every* completely simple”.
“If you do, then no one will *every* be left hanging for a long time waiting on you.”
2) Great article. I like your advice and will endeavour to try use this as much as I can.
Its been really astonishing to hear that you are quitting google but the work you had done is really very fine.One thing I want to point out is that code review is not just for ‘developers’. Operations people should use it too, for exactly the same reasons.Writinghunt blog
This post is not protected by any spam prevention software? It’s got spam all over the place.
Code reviews waste people’s time, here’s why:
1) The only bugs it catches are the easy ones, the ones that get caught anyway from testing. 99% of the time you don’t find a bug anyway – more wasted time.
2) Spending time familiarizing yourself with others people’s code in an arbitrary excercise will never be as useful as spending time familiarizing yourself with others people’s code while your working on something that’s ACTUALLY USEFUL. Look at this way, you can either kill 2 birds with one stone or you can kill 1 bird with one stone.
All the people who are in such great favor of code reviews, don’t understand opportunity cost. Generally it’s the same people who think every function should be unit tested, doubling or tripling the development time of the entire project.
We have one guy at work who insists on Code Reviewing everyone’s checkin. This could be a good thing, but usually, he’s always the one who’s late for Code freeze, the one’s still working on his code and fixing his own buggy code after the code freeze date.
There’s a time and place for code reviews: it’s when everything else has already been done. That means, you should have already finished ALL your sprint tasks, reviewed your own code, tested your code, thought of every possible test and corner case, deployed it to the testing environment and re-run the basic tests. Then when you’ve finished everything else, and have nothing left to do, you can go and review other people’s code.
I am working right now in trying to implement code reviews using Crucible and my question is that if doing code review after commiting is a good practise or not. From my point of view it would be better doing code review before commiting anything into the repository but using Crucible there is not other way of doing it, what do you think about it? Do you use other tools to avoid commiting non-reviewed code?
Thanks in advance!
“The most common mistake in code review – the mistake that everyone makes when they’re new to it – is judging code by whether it’s what the reviewer would have written.”
That statement right there is how I know you know what you’re talking about. It’s so true. Your job as a technical leader isn’t to do people’s thinking for them (that’s what they’re paid to do). As long as code solves a well-defined problem whose definition was agreed beforehand, I don’t care whether someone implements a solution that reflects exactly how I imagine I would have tackled the same problem with hindsight.
There are only two ‘stylistic’ (as opposed to functional) issues I’d ever pick up upon: 1) if variables are named “String1”, “String2”, etc, instead of being given actually meaningful names (I rarely see this issue these days, but it still happens occasionally). And 2) If the project has a particular architectural approach, I don’t expect to see that approach ignored or cirvumvented in a developer’s approach. e.g., if there is a data access layer that manages communication with the database I don’t expect to see a UI that has a database connection string and a direct call to the database embedded within it. And if we’ve agree to use Unit Testing I expect to see any solution that’s checked in to be protected by meaningful tests. (It’s nice that your solution works right now whilst several assumptions you’ve made remain true, but I still want your code to do what you designed it to do when the new graduate we’ve just hired checks in their first ever piece of code 18 months from now whilst you’re on holiday, and unintentionally breaks your solution in the process).
Pingback: 5 things to do before paying your website designer - thisismyurl.com