
Moving Fast with High Code Quality - kilimchoi
http://engineering.quora.com/Moving-Fast-With-High-Code-Quality?share=1
======
omouse
This is the part that I don't understand,

> _Even if each round of review takes 2 days and there are 2-3 such
> iterations, it is easy for the author to stay blocked for good part of a
> week leading to wasted time._

How can the author of the code be blocked while waiting for a code review?
Can't they use that time to review someone else's code or to work on something
else in a separate branch or, heaven forbid, _learn_ or _train_ themselves?

I mean we never seem to find time to train developers but the time between
writing code and having it pass review could be used for education not just
sitting around and waiting.

> _For code reviews to work well, each change should be reviewed by the people
> who have enough context on that change. It 's even better if these code
> reviewers are responsible for “maintaining” the code they are reviewing so
> that they are incentivized to make the right long-term trade-offs_

This makes sense at first but don't you also want _more_ people looking at the
code who don't have complete context so that the context must be explained to
them and then they can learn more about the code base? Isn't reducing the bus
factor also a priority?

~~~
chadaustin
The problem is not that the time waiting on a code review can't be used in
some other way. It's that the constant interruptions seriously hurt straight-
line productivity. "OK, done with this minor patch. Now, uhhh, while I wait
for code review, I guess I'll go review this other thing... And then start a
new patch.... Code review is done now? Better context-switch back."

I'm not strictly opposed to pre-commit reviews, but I think it's incorrect to
be dismissive of their cost.

~~~
mkozlows
I agree with this to some extent, but it can be ameliorated a lot with smaller
and more frequent PRs. If your PR is enormous and represents a week's worth of
work and you end up having to respond to 20 comments, it's going to be awful.
If your PR is small and represents hours' worth of work and you have to
respond to 1-2 comments, it's a lot less obnoxious. (And you'll have more
natural opportunities for context-switching, so responding to comments won't
be so distracting.)

~~~
deepGem
I can't agree more to this approach. Smaller changes are easy to review and
manaage. We developers need a push to make this happen though.

~~~
MaulingMonkey
This also needs proper VCS support - by which I mean DVCSes that let you pile
commits up to the same files one after another sanely, or reviews after commit
rather than before (better have a good branching setup!)

My last job was perforce (no unsubmitted commit stacking) with everyone
developing directly against a single branch. In theory, code reviews were
supposed to happen before hand.

In practice, I submitted small changes from the hip, and complained when my
coworkers gave me a week long spitball of unrelated features in a single mega-
commit. Not that I wasn't guilty of that myself. Bleh.

------
groby_b
I think post-commit reviews are a bad practice. The point of code review is to
prevent issues in the first place. If you have a privacy issue and discover
that two days _after_ you pushed the code to prod, that's not much fun.

And if each part of the review takes 2 days in the first place, there might be
an issue with your code reviews (or your checkins are gargantuan).

~~~
relaunched
They specifically address areas where the default to pre-commit review, for
the reason you just called out:

We switch to pre-commit reviews instead of the usual post-commit review flow
if the potential errors/mistakes are costly and can cause significant
problems. Some examples of such cases are:

    
    
        Touching code which deals with privacy and anonymity of users.
        Touching a core abstraction since a lot of other code might depend on it.
        Touching some parts of the infrastructure where mistakes can cause a downtime.

~~~
groby_b
That makes the assumption you can correctly all identify all those places. And
maybe I'm too chicken, and you actually can - but I'd personally still prefer
pre-commit. I'm a belt-and-suspenders gal :)

But I'm really curious - if you can talk about it, that is - why devs push on
average almost 4 changes per day & dev, but a review takes 2 days roundtrip.

------
johnrob
The best case for velocity over time is a line that slowly declines to the
right. Even with the cleanest code, as a product adds features it takes longer
and longer to properly add the next one. Part of this is simply the multitude
of cases to consider (because of all those features!), but another part is
simply the number of lines of code.

The _only_ way around this, IMO, is if you split the product into parts that
have no dependancies on the others (the notion of a 'mono-repo' undermines
this). This works because you trade one big product for many small ones, and
thus avoid having to solve all the issues that come with big products. Of
course, this is much easier said than done.

------
subb
This is a great application of generally accepted software engineering
principles (and some less common like "cleanup week"), but it would have been
nice to see some real metric instead of some napkin graphs.

~~~
relaunched
I agree. However, a post like this has multiple purposes, non-the-least is a
dictum about their culture and what they value.

~~~
mooreds
Yes. This was a 'please come join us' post. I've been seeing more and more of
these lately. Formula:

    
    
       * cool tech
       * long format
       * looks good
       * describe interesting problems
       * end with link to careers page

~~~
angelbob
Expect to continue seeing more of it.

It works, of course.

But it's also much less obnoxious than previous methods, so it also generates
less bad will in the community.

~~~
mooreds
Yes. I think there might be a business in there somewhere, as I wrote on my
blog a year ago:
[http://www.mooreds.com/wordpress/archives/1502](http://www.mooreds.com/wordpress/archives/1502)

------
andreasklinger
I highly agree on the linter part - it's too underestimated - in our team we
currently have linters in place for every language and add custom linting
rules on regular basis.

Basically whenever we notice codestyle discussion in PRs repeating a custom
linter will come to establish convention (/rule).

~~~
realharo
Some of their uses (like the private property convention the article mentions)
just seem to be making up for the language itself not being strict/expressive
enough.

Once you already have a large existing codebase, adding some of this stuff via
static analysis later is a nice and cheap way to get those extra checks
anyway.

------
compostor42
This is all great stuff. It pains me how difficult it has been to get my org
to resemble anything remotely close to this. There is simply too much
resistance to change here. Being a more junior employee myself has made it
even more difficult to enact change.

I was on a project for 5 months late last year/early this year where we ran
things like this and it was a dream. Don't think I have ever loved my job so
much. Unfortunately that project was scrapped. Been on the lookout for a new
employer where these practices are the norm though it is hard to suss out the
fakers who claim they do these things from the real deal.

Are the practices/culture outlined in this article typical of many Silicon
Valley shops? Or is it just as rare there as it seems to be elsewhere?

------
calinet6
Solid basic idea: systems enable you to go fast with high quality. It's not
always a tradeoff, folks, and the long-term benefit of quality is _more
speed._ It's a clear winner.

~~~
jonathankoren
It doesn't enforce quality at all. All it does is file a cleanup task at most,
while the jankey thing that never should have been committed stays in
production.

This is a bad idea.

~~~
calinet6
Yeah, maybe the specific implementation here isn't so great.

------
aquilaFiera
I would love to see Quora's qlint tool open sourced. Sounds like a great tool.

------
jbrooksuk
Continuous Integration is especially useful for code quality. As a massive
coincidence my company released our first SaaS product today.

StyleCI ([https://styleci.io](https://styleci.io)) is a PHP coding style
service. Code style to us is as much code quality is to others. A good code
standard can help some issues in the first place.

------
sjLongjorn
I would really like to hear more about the testing policies. Who code reviews
the tests, which presumably must be in place before the code to be tested gets
deployed? And what metrics does Quora's CI system use to determine there's an
issue before rolling back a release?

------
zobzu
I thought thats why you can have like try/dev and master/prod branches (or
repos)

------
compostor42
Anyway you guys could open source your QLint tool?

------
curiousjorge
how accurate is that graph?

