

Why pre is better than post production code review - sytse
https://about.gitlab.com/2015/08/05/6-reasons-why-pre-is-better-than-post-production-code-review/

======
scrabble
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.

~~~
sheepmullet
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.

~~~
zzalpha
Post commit, sure... I still prefer pre-commit, but I get why some might do
post-commit.

But post- _prod_? Why even bother at that point? Might as well put off writing
tests and so forth, too! Go BIG!

~~~
benjiweber
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.

~~~
zzalpha
_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.

------
trcollinson
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.

~~~
lifeisstillgood
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.

------
arielweisberg
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.

~~~
sytse
I think after it is shipped on a environment with actual users. But at GitLab
we always review before merging into master.

------
rjbrock
You can make code reviews required by using something like Gerrit
([https://code.google.com/p/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

------
sheepmullet
"Small Reviews > Large Reviews"

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.

~~~
sytse
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.".

~~~
sheepmullet
> 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.

~~~
sytse
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.

------
kelukelugames
A lot of people at my company did this:

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.

------
mrbig4545
we use gitlab, it makes doing pre code review easy. when your code is ready,
send a merge request, someone else has to review it to merge it.

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

~~~
sytse
Thanks, glad to hear you like it!

------
sytse
OP here, I would love to discuss code review best practices from the crowd
here.

