8 ms·

Moving Fast with High Code Quality

198 points by kilimchoi 11y ago

subb 11y ago
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 11y ago
  I agree. However, a post like this has multiple purposes, non-the-least is a dictum about their culture and what they value.
  - mooreds 11y ago
    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 11y ago
      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 11y ago
        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
    - nstart 11y ago
      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.
- paulojreis 11y ago
  > 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.
groby_b 11y ago
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 11y ago
  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.
  - mdlowman 11y ago
    You're right, but where I work almost anything can cause a bug that affects users. Pre-review eliminates some of those bugs. I would not move to this workflow, although it's interesting to hear from somewhere that it works.
  - chipgap98 11y ago
    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.
  - groby_b 11y ago
    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.
calinet6 11y ago
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 11y ago
  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 11y ago
    Yeah, maybe the specific implementation here isn't so great.
jbrooksuk 11y ago
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.
andreasklinger 11y ago
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).
- awinder 11y ago
  How do you develop code style rules? Just team discussion, lead driven decision, etc.? Asking for a friend ;-)
- realharo 11y ago
  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.
omouse 11y ago
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?
- awinder 11y ago
  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.
  - omouse 11y ago
    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.
    - gknoy 11y ago
      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.
zobzu 11y ago
I thought thats why you can have like try/dev and master/prod branches (or repos)
aquilaFiera 11y ago
I would love to see Quora's qlint tool open sourced. Sounds like a great tool.
johnrob 11y ago
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.
sjLongjorn 11y ago
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?
curiousjorge 11y ago
how accurate is that graph?
compostor42 11y ago
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?
compostor42 11y ago
Anyway you guys could open source your QLint tool?

198 points by kilimchoi 11y ago

subb 11y ago

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 11y ago
I agree. However, a post like this has multiple purposes, non-the-least is a dictum about their culture and what they value.
- mooreds 11y ago
  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 11y ago
    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 11y ago
      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
  - nstart 11y ago
    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.
paulojreis 11y ago
> 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.

groby_b 11y ago

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 11y ago
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.
- mdlowman 11y ago
  You're right, but where I work almost anything can cause a bug that affects users. Pre-review eliminates some of those bugs. I would not move to this workflow, although it's interesting to hear from somewhere that it works.
- chipgap98 11y ago
  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.
- groby_b 11y ago
  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.

calinet6 11y ago

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 11y ago
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 11y ago
  Yeah, maybe the specific implementation here isn't so great.

jbrooksuk 11y ago

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.

andreasklinger 11y ago

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

awinder 11y ago
How do you develop code style rules? Just team discussion, lead driven decision, etc.? Asking for a friend ;-)
realharo 11y ago
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.

omouse 11y ago

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?

awinder 11y ago
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.
- omouse 11y ago
  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.
  - gknoy 11y ago
    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.

zobzu 11y ago

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

aquilaFiera 11y ago

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

johnrob 11y ago

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.

sjLongjorn 11y ago

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?

curiousjorge 11y ago

how accurate is that graph?

compostor42 11y ago

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?

compostor42 11y ago

Anyway you guys could open source your QLint tool?