>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?
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.
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.)
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.
Right, but if that's the only change made, reviewing find-and-replace is trivial. The diff might bleed a bit on the pattern matching, but as long as you restrict the commit to the single logical change, verification is still pretty easy.
The entire point of such a commit is to not be just a search and replace, but instead to have different behavior depending on the value of the error type. E.g. crash on network error, UPDATE rather than INSERT on specific SQL error.
Some changes simply cannot be broken into tiny atomic pieces. Often these are important changes for long term maintainability.
Here's my experience with this sort of change: You have exactly one usage where you NEED the update. After deciding to make it you've identified a few other places where you can use the updated code.
The Right Way (TM) to make that change, then, is to make only the change to the result type in the first commit (along with the pattern matching necessary to ignore it), followed by a commit for each place that the updated result type can be usefully used.
Change the code without changing the behavior: i.e. REFACTORING. Run the tests, verify you didn't break anything, then start writing tests for the behavior you wish to change, followed by changing that behavior.
If you think you're at atoms you probably haven't seen strings.
I suppose it could be done that way. But that seems like a lot of extra work, and you also lose the compilers help. Also, in my experience changes like this actually do involve lots of subtly broken code - think about it, anyone using fold on a network error is making a mistake (retrying the use of a closed connection).
If you fix the behavior everyplace the compiler complains, you are guaranteed to resolve most issues (ignoring overloaded methods, e.g. getOrElse). If you use a toOption method on the union type to make a tiny atomic change, the compiler will no longer complain in all the places that mistakes are being made.
I suppose an intermediate way might be to rename f to f_DEPRECATED, replace everywhere, and build the new method returning the right type. But in my experience its better just to bite the bullet and change everything.
My team added something four weeks ago that might be an idea to try - we're still trailing it ourselves. It started accidentally and became a tradition on our team though so your mileage may vary.
If the pull request is small, fixes a bug or is a blocking you, then you can spend you quick-code-review-chip for the week and ask the team to look at it at their first available opportunity. (Sometimes reviewed within 15 minutes, usually an hour tops)
It allows team members to ask the rest of the team to unblock them quickly. Limiting it to one a week prevents abuse of their team mates' time and encourages all team members to plan their work out ahead as to reduce blocking.
We also wrote a quick HipChat bot that keeps track of who's spent their quick-review-chip too.
My team handles this by committing to personal dev branches, then reviewing relatively small merges to mainline. If a review is in progress and you want to work on changes that depend on the code in review, you make another dev branch from the review commit. Then you merge any changes back in after the review is done.
Ordinarily this final merge is fairly painless, since you ideally won't have many code reviews that call for sweeping changes in your design. If you do, that's a sign of failure earlier in the process.
I completely agree that how can you get blocked waiting for a code review? Some concurrency is wonderful for avoiding being blocked.
In work I currently have 3 pull requests open all touching different sections of the product. The 3 PRs are all at different stages of the review process. For example, I'll be closing one first thing tomorrow and deploying it out (don't like deploying in the evenings - its bad luck). Another I just opened as I left work today, so I doubt anyone's reviewed it yet.
I'm sure Quora has a big enough codebase and not enough developers (like most companies) that they always have a list of things a mile long that they would like to do or should do, be that new features or just refactoring.
2-3 days isn't long for a review and iteration cycle in itself. I have one PR that is currently open for 5 working days at this stage. We decided that it didn't quite fit the bill on the first draft . It is blocking progress on two other tasks in our backlog but we also have 14 things on our backlog of goals for this week. The team is working around it while we finish it off or create issues for things we think have fallen outside the scope of the initial job.
One thing we've noticed alot on my team is that we try and make our pull requests for review as small as possible. We all have a rough guide of about max 500 lines. Sometimes we do go over but its a rough guideline. Smaller PRs are just easier to review for everyone involved. Reviewers don't have walls of code to break through or need to keep tons of context in their head.
Post-commit reviews (or post-merge into master or whatever you process is) is an interesting idea. I'd be curious to see what my team think of the idea tomorrow for a small sub set of our codebase.
I love this comment. I've heard this actually voiced by developers having trouble coping with code reviews -- "I'm blocked, I can't work on anything else". Think you're right on with the idea of using it for training / research / pet project time.
I've actually gotten into trouble because I spent more than 5 minutes doing a code review of a pull request. I had to code review for 3 junior devs and one senior; the junior devs all knew little Python so only I could review their code while the other senior dev was busy. It took hours to wade through and double-check their code. I always suggested to them to look up some Python libraries or to work on puzzles to practice or to review the code as well or talk to each other about the next piece of work. There's so many other things to do than to keep coding.
That sounds less like "code review", and more like "Mentoring". If you find a way to position it that way, you can bring that up with your boss(es) before your next performance review.
I've found mentoring other developers (whether it's explaining "Here's the facets of how we test All The Things", or "Here's how git branching works") to be profoundly fulfilling. I love explaining things. The best part is, fostering the idea of teaching each other means that others can mentor YOU as well on things, and you (and they) don't need to feel threatened.
>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?
Yes, but context-switches are expensive, particularly when you have several reviews in progress across several projects.
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).
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.
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.
While the examples given do highlight areas where a bug could do critical damage to a company, there are plenty of ways to break things that cause problems for users. If you put code through at least a lightweight review process you can filter out many of these bugs.
While post commit review might work for smaller changes I would rather err on the side of caution and use precommit review. We deploy about the same number of times/day/engineer at my company, so I don't think that the review process is what drives that metric.
There is a continuum between pre-commit and push to production. You can push to integration, push to QA, push to nightly build, and so on.
I'm highly skeptical of code reviews as the way to find bugs. I think code reviews have other values, just not that one so much. Getting multiple eyes on code and the results of that code, getting it into the hands of QA, dog fooding it, all play a role. That all sounds way more ad-hoc than I mean it. I used to write life critical software, and you have well defined ways to handling source control and reviews. I advocate figuring out what your project needs, adopting that, and then continually adapting as the project changes, as any project will.
Absolutely. I think code reviews make sense for knowledge sharing and improving the quality of the code. Most people during code reviews don't actually find bugs in their comments, according to some research done by Microsoft.
http://research.microsoft.com/pubs/180283/ICSE%202013-codere...
There is more to code reviews than just preventing issues. Code review is a valuable tool for maintaining code quality, and share code knowledge across the team. Those two things can be achieved with post-commit reviews.
There really can't be a one-size-fits-all. New devs must earn the right to commit without review (post-commit-review), devs with proven quality records get post-commit-review, sensitive code areas get pre-commit-reviews.
At Google the worst offenders for broken builds & minor bugs were often the most experienced & productive devs. They developed reputations around their particular teams, which meant their teammates would think "Oh, X wrote this, he usually knows what he's doing, so I don't have to be quite as careful with the review." As a result, a bunch of silly mistakes would make it through. I had one (very productive) coworker who would slip intentional bugs or style violations into his changes to see if the reviewer was paying attention.
Strangely, this didn't hold for major bugs, which were often done by newbies to the codebase. Major bugs are often created by not understanding the implications of a change or the other systems that might be impacted; experienced devs usually know this by heart, so they don't screw it up in their designs.
> I had one (very productive) coworker who would slip intentional bugs or style violations into his changes to see if the reviewer was paying attention.
I've heard of this process ("bebugging", the opposite of "debugging") being used for statistical QA analysis. If you purposely inject 10 bugs (don't forget to remove them! :) and QA or fuzzing finds 7 of them plus (say) 42 other bugs, then you can extrapolate that you have about 18 undiscovered bugs (10/7*42-42 = 18). This assumes your 10 injected bugs are representative of the actual bugs you find, which is harder than it sounds.
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.
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.
I like this format. It feels like watching a good advertisement instead of one that just screams "BUY!". It's great because at the end of the day, there's some education, and something to be taken away. Even if that thing is a write up you can show your boss to convince her that it's a good idea to pick up some of these practices. There's some value at the end of the day so yeyy :D.
And honestly, I'd love to work for a team like this. I'm starving for a team that'll kick my ass and let me learn what it's like to work with the best practices.
Not that what I do right now is bad. Education wise though it's fairly stagnant.
> but it would have been nice to see some real metric instead of some napkin graphs
That's roughly why software engineering isn't really engineering. We're too cool for quantification. We feel that we don't need to give solid metrics to justify investment on what we think is cool. But, boy, if management asks us something that we don't like, then they'll have to handle solid and quantifiable evidence on the utility of it.
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).
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.
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?
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.
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.
Continuous Integration is especially useful for code quality. As a massive coincidence my company released our first SaaS product today.
StyleCI (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.
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?
>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?