I personally review all of the code from anyone on my relatively large team when they check it in. When I first started at my current organization everyone thought this idea would be absolutely crazy. The objections were usually in one of two categories. From engineers it was "My code doesn't need reviewed! Why would you review my code? Do you not trust me?" and from management "There is no way you will have time to review that much code! You'll slow everything down!" Neither of these is true.
First of all I let every engineer review my code (and encouraged it). They found problems. They laughed. They realized I wasn't embarrassed by having to fix my code... and they realized it was ok for them to do the same with theirs. As for managers, if you review code early in the development process there just isn't that much to review. I wouldn't go line by line and check every possible thing. I generally am looking for code smells. If tests are there and CI runs then my biggest concerns can be found relatively quickly, commented upon, and then I can move on and so can the engineers.
6 months later, I have had a number of engineers say that would never want to have it any other way again. Frankly, that's how it started with me too. I really enjoy code reviews.
I am enjoying Ex Catmull's book "Creativity Inc" about his time founding and running Pixar and his struggles to build a creative company that keeps being creative.
The parallels to software engineering are many and obvious, and I encourgE you to read it (candour, search for quality etc).
But the big one that struck me and I am looking for a way to try it is in "the brain trust". A group of 20 or so directors and producers gather every three months to watch the latest version of a film (I guess they all meet more than three months as they make more than one film at a time but whatever). The goal is to give feedback on what does and does not work as story arcs and emotional content and so forth. Not to fix it, but to raise the issues, and not to proscribe solutions but to point out blind spots.
Daily code reviews are great, but stepping back every few months with comfortable peers and just looking around a system is something that I would love - it seems to only happen after six or more months when things are starting to break.
You can make code reviews required by using something like Gerrit (https://code.google.com/p/gerrit/) and having it be the gate to your master branch.
The system I use at work is:
code -> push to gerrit -> gerrit automatically runs all unit tests and if they pass you get a +1 from gerrit -> get a code review + approval from a colleague -> submit.
So to get a piece of code to production you need both all unit tests to pass AND a code review. I am a big fan of this system and gerrit
Are there really places doing post-production code review? That sounds like a terrible practice.
I've worked for places that didn't have code review, and places with pre-production code review. The ones that didn't code review brought in code review practices (like they should). I have never seen a post-production review process though.
There are, but they frame it more as an "architecture review". Often this takes the form of a 3+ hour meeting going over all code that devs have committed in the last month or so.
It is a terrible system. If you aren't reviewing code before it goes in you are missing a HUGE opportunity to review for bugs and correctness before a client experiences a crash.
There are plenty of places that do code reviews post-commit, but these places pretty much always just have their CI push to a staging server and not prod.
From the other side pre-prod code review just delays software getting into production. Delays here tend to lead to bigger, riskier changesets.
I'd rather continuously-deliver sub 1hr of changes to production several times a day. Pair & Mob programming provide continuous code review pre-commit, but they don't provide all the benefits of traditional code reviews. It can be valuable to sit down as a team and or with others who didn't work on some code and think through how to make it better/improve it asynchronously.
From the other side pre-prod code review just delays software getting into production. Delays here tend to lead to bigger, riskier changesets.
That makes no sense.
If delays introduced by pre-prod code review result in "bigger changesets", you're clearly doing it wrong. In fact, if you batch up commits to deliver to prod on some semi-regular basis, the exact opposite should happen.
Individual changesets would be exactly the same. Velocity may slow down (that is, it may take longer for any given changeset to make it to production), but you're producing higher quality code in exchange.
This depends on what you are looking for. For style and "obvious" bugs? Agreed.
For finding bugs? Couldn't be more wrong. Functionality based reviews pick up far more defects, if done properly, than reviews of change sets. And it's because you pull back and think about the entire context of the feature.
Of course they also require a much more significant time investment and need to be run differently than a standard code review, but they are also much more valuable.
I agree sometimes you need to review a piece of functionality. But I think of this more of an escalation of a change set review. As in: 'this code stinks, why is that, lets look at the bigger picture, mmmm, lets schedule a review for this'. From the article: "It’s possible that a larger piece of code needs refactoring. If this is the case, do it within the merge request or schedule the refactoring as a separate issue.".
> I agree sometimes you need to review a piece of functionality. But I think of this more of an escalation of a change set review.
I find it's more like the difference between unit testing and integration testing. No matter how well you unit test it doesn't replace integration testing.
Most of the bugs I run into in prod are not the result of localised issues but are rather caused by the interplay of different modules/classes/subsystems.
I agree there is scale in this. My point is that the smaller ones (here is a bug we need to fix/code with high churn) are the best way to find the larger ones (here is code we need to refactor.
What does post production mean? After it's shipped? After it's merged to a release branch?
All of those sound like madness to me. It's not done if it's not reviewed. How are you going to release something that isn't done and expect the result to be what you want, unless what you want it to ship something that isn't done.
I can't ship stuff right all the time without code review. And I know I'm not terrible. I don't know anyone who is better. I find that defect rates are constant across skill levels and developers after a certain point. The real difference is productivity which results in more defects from more productive developers.
19 comments
[ 3.5 ms ] story [ 57.1 ms ] threadFirst of all I let every engineer review my code (and encouraged it). They found problems. They laughed. They realized I wasn't embarrassed by having to fix my code... and they realized it was ok for them to do the same with theirs. As for managers, if you review code early in the development process there just isn't that much to review. I wouldn't go line by line and check every possible thing. I generally am looking for code smells. If tests are there and CI runs then my biggest concerns can be found relatively quickly, commented upon, and then I can move on and so can the engineers.
6 months later, I have had a number of engineers say that would never want to have it any other way again. Frankly, that's how it started with me too. I really enjoy code reviews.
The parallels to software engineering are many and obvious, and I encourgE you to read it (candour, search for quality etc).
But the big one that struck me and I am looking for a way to try it is in "the brain trust". A group of 20 or so directors and producers gather every three months to watch the latest version of a film (I guess they all meet more than three months as they make more than one film at a time but whatever). The goal is to give feedback on what does and does not work as story arcs and emotional content and so forth. Not to fix it, but to raise the issues, and not to proscribe solutions but to point out blind spots.
Daily code reviews are great, but stepping back every few months with comfortable peers and just looking around a system is something that I would love - it seems to only happen after six or more months when things are starting to break.
The system I use at work is:
code -> push to gerrit -> gerrit automatically runs all unit tests and if they pass you get a +1 from gerrit -> get a code review + approval from a colleague -> submit.
So to get a piece of code to production you need both all unit tests to pass AND a code review. I am a big fan of this system and gerrit
I've worked for places that didn't have code review, and places with pre-production code review. The ones that didn't code review brought in code review practices (like they should). I have never seen a post-production review process though.
It is a terrible system. If you aren't reviewing code before it goes in you are missing a HUGE opportunity to review for bugs and correctness before a client experiences a crash.
But post-prod? Why even bother at that point? Might as well put off writing tests and so forth, too! Go BIG!
I'd rather continuously-deliver sub 1hr of changes to production several times a day. Pair & Mob programming provide continuous code review pre-commit, but they don't provide all the benefits of traditional code reviews. It can be valuable to sit down as a team and or with others who didn't work on some code and think through how to make it better/improve it asynchronously.
That makes no sense.
If delays introduced by pre-prod code review result in "bigger changesets", you're clearly doing it wrong. In fact, if you batch up commits to deliver to prod on some semi-regular basis, the exact opposite should happen.
Individual changesets would be exactly the same. Velocity may slow down (that is, it may take longer for any given changeset to make it to production), but you're producing higher quality code in exchange.
most of the time it doesn't catch anything, but every now and then it does, so that's cool.
it also means we can revert features easily most of the time, should the need arise.
all in all, gitlab is really good and you should check it out
1) send out code review 2) commit the change and say "I will address the comments in the future."
A follow up never ever happens.
This depends on what you are looking for. For style and "obvious" bugs? Agreed.
For finding bugs? Couldn't be more wrong. Functionality based reviews pick up far more defects, if done properly, than reviews of change sets. And it's because you pull back and think about the entire context of the feature.
Of course they also require a much more significant time investment and need to be run differently than a standard code review, but they are also much more valuable.
I find it's more like the difference between unit testing and integration testing. No matter how well you unit test it doesn't replace integration testing.
Most of the bugs I run into in prod are not the result of localised issues but are rather caused by the interplay of different modules/classes/subsystems.
All of those sound like madness to me. It's not done if it's not reviewed. How are you going to release something that isn't done and expect the result to be what you want, unless what you want it to ship something that isn't done.
I can't ship stuff right all the time without code review. And I know I'm not terrible. I don't know anyone who is better. I find that defect rates are constant across skill levels and developers after a certain point. The real difference is productivity which results in more defects from more productive developers.