9 ms·

A bogus study on code review

65 points by luu 9y ago

coding123 9y ago
93% of statistics are made up on the spot. In all seriousness I have found it's not the number of reviews or that they are done consistently. I have found that the most helpful reviews are all the ones where really good code is reviewed by lots of people that has all of the things that organization considers a good practice. 1 Review like that does a lot more good than 25 reviews by lesser skilled coders.
Morendil 9y ago
This needs some signal-boosting! I would trust someone with a sharp eye like that to review my code much more than I would trust the author of the original article. The PDF that requires an email can also be found here without giving one: https://pdfs.semanticscholar.org/feb0/0dd349ad701e1e4045282b1bd10073718a7c.pdf https://pdfs.semanticscholar.org/feb0/0dd349ad701e1e4045282b...
prepend 9y ago
It’s also really important to define defect during these reviews. I worked at a company where code reviews would identify lots of superficial errors like indention, improper documentation, and missing metadata. You want these in quality code based, but if this counts as a defect, it’s not as important as serious bugs being identified and corrected.
- maxxxxx 9y ago
  A real code review takes a lot of time. With the time available typically you can only find superficial things like style.
  - gburt 9y ago
    Have you ever actually been constrained in how much time you can spend on code review? I generally try to get my developers to spend more time on it than they think is necessary -- if it comes at the short-term pains of their productivity, I am totally ok with that and will work to revise their individual contributor expectations.
    - maxxxxx 9y ago
      In most companies I have seen code reviews were scheduled in addition to the regular workload. This means you have to do find extra time to do them. There are also only a few people who can give a meaningful review of complex code. I am now a believer in pair programming. Code review is already done during development time.
      - valuearb 9y ago
        We just do code reviews on every commit, it’s just part of our flow. My experience is it’s far more productive than paired programming, our team doesn’t care about style issues and most code is clearly correct. So review of 4 hours of work can usually be done in less than 20 mins. As far as cutting in the schedule, we get done what we get done. The schedulers can bite me, i wasn’t the one who mislead them to think dev estimates had any accuracy or usefulness.
      - gburt 9y ago
        My teams also assign code review "in addition" to the regular workload, but this is always in the context of an agile team effort: we get done what we get done [1]. A developer takes a story and works on it until it is solved (or splits it appropriately), then another developer (or a few) are expected to do review. -- [1] I might be measuring productivity with story points in the background, but there is no "scheduled workload," this is merely an averaging/planning exercise. The (correct, IMO) managerial effort to improve individual productivity is much softer and more understanding than that and comes with the understanding that your output takes many forms. Some of my best developers directly complete almost zero stories in a sprint because their time is spent on code review, pair programming, direct assistance and architecture discussion.
  - alangpierce 9y ago
    It depends a bit on context, I suppose, but if you spend most of your time/brainpower in code reviews on style issues, then I think you're probably not making a very good use of time. I think it's much more important to look at the problem being solved and the solution and to think through whether it makes sense. Often times I prefer to first have a conversation with the submitter where we talk through the structure and the decisions made, then I'll go through and leave inline comments for things like style as I see them. I would put much more trust in a codebase built with careful engineering solutions than a codebase with careful formatting, and I think I would find it to be more pleasant to work with as well. Ideally you have both, of course.
    - maxxxxx 9y ago
      I totally agree with you. However, in my experience the time you get for reviews is often not much and the developer already has moved on to something else and doesn't want to deal with the review. Obviously this is an organizational problem but one I have seen a lot.
      - valuearb 9y ago
        On my team all code has to be reviewed before it goes to QA. We strive to do it same day, to make it more productive for reviewers and writers of the code. It’s not too much overhead. Even though most reviews are satisfactory, occasionally it catches something that’s a good learning opportunity, and that’s golden.
  - valuearb 9y ago
    Not sure why a review would take so much time, mine typically take 20 minutes and I do two a day. We ignore formatting (which can be formalized with a linter should we choose). Basically i check every line for reasonable error handling, review the whole for reasonable algorithm/data structures and i’m done. What am i missing?
    - maxxxxx 9y ago
      It probably depends on the work you are doing. We do test harnesses for hardware and most reviews are the result of a lot of experimentation and learning about protocols. In order to understand that code you need to know the characteristics of the device you are interfacing with. This may take weeks for a reviewer.
- alangpierce 9y ago
  If you search "Counting Defects" in the pdf posted by Morendil ( https://pdfs.semanticscholar.org/feb0/0dd349ad701e1e4045282b1bd10073718a7c.pdf https://pdfs.semanticscholar.org/feb0/0dd349ad701e1e4045282b... ), you get some of the methodology on page 78 (the page with "78" written on it, not PDF numbering): > When a reviewer or consensus of reviewers determines that code must be changed before it is acceptable, it is a “defect.” If the algorithm is wrong, it’s a defect. If the code is right but unintelligible due to poor documentation, it’s a defect. If the code is right but there’s a better way to do it, it’s a defect. A simple conversation is not a defect nor is a conversation where a reviewer believed he found a defect but later agreed that it wasn’t one. In any event a defect is an improvement to the code that would not have occurred without review. I'd say that's a bit looser of a definition than I'd prefer, but it's at least good that the study didn't try to use some metric like "number of inline comments", since often times I use inline comments for praise or to mention possible alternatives that may or may not be any better than the code.
Clubber 9y ago
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
I've read this and similar posts on this study a dozen times in the last few weeks. I think the data is poorly interpreted and what you're really seeing is that shorter pull requests elicit better feedback (in this case "more defects per line"). In my experience running code reviews, shorter pull requests, presumably due to their reduced effort necessary to understand, tend to get better review, review that is more than just superficial style/linting errors. In light of that, I always encourage developers to aim for the shortest reasonable changeset - and make it very clear to them that 2 line PRs are totally acceptable - on my current team, we go as far as to encourage tricks like rewriting (local) Git history and cherrypicking to ensure that is the case. Continuous integration and good unit tests help to ensure that strange states generated from that process are still good and I find the costs are far outweighed by the better reviews. Another little lesson I've learned managing that process is to encourage the submitting developer to highlight problem areas in his own code: when I submit my own code for review, I'll actually do the review first, line-level highlighting areas I wish the reviewer to pay attention to. This will only work if you can trust your developers to not try to "sneak something by," but if you can't do that, you've probably already lost. Developers generally know what they weren't sure about during the process, where things are going to be difficult to understand and where someone else on the team is going to have helpful contributions to the quality. I think it is important for developer-managers to remember that programming is largely a craft and developers largely want to be proud of their output. People _like_ producing "good code" and it is easy to align the goals by having the process help improve their craft.
- majormajor 9y ago
  This matches my experience, and also has implications for date-driven projects. People have two typical responses to long code reviews: skim and ignore, or go in depth and ask a lot of questions because there's a lot going on that's new. If it's a critical feature, I try to push the team to not just ignore, so the longer someone works in a silo, the longer it takes to merge those changes back in with everything else, the higher the risk of getting major "why not do it this other way instead" late feedback throwing a wrench in everything last minute. What I've taken away from recognizing this pattern is to ensure everyone's checking in frequently, even in the early design and idea phase, so that nothing ever turns into a bad surprise in a code review. Depending on the seniority of the dev driving the feature, there isn't a single one-size-fits-all prescription for what's a "too big" code review, but the rest of the team's job is to make sure that whoever is driving the feature never ends up in a position where their technical choices put the delivery at significant risk - nobody wants to feel like their team left them to sink or swim and then only offered criticism when they finally submitted their code for review. I'm also a huge fan of the described "identify what I dislike about my own code when submitting it for review" practice, too. I'll come straight out and ask "I don't like how I did this, anyone got better ideas?" and I've seen other people start picking this up on the team too.
hacker314159 9y ago
A lot of the comments here imply that style can be fixed with a linter, but what about the style of things other than syntax, like choices in variable names, DRY-ing through mixins vs composition, grammatical style in code comments, etc.

65 points by luu 9y ago

93% of statistics are made up on the spot. In all seriousness I have found it's not the number of reviews or that they are done consistently. I have found that the most helpful reviews are all the ones where really good code is reviewed by lots of people that has all of the things that organization considers a good practice. 1 Review like that does a lot more good than 25 reviews by lesser skilled coders.

Morendil 9y ago

This needs some signal-boosting! I would trust someone with a sharp eye like that to review my code much more than I would trust the author of the original article. The PDF that requires an email can also be found here without giving one: https://pdfs.semanticscholar.org/feb0/0dd349ad701e1e4045282b1bd10073718a7c.pdf https://pdfs.semanticscholar.org/feb0/0dd349ad701e1e4045282b...

prepend 9y ago

It’s also really important to define defect during these reviews. I worked at a company where code reviews would identify lots of superficial errors like indention, improper documentation, and missing metadata. You want these in quality code based, but if this counts as a defect, it’s not as important as serious bugs being identified and corrected.

maxxxxx 9y ago
A real code review takes a lot of time. With the time available typically you can only find superficial things like style.
- gburt 9y ago
  Have you ever actually been constrained in how much time you can spend on code review? I generally try to get my developers to spend more time on it than they think is necessary -- if it comes at the short-term pains of their productivity, I am totally ok with that and will work to revise their individual contributor expectations.
  - maxxxxx 9y ago
    In most companies I have seen code reviews were scheduled in addition to the regular workload. This means you have to do find extra time to do them. There are also only a few people who can give a meaningful review of complex code. I am now a believer in pair programming. Code review is already done during development time.
    - valuearb 9y ago
      We just do code reviews on every commit, it’s just part of our flow. My experience is it’s far more productive than paired programming, our team doesn’t care about style issues and most code is clearly correct. So review of 4 hours of work can usually be done in less than 20 mins. As far as cutting in the schedule, we get done what we get done. The schedulers can bite me, i wasn’t the one who mislead them to think dev estimates had any accuracy or usefulness.
    - gburt 9y ago
      My teams also assign code review "in addition" to the regular workload, but this is always in the context of an agile team effort: we get done what we get done [1]. A developer takes a story and works on it until it is solved (or splits it appropriately), then another developer (or a few) are expected to do review. -- [1] I might be measuring productivity with story points in the background, but there is no "scheduled workload," this is merely an averaging/planning exercise. The (correct, IMO) managerial effort to improve individual productivity is much softer and more understanding than that and comes with the understanding that your output takes many forms. Some of my best developers directly complete almost zero stories in a sprint because their time is spent on code review, pair programming, direct assistance and architecture discussion.
- alangpierce 9y ago
  It depends a bit on context, I suppose, but if you spend most of your time/brainpower in code reviews on style issues, then I think you're probably not making a very good use of time. I think it's much more important to look at the problem being solved and the solution and to think through whether it makes sense. Often times I prefer to first have a conversation with the submitter where we talk through the structure and the decisions made, then I'll go through and leave inline comments for things like style as I see them. I would put much more trust in a codebase built with careful engineering solutions than a codebase with careful formatting, and I think I would find it to be more pleasant to work with as well. Ideally you have both, of course.
  - maxxxxx 9y ago
    I totally agree with you. However, in my experience the time you get for reviews is often not much and the developer already has moved on to something else and doesn't want to deal with the review. Obviously this is an organizational problem but one I have seen a lot.
    - valuearb 9y ago
      On my team all code has to be reviewed before it goes to QA. We strive to do it same day, to make it more productive for reviewers and writers of the code. It’s not too much overhead. Even though most reviews are satisfactory, occasionally it catches something that’s a good learning opportunity, and that’s golden.
- valuearb 9y ago
  Not sure why a review would take so much time, mine typically take 20 minutes and I do two a day. We ignore formatting (which can be formalized with a linter should we choose). Basically i check every line for reasonable error handling, review the whole for reasonable algorithm/data structures and i’m done. What am i missing?
  - maxxxxx 9y ago
    It probably depends on the work you are doing. We do test harnesses for hardware and most reviews are the result of a lot of experimentation and learning about protocols. In order to understand that code you need to know the characteristics of the device you are interfacing with. This may take weeks for a reviewer.
alangpierce 9y ago
If you search "Counting Defects" in the pdf posted by Morendil ( https://pdfs.semanticscholar.org/feb0/0dd349ad701e1e4045282b1bd10073718a7c.pdf https://pdfs.semanticscholar.org/feb0/0dd349ad701e1e4045282b... ), you get some of the methodology on page 78 (the page with "78" written on it, not PDF numbering): > When a reviewer or consensus of reviewers determines that code must be changed before it is acceptable, it is a “defect.” If the algorithm is wrong, it’s a defect. If the code is right but unintelligible due to poor documentation, it’s a defect. If the code is right but there’s a better way to do it, it’s a defect. A simple conversation is not a defect nor is a conversation where a reviewer believed he found a defect but later agreed that it wasn’t one. In any event a defect is an improvement to the code that would not have occurred without review. I'd say that's a bit looser of a definition than I'd prefer, but it's at least good that the study didn't try to use some metric like "number of inline comments", since often times I use inline comments for praise or to mention possible alternatives that may or may not be any better than the code.

Clubber 9y ago

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

I've read this and similar posts on this study a dozen times in the last few weeks. I think the data is poorly interpreted and what you're really seeing is that shorter pull requests elicit better feedback (in this case "more defects per line"). In my experience running code reviews, shorter pull requests, presumably due to their reduced effort necessary to understand, tend to get better review, review that is more than just superficial style/linting errors. In light of that, I always encourage developers to aim for the shortest reasonable changeset - and make it very clear to them that 2 line PRs are totally acceptable - on my current team, we go as far as to encourage tricks like rewriting (local) Git history and cherrypicking to ensure that is the case. Continuous integration and good unit tests help to ensure that strange states generated from that process are still good and I find the costs are far outweighed by the better reviews. Another little lesson I've learned managing that process is to encourage the submitting developer to highlight problem areas in his own code: when I submit my own code for review, I'll actually do the review first, line-level highlighting areas I wish the reviewer to pay attention to. This will only work if you can trust your developers to not try to "sneak something by," but if you can't do that, you've probably already lost. Developers generally know what they weren't sure about during the process, where things are going to be difficult to understand and where someone else on the team is going to have helpful contributions to the quality. I think it is important for developer-managers to remember that programming is largely a craft and developers largely want to be proud of their output. People _like_ producing "good code" and it is easy to align the goals by having the process help improve their craft.

majormajor 9y ago
This matches my experience, and also has implications for date-driven projects. People have two typical responses to long code reviews: skim and ignore, or go in depth and ask a lot of questions because there's a lot going on that's new. If it's a critical feature, I try to push the team to not just ignore, so the longer someone works in a silo, the longer it takes to merge those changes back in with everything else, the higher the risk of getting major "why not do it this other way instead" late feedback throwing a wrench in everything last minute. What I've taken away from recognizing this pattern is to ensure everyone's checking in frequently, even in the early design and idea phase, so that nothing ever turns into a bad surprise in a code review. Depending on the seniority of the dev driving the feature, there isn't a single one-size-fits-all prescription for what's a "too big" code review, but the rest of the team's job is to make sure that whoever is driving the feature never ends up in a position where their technical choices put the delivery at significant risk - nobody wants to feel like their team left them to sink or swim and then only offered criticism when they finally submitted their code for review. I'm also a huge fan of the described "identify what I dislike about my own code when submitting it for review" practice, too. I'll come straight out and ask "I don't like how I did this, anyone got better ideas?" and I've seen other people start picking this up on the team too.

hacker314159 9y ago

A lot of the comments here imply that style can be fixed with a linter, but what about the style of things other than syntax, like choices in variable names, DRY-ing through mixins vs composition, grammatical style in code comments, etc.