6 ms·

I don't like code reviews, but it has a few benefits: Pros: 1. Developers pay more attention to what they throw over the wall as a completed tasks because the

I don't like code reviews, but it has a few benefits: Pros: 1. Developers pay more attention to what they throw over the wall as a completed tasks because they will get grilled on it. 2. Reviewing other people's code is good for learning a new system. 3. It keeps creating of methods that already exists to a minimum and boosts proper code reuse (of existing, difficult to find code). Cons: 1. It takes a lot of time. 2. Petty arguments over style rather than technique. It can devolve into minutia / trivia. 3. Once a developer "gets the idea" of how the organization wants coding done, it becomes more of a seemingly pointless routine. I suggest code reviews for a developers first 3-6 months, or when working with new (to them) modules / subsystems. After that, skip it (unless they are slow to conform).

gburt 9y ago
Con 2 has historically been the biggest problem for me. If any experienced people have good methods to help me mitigate that problem on my teams, I want to hear it. We've done a few strategies at places I've managed code review: 1. Insist on linting (in Javascript, eslint-config-airbnb) and opinionated style guides/automation (in JS, Prettier), and have the team buy in on "let's just not even debate this and just do what it says." 2. Have programmers highlight problem areas (structural/architectural/clarity) in their own code before submitting the PR. This is draws the discussion to the high ROI locations quickly, while not prohibiting the reviewers from catching more minor issues. 3. Clearly indicate that the goal of code review is to, yes, impart style and feel on the rest of the team, but largely to avoid duplication, resolve bugs and improve everyone's skill. It is a within-team visibility and knowledge sharing process that also happens to catch meaningful defects.
- szucs 9y ago
  I found that the best coded reviews are always from small change sets. They tend to have more comments on what the code is doing rather than what the code looks like. Sometimes this can’t be done though. I’d also say that if your team is commenting only on style in code reviews then they don’t understand that part of the code base very well. It isn’t a bad thing but presents a signal that they could spend some of their work day understanding that part of the code. Keep in mind the culture of the team needs to allow this to happen. It pays off in the long term. One of the best reasons for code review, hammered into me by two very experienced developers, is high visibility of changes and the opportunity to learn more about what other developers are working on. Don’t underestimate this benefit. Ultimately every code review doesn’t need to have a high net benefit for it to be good for your team. On a side note we didn’t have any style guide at my old workplace but found that our code reviews were incredibly high quality for core components. Part of this is culture and the other part is the people you work with.
  - gburt 9y ago
    With some reflection, I think you're correct on the understanding problem. The problem has always been inconsistent - some code reviews are worth thousands of times more than others - and a determining factor may be how invested the particular reviewer is in that piece of code, how well they understand the surrounding ecosystem and things of that nature. As a concrete example, it has always been rare that a backend API change PR doesn't get great feedback from the frontend team that is dependent on it. The visibility is a core benefit here and the clear process to collaborate (especially if done early, low cost with individual small changes) is essential to the return on time investment. For the record, I agree, I think a strong culture of code review is the most valuable process I've added to any development team. In my grandparent post, I was merely drawing attention to a potential spot for improvement, I hope no one took it as against code review as a process.
    - szucs 9y ago
      That’s very true. I’ve seen code review when it’s a bit more relaxed and most of the reviews are comments about style - only a handful of people provide useful comments about what the code does. In your defence, it’s hard to find out a way to fix a bad code review process with a team. I’m unsure how to really tackle that.
- majormajor 9y ago
  My main strategy is what you call (3). I've found that if I take a strong stance on code style - namely, "I don't care about this style matter, I'm going to approve your reviews regardless of it, and will call people out for blocking code on purely style matters" - then I haven't gotten much pushback, and the teams naturally loosely find a happy medium shared style that includes automated linting type stuff so that there's an objective criteria everyone's on board with but little personal opinion-based quibbling. I like code reviews to end up talking about "does this functionality belong in this file/block/class/whatever?" because that's what I find turns into tech debt if the whole team isn't on board with the strategy, and so I don't think anyone ever outgrows peer review on that. Those are critically important decisions, and lord knows I've inadvertently gotten it wrong as often as anyone else I've worked with. It's not that any particular dev needs oversight, it's just a really hard problem as shown by the fact that pretty much every codebase at every company turns into a big mess over time. It's possible one bad actor could still mess things up for a team with that, but then that's a managerial problem. If someone's causing the whole team to slow down, that's a problem that's fairly easy to communicate to them (and if they aren't listening, it's easy to communicate to the rest of the team in terms of "this is why we had to do something, you don't need to be worried yourself").
- kazagistar 9y ago
  The automatic formatter, extensive style guide, and linter helps. We also have a (team local) culture where it's ok to make (logically justified) style suggestions, but they aren't ever blockers and can be safely ignored if you disagree with them or are just in a hurry or whatever else. Basically, if you don't see any logical flaws or duplicated code or whatnot, you hit accept, but it's fine to make further comments.
  - gburt 9y ago
    Another thing to add, is that for "obvious" style suggestions (i.e., changes that everyone is nearly certain to agree on, though they might not agree on the importance), I encourage the reviewer to simply make the change. This is only for stuff like missing whitespace or a typo in a comment. It is not worth the roundtrip to call that stuff out and I've never seen it cause a meaningful conflict on my teams to simply fix it. I've only directly managed teams <10 local people or <5 remote people, I can imagine more structure is necessary on considerably larger or more disconnected teams. While not a problem I've ever really handled, this approach may have the side-effect of reducing dysfunctional codebase ownership that I've heard about from other team leads.
alangpierce 9y ago
Out of curiosity, what's the context in which you think developers should stop doing code reviews after 3-6 months? Every place I've worked has required code reviews for all changes (except in emergencies or when the author thinks the change is trivial). For example, how big of a company are you talking about, and how critical is correctness and maintainability? My impression is that skipping code reviews mostly makes sense for early startups, internal tools, prototypes, and other situations where velocity is more important than quality, but that most companies should have pretty much all code go through review.
- valuearb 9y ago
  I can’t imagine any reason to skip code reviews in any professional development. Someone is paying the team to produce professional code, finding out well after the fact that one section/contributed failed that test is unacceptable. Code reviews shouldn’t be lots of overhead. They shouldn’t devolve into style critiques. They should be the best tool you have for teaching and communicating best practices. Reviewing a more senior devs code can teach a junior a lot, and occasionally catch significant errors.
  - sharkdesk 9y ago
    Actually, someone is paying the team to solve business problems and create revenue.
    - Clubber 9y ago
      Preferably in the most efficient way possible (which is of course subjective). I believe that after 3-6 months in a particular subsystem, the ROI of code reviews increasingly decreases over time.
      - Anticapitalist 9y ago
        But shouldn't the time required to review also drop significantly when the reviewer is familiar with the module?
jacquesm 9y ago
1) but less time than looking for bugs from production symptoms 2) So lay down the law: no bitching over style, add a style verification step to your CI 3) you are still stuck on style. I suggest you never skip code review, stick to functionality and make sure everybody's stuff (including the gurus) gets reviewed.
kazagistar 9y ago
One of the biggest pros is that reviewing code keeps you up to date on the current state of the code base. I have, at least briefly, read and understood every line of code in our teams code base. I might not remember it perfectly, but I still feel like it is fairly easy to dive into and change any part of it, even the parts I haven't worked on in a while. This, plus occasional informal architecture standups, means we have a lot of flexibility in terms of developer allocation for tickets.
u801e 9y ago
> Cons: > 2. Petty arguments over style rather than technique. It can devolve into minutia / trivia. This can be mostly addressed by using a linter. In our organization, we have a rule that if there is a glaring style issue, we ask whether the submitter had actually run the code through the linter before submitting it for review. On the other hand, if the linter does not complain about some code, but the style it's written in is not agreed upon, we update the linter configuration to handle that case if the rest of the team agrees. Otherwise we leave the code as is (assuming it works as its supposed to). > 3. Once a developer "gets the idea" of how the organization wants coding done, it becomes more of a seemingly pointless routine. I find that there's always room for improvement in terms of how things are done and those things could be done in a code review. The other thing is that its always a good thing to have at least one other person look over a change just to make sure that its correct and something obvious wasn't missed. Edit: s/submitted/submitter/
- Clubber 9y ago
  >I find that there's always room for improvement in terms of how things are done and those things could be done in a code review. I agree that there is room for improvement, but after 3-6 months, it starts violating the 80/20 rule, meaning you are spending a hell of a lot of time & energy (money for business guys) for increasingly depreciating value. >The other thing is that its always a good thing to have at least one other person look over a change just to make sure that its correct and something obvious wasn't missed. Eh, developer should be reviewing their own code fairly thoroughly anyway. If a developer can self review their code, then the second person doing it costs a lot and doesn't get a lot of value. I wouldn't expect glaring mistakes from a developer that has lasted 3-6 months. (it's a requirement for employment, IMO). Again, what you are saying is right, but I believe you get decreasing values over time as the developer starts to "get" how the org wants things done. Like I said, I think code reviews have great value the first 3-6 months, then ever decreasing value after that. I'm also assuming competent developers. If an org is hiring cheap coders, they don't really value development efficiency in the first place and my theory doesn't really apply.
  - u801e 9y ago
joshuamorton 9y ago
In terms of the cons, I find that if the output code is good and the team as good practices regarding style (a linter and styleguide), reviews for unobjectionable code are a breeze, and often take minutes for reasonably sized changes that have no issues. A style guide and linter also remove the arguments about minutia. Google's python style guide is a lot less strict than the go or java styles, for example, and even still most of the style comments I make when reviewing code amount to "be internally consistent" about something, and those are nits. And that whole "reviews take 5 minutes" doesn't mean that they aren't useful. I often find something that is either confusing, or a typo, or dumb mistake/overlooked thing or a non-idiomatic way of doing something, and leave a comment expressing that. The review still takes 5 minutes, but I've caught a bug or a bit of technical debt later. As a code-writer, I have more or less the same opinion. I would flatly refuse to work somewhere that doesn't do code review. It makes working in the codebase difficult, and makes personal development more difficult.
- Clubber 9y ago
  Our code reviews are line-by-line and can take several hours because the reviewers feel they must completely understand everything that is going on to properly review changes (which I would agree with). I just don't see how you can do a useful code review in 5 minutes, unless it's just for posterity's sake, in which case, why bother? Can you explain your code review process? Maybe we're just doing it wrong, or maybe we're just talking about different types of changes. We typically do it at the final merge, after developer and unit testing, before QA, and most of my code is new systems, new subsystems or substantial features.
  - joshuamorton 9y ago
    I work at Google. I do code review in two contexts: one within my team, and I also volunteer to review python code for teams across google. The code review I do is subtly different in those two contexts. Within my team, my goals are to keep us to a very high standard of committed code, it should be clear, simple, and well documented. We want our output to be relatively reusable, well tested and testable, and well designed. This goes beyond our newly written code, we are stewards of libraries and tools, and we should maintain them as well as we can. On the other hand when reviewing google-wide code, I am, yes, looking for bugs, but mostly looking for good "mastery" of python. That is, I'm explicitly looking for idiomatic python from people who are not as experienced with it. These reviews also are often larger and longer. Within my team, we have a number of practices that help make code reviews fast: - Most changes are small: within my team I reviewed 3 changes of more than 250 lines this quarter, compared to a dozen or so of <10 lines - Most of the code written is really good. I rarely, if ever, have to mention lint errors, nor do I often come across bugs. - My comments mostly revolve around idiomatic ways of solving problems and, as a team python expert, "pythonic" approaches, and better approaches using the python standard library and known tooling (ie. "You're reinventing the wheel here, just use XYZ", or "this is confusing, use pathlib.path instead.") - The other big thing I comment about is clarity and readability, as a reviewer, I want to be able to follow your code, code is written more than it is read, and so most of my comments are things that amount to "This is difficult to follow as someone reading, can you refactor this to make it easier to read." These comments vary in both necessity and type, they can range from "hey maybe reorder your conditionals here and see if it reads better" to "holy wow it took me 10 minutes with a repl open and asking for help to figure out what the heck this function is doing, this needs to be refactored." - And the last thing I suggest is future work to be done: "We should refactor this method in the future, we're having to work around it a lot", or a very common one: "We can parameterize these tests". - The other big thing we do to make code reviews quick is that everyone does them. They don't just fall on the TL or manager or whatever. I review code for everyone, and everyone reviews my code, and as a result the code is not mine but the team's. Nitpicks aren't really a thing, because if someone is picky just to be picky, they'll get the same treatment in turn (and also because that's a dick move and we don't do that). Note that for the most part, the things I'm looking for are pretty easy to notice, unidiomatic code is obvious, and "this is difficult or tricky" is also very clear. I still notice bugs and such, but they're rare, and we write code to make reviewing easy and errors obvious, and as a result have fewer of them. The across-google reviews are on average bigger than within my team, and often result in more comments, because the writers are less good at python (everyone on my team has python readability, whereas the people I'm reviewing for python do not, otherwise they wouldn't need my review). They take longer. Many of them are still relatively small, but yeah the big ones that cross 500 or 1000 lines, and are in an unfamiliar part of the codebase, those do take a significant chunk of time to review (normally ~1-2 hours), but they're rare, I get one of those every couple of weeks. In general I'd say the answer to how to make code reviews go faster is 1. Encourage small changes when possible 2. Use code review to teach best practices 3. Encourage writing code for readability and testability 4. Have everyone review code These three things imo reduce errors even before review, and make it easier and faster to review the code you write. Edit: Added a fourth thing
gregmac 9y ago
It is a fallacy that it takes a lot of time, like saying QA is worthless because it too takes a lot of time. Just like it's easier to fix a bug when QA finds it before a customer does, it's even easier to fix when a developer spots it in code. If you think you stop making coding mistakes after a few months (well, or ever), you're kidding yourself. I've never got into an argument over code style in a PR, and it seems like if this did happen I'd be looking to fix deeper problems in the team. I've had comments in open source PRs asking me to change something, but that's part of getting into a new code base. The right style is there one that's there, so unless your PR has the sole purpose of specifically changing the style, you should comply with existing style, no arguing. Anyway, there are lots of other pros: A reviewer might suggest a better way of doing something, helping others learn, and likewise will pick up new techniques from the code they're reviewing. This applies to even the most senior, experienced person in the team. If a new library is brought in, it can be checked for conflicting versions or overlap/redundancy with an existing library, and license compliance. Noticing simple things like hardcoded parameters/constants, unchecked config or input values, spots with potential unhanded errors, among dozens of other problems. Unrelated changes are usually caught. Even if it's fixing another bug, I'd argue it's better to do separately with its own issue, because otherwise it gets lost when you go to build release notes or a change log.
- Clubber 9y ago
  >It is a fallacy that it takes a lot of time, like saying QA is worthless because it too takes a lot of time. If you think it's so valuable, why not have every developer in the team review every merge? That's like 15 code reviews for each piece of code checked in. Obviously, it's because at some point you get diminishing returns. We have developer testing, unit testing, QA and UAT, all to catch defects before production. That's 4 layers of testing. Code reviews make it 5 layers. Not only is it a fifth layer, it's expensive because the most expensive people (in both wages and opportunity cost) are doing it. Reviews are just another iteration of essentially the same thing. At some point, you are spending a lot of money on little return. For me that is mandatory code review after 3-6 months.

gburt 9y ago

Con 2 has historically been the biggest problem for me. If any experienced people have good methods to help me mitigate that problem on my teams, I want to hear it. We've done a few strategies at places I've managed code review: 1. Insist on linting (in Javascript, eslint-config-airbnb) and opinionated style guides/automation (in JS, Prettier), and have the team buy in on "let's just not even debate this and just do what it says." 2. Have programmers highlight problem areas (structural/architectural/clarity) in their own code before submitting the PR. This is draws the discussion to the high ROI locations quickly, while not prohibiting the reviewers from catching more minor issues. 3. Clearly indicate that the goal of code review is to, yes, impart style and feel on the rest of the team, but largely to avoid duplication, resolve bugs and improve everyone's skill. It is a within-team visibility and knowledge sharing process that also happens to catch meaningful defects.

szucs 9y ago
I found that the best coded reviews are always from small change sets. They tend to have more comments on what the code is doing rather than what the code looks like. Sometimes this can’t be done though. I’d also say that if your team is commenting only on style in code reviews then they don’t understand that part of the code base very well. It isn’t a bad thing but presents a signal that they could spend some of their work day understanding that part of the code. Keep in mind the culture of the team needs to allow this to happen. It pays off in the long term. One of the best reasons for code review, hammered into me by two very experienced developers, is high visibility of changes and the opportunity to learn more about what other developers are working on. Don’t underestimate this benefit. Ultimately every code review doesn’t need to have a high net benefit for it to be good for your team. On a side note we didn’t have any style guide at my old workplace but found that our code reviews were incredibly high quality for core components. Part of this is culture and the other part is the people you work with.
- gburt 9y ago
  With some reflection, I think you're correct on the understanding problem. The problem has always been inconsistent - some code reviews are worth thousands of times more than others - and a determining factor may be how invested the particular reviewer is in that piece of code, how well they understand the surrounding ecosystem and things of that nature. As a concrete example, it has always been rare that a backend API change PR doesn't get great feedback from the frontend team that is dependent on it. The visibility is a core benefit here and the clear process to collaborate (especially if done early, low cost with individual small changes) is essential to the return on time investment. For the record, I agree, I think a strong culture of code review is the most valuable process I've added to any development team. In my grandparent post, I was merely drawing attention to a potential spot for improvement, I hope no one took it as against code review as a process.
  - szucs 9y ago
    That’s very true. I’ve seen code review when it’s a bit more relaxed and most of the reviews are comments about style - only a handful of people provide useful comments about what the code does. In your defence, it’s hard to find out a way to fix a bad code review process with a team. I’m unsure how to really tackle that.
majormajor 9y ago
My main strategy is what you call (3). I've found that if I take a strong stance on code style - namely, "I don't care about this style matter, I'm going to approve your reviews regardless of it, and will call people out for blocking code on purely style matters" - then I haven't gotten much pushback, and the teams naturally loosely find a happy medium shared style that includes automated linting type stuff so that there's an objective criteria everyone's on board with but little personal opinion-based quibbling. I like code reviews to end up talking about "does this functionality belong in this file/block/class/whatever?" because that's what I find turns into tech debt if the whole team isn't on board with the strategy, and so I don't think anyone ever outgrows peer review on that. Those are critically important decisions, and lord knows I've inadvertently gotten it wrong as often as anyone else I've worked with. It's not that any particular dev needs oversight, it's just a really hard problem as shown by the fact that pretty much every codebase at every company turns into a big mess over time. It's possible one bad actor could still mess things up for a team with that, but then that's a managerial problem. If someone's causing the whole team to slow down, that's a problem that's fairly easy to communicate to them (and if they aren't listening, it's easy to communicate to the rest of the team in terms of "this is why we had to do something, you don't need to be worried yourself").
kazagistar 9y ago
The automatic formatter, extensive style guide, and linter helps. We also have a (team local) culture where it's ok to make (logically justified) style suggestions, but they aren't ever blockers and can be safely ignored if you disagree with them or are just in a hurry or whatever else. Basically, if you don't see any logical flaws or duplicated code or whatnot, you hit accept, but it's fine to make further comments.
- gburt 9y ago
  Another thing to add, is that for "obvious" style suggestions (i.e., changes that everyone is nearly certain to agree on, though they might not agree on the importance), I encourage the reviewer to simply make the change. This is only for stuff like missing whitespace or a typo in a comment. It is not worth the roundtrip to call that stuff out and I've never seen it cause a meaningful conflict on my teams to simply fix it. I've only directly managed teams <10 local people or <5 remote people, I can imagine more structure is necessary on considerably larger or more disconnected teams. While not a problem I've ever really handled, this approach may have the side-effect of reducing dysfunctional codebase ownership that I've heard about from other team leads.

alangpierce 9y ago

Out of curiosity, what's the context in which you think developers should stop doing code reviews after 3-6 months? Every place I've worked has required code reviews for all changes (except in emergencies or when the author thinks the change is trivial). For example, how big of a company are you talking about, and how critical is correctness and maintainability? My impression is that skipping code reviews mostly makes sense for early startups, internal tools, prototypes, and other situations where velocity is more important than quality, but that most companies should have pretty much all code go through review.

valuearb 9y ago
I can’t imagine any reason to skip code reviews in any professional development. Someone is paying the team to produce professional code, finding out well after the fact that one section/contributed failed that test is unacceptable. Code reviews shouldn’t be lots of overhead. They shouldn’t devolve into style critiques. They should be the best tool you have for teaching and communicating best practices. Reviewing a more senior devs code can teach a junior a lot, and occasionally catch significant errors.
- sharkdesk 9y ago
  Actually, someone is paying the team to solve business problems and create revenue.
  - Clubber 9y ago
    Preferably in the most efficient way possible (which is of course subjective). I believe that after 3-6 months in a particular subsystem, the ROI of code reviews increasingly decreases over time.
    - Anticapitalist 9y ago
      But shouldn't the time required to review also drop significantly when the reviewer is familiar with the module?

jacquesm 9y ago

1) but less time than looking for bugs from production symptoms 2) So lay down the law: no bitching over style, add a style verification step to your CI 3) you are still stuck on style. I suggest you never skip code review, stick to functionality and make sure everybody's stuff (including the gurus) gets reviewed.

kazagistar 9y ago

One of the biggest pros is that reviewing code keeps you up to date on the current state of the code base. I have, at least briefly, read and understood every line of code in our teams code base. I might not remember it perfectly, but I still feel like it is fairly easy to dive into and change any part of it, even the parts I haven't worked on in a while. This, plus occasional informal architecture standups, means we have a lot of flexibility in terms of developer allocation for tickets.

u801e 9y ago

> Cons: > 2. Petty arguments over style rather than technique. It can devolve into minutia / trivia. This can be mostly addressed by using a linter. In our organization, we have a rule that if there is a glaring style issue, we ask whether the submitter had actually run the code through the linter before submitting it for review. On the other hand, if the linter does not complain about some code, but the style it's written in is not agreed upon, we update the linter configuration to handle that case if the rest of the team agrees. Otherwise we leave the code as is (assuming it works as its supposed to). > 3. Once a developer "gets the idea" of how the organization wants coding done, it becomes more of a seemingly pointless routine. I find that there's always room for improvement in terms of how things are done and those things could be done in a code review. The other thing is that its always a good thing to have at least one other person look over a change just to make sure that its correct and something obvious wasn't missed. Edit: s/submitted/submitter/

Clubber 9y ago
>I find that there's always room for improvement in terms of how things are done and those things could be done in a code review. I agree that there is room for improvement, but after 3-6 months, it starts violating the 80/20 rule, meaning you are spending a hell of a lot of time & energy (money for business guys) for increasingly depreciating value. >The other thing is that its always a good thing to have at least one other person look over a change just to make sure that its correct and something obvious wasn't missed. Eh, developer should be reviewing their own code fairly thoroughly anyway. If a developer can self review their code, then the second person doing it costs a lot and doesn't get a lot of value. I wouldn't expect glaring mistakes from a developer that has lasted 3-6 months. (it's a requirement for employment, IMO). Again, what you are saying is right, but I believe you get decreasing values over time as the developer starts to "get" how the org wants things done. Like I said, I think code reviews have great value the first 3-6 months, then ever decreasing value after that. I'm also assuming competent developers. If an org is hiring cheap coders, they don't really value development efficiency in the first place and my theory doesn't really apply.
- u801e 9y ago

joshuamorton 9y ago

In terms of the cons, I find that if the output code is good and the team as good practices regarding style (a linter and styleguide), reviews for unobjectionable code are a breeze, and often take minutes for reasonably sized changes that have no issues. A style guide and linter also remove the arguments about minutia. Google's python style guide is a lot less strict than the go or java styles, for example, and even still most of the style comments I make when reviewing code amount to "be internally consistent" about something, and those are nits. And that whole "reviews take 5 minutes" doesn't mean that they aren't useful. I often find something that is either confusing, or a typo, or dumb mistake/overlooked thing or a non-idiomatic way of doing something, and leave a comment expressing that. The review still takes 5 minutes, but I've caught a bug or a bit of technical debt later. As a code-writer, I have more or less the same opinion. I would flatly refuse to work somewhere that doesn't do code review. It makes working in the codebase difficult, and makes personal development more difficult.

Clubber 9y ago
Our code reviews are line-by-line and can take several hours because the reviewers feel they must completely understand everything that is going on to properly review changes (which I would agree with). I just don't see how you can do a useful code review in 5 minutes, unless it's just for posterity's sake, in which case, why bother? Can you explain your code review process? Maybe we're just doing it wrong, or maybe we're just talking about different types of changes. We typically do it at the final merge, after developer and unit testing, before QA, and most of my code is new systems, new subsystems or substantial features.
- joshuamorton 9y ago
  I work at Google. I do code review in two contexts: one within my team, and I also volunteer to review python code for teams across google. The code review I do is subtly different in those two contexts. Within my team, my goals are to keep us to a very high standard of committed code, it should be clear, simple, and well documented. We want our output to be relatively reusable, well tested and testable, and well designed. This goes beyond our newly written code, we are stewards of libraries and tools, and we should maintain them as well as we can. On the other hand when reviewing google-wide code, I am, yes, looking for bugs, but mostly looking for good "mastery" of python. That is, I'm explicitly looking for idiomatic python from people who are not as experienced with it. These reviews also are often larger and longer. Within my team, we have a number of practices that help make code reviews fast: - Most changes are small: within my team I reviewed 3 changes of more than 250 lines this quarter, compared to a dozen or so of <10 lines - Most of the code written is really good. I rarely, if ever, have to mention lint errors, nor do I often come across bugs. - My comments mostly revolve around idiomatic ways of solving problems and, as a team python expert, "pythonic" approaches, and better approaches using the python standard library and known tooling (ie. "You're reinventing the wheel here, just use XYZ", or "this is confusing, use pathlib.path instead.") - The other big thing I comment about is clarity and readability, as a reviewer, I want to be able to follow your code, code is written more than it is read, and so most of my comments are things that amount to "This is difficult to follow as someone reading, can you refactor this to make it easier to read." These comments vary in both necessity and type, they can range from "hey maybe reorder your conditionals here and see if it reads better" to "holy wow it took me 10 minutes with a repl open and asking for help to figure out what the heck this function is doing, this needs to be refactored." - And the last thing I suggest is future work to be done: "We should refactor this method in the future, we're having to work around it a lot", or a very common one: "We can parameterize these tests". - The other big thing we do to make code reviews quick is that everyone does them. They don't just fall on the TL or manager or whatever. I review code for everyone, and everyone reviews my code, and as a result the code is not mine but the team's. Nitpicks aren't really a thing, because if someone is picky just to be picky, they'll get the same treatment in turn (and also because that's a dick move and we don't do that). Note that for the most part, the things I'm looking for are pretty easy to notice, unidiomatic code is obvious, and "this is difficult or tricky" is also very clear. I still notice bugs and such, but they're rare, and we write code to make reviewing easy and errors obvious, and as a result have fewer of them. The across-google reviews are on average bigger than within my team, and often result in more comments, because the writers are less good at python (everyone on my team has python readability, whereas the people I'm reviewing for python do not, otherwise they wouldn't need my review). They take longer. Many of them are still relatively small, but yeah the big ones that cross 500 or 1000 lines, and are in an unfamiliar part of the codebase, those do take a significant chunk of time to review (normally ~1-2 hours), but they're rare, I get one of those every couple of weeks. In general I'd say the answer to how to make code reviews go faster is 1. Encourage small changes when possible 2. Use code review to teach best practices 3. Encourage writing code for readability and testability 4. Have everyone review code These three things imo reduce errors even before review, and make it easier and faster to review the code you write. Edit: Added a fourth thing

gregmac 9y ago

It is a fallacy that it takes a lot of time, like saying QA is worthless because it too takes a lot of time. Just like it's easier to fix a bug when QA finds it before a customer does, it's even easier to fix when a developer spots it in code. If you think you stop making coding mistakes after a few months (well, or ever), you're kidding yourself. I've never got into an argument over code style in a PR, and it seems like if this did happen I'd be looking to fix deeper problems in the team. I've had comments in open source PRs asking me to change something, but that's part of getting into a new code base. The right style is there one that's there, so unless your PR has the sole purpose of specifically changing the style, you should comply with existing style, no arguing. Anyway, there are lots of other pros: A reviewer might suggest a better way of doing something, helping others learn, and likewise will pick up new techniques from the code they're reviewing. This applies to even the most senior, experienced person in the team. If a new library is brought in, it can be checked for conflicting versions or overlap/redundancy with an existing library, and license compliance. Noticing simple things like hardcoded parameters/constants, unchecked config or input values, spots with potential unhanded errors, among dozens of other problems. Unrelated changes are usually caught. Even if it's fixing another bug, I'd argue it's better to do separately with its own issue, because otherwise it gets lost when you go to build release notes or a change log.

Clubber 9y ago
>It is a fallacy that it takes a lot of time, like saying QA is worthless because it too takes a lot of time. If you think it's so valuable, why not have every developer in the team review every merge? That's like 15 code reviews for each piece of code checked in. Obviously, it's because at some point you get diminishing returns. We have developer testing, unit testing, QA and UAT, all to catch defects before production. That's 4 layers of testing. Code reviews make it 5 layers. Not only is it a fifth layer, it's expensive because the most expensive people (in both wages and opportunity cost) are doing it. Reviews are just another iteration of essentially the same thing. At some point, you are spending a lot of money on little return. For me that is mandatory code review after 3-6 months.