A bogus study on code review
blog.wesleyac.com
A bogus study on code review
1–10 of 61 posts
Re: A bogus study on code review
#2This 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/0dd349ad701e1e4045282b...
Re: A bogus study on code review
#3It’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.
Re: A bogus study on code review
#493% 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.
Re: A bogus study on code review
#5It’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.
A real code review takes a lot of time. With the time available typically you can only find superficial things like style.
Re: A bogus study on code review
#6I 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).
Re: A bogus study on code review
#7It’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.
If you search "Counting Defects" in the pdf posted by Morendil ( 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.
Re: A bogus study on code review
#8I'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.
Re: A bogus study on code review
#9It’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.
A real code review takes a lot of time. With the time available typically you can only find superficial things like style.
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.
Re: A bogus study on code review
#10It’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.
A real code review takes a lot of time. With the time available typically you can only find superficial things like style.
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.