Moving Fast with High Code Quality
engineering.quora.com
engineering.quora.com
>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?
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.
I'm not strictly opposed to pre-commit reviews, but I think it's incorrect to be dismissive of their cost.
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.
- def f(x: X): Option[X]
+ def f(x: X): MyErrorType \/ X
Some changes simply cannot be broken into tiny atomic pieces. Often these are important changes for long term maintainability.
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.
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.
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.
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.
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.
Yes, but context-switches are expensive, particularly when you have several reviews in progress across several projects.
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).
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.I would not move to this workflow, although it's interesting to hear from somewhere that it works.
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.
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.
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'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.
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.
At Quora, we generally do post-commit code reviews. That is, the code goes out live in production first and someone comes and reviews the code later.
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.
* cool tech
* long format
* looks good
* describe interesting problems
* end with link to careers pageIt works, of course.
But it's also much less obnoxious than previous methods, so it also generates less bad will in the community.
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.
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.
Basically whenever we notice codestyle discussion in PRs repeating a custom linter will come to establish convention (/rule).
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.
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?
This is a bad idea.
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.