
A bogus study on code review - luu
http://blog.wesleyac.com/posts/bogus-code-review-data
======
gburt
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.

~~~
sharkdesk
In regards to your last sentence - as a developer, I _like_ solving business
problems, and coding is just a tool used to get there. What makes me proud is
when end users are able to use the application and it solves some problem they
have, and as a side effect, produces revenue for the company.

Religious adherence to processes gets in the way of that and if they block me,
I'm happy to shove them to the side and get executive support to do so. My
experience with code reviews is that they are 95% style nitpicks, 4% ways to
make code cleaner and simpler, and 1% real bugs which impact end users. I only
value the 5% myself. From a cost/value perspective, I haven't been sold on the
value of doing code review on everything.

Not all developers feel that way, sure. But it is a mistake to assume all
developers are driven by the same things.

~~~
gburt
I'm not sure I assumed developers are driven by the same things. A careful
reading of my last sentence did not define what it meant for code to be good
or by what process you would be proud of the output, I think that is a product
for you, your team and leadership to decide. Perhaps my use of the word pride
was too strong, the core of my philosophizing there is that software
development on a team is a social endeavour and that code review _can_ be a
very effective alignment mechanism.

Someone further down the page suggested that the style nitpicks arise from a
lack of understanding and familiarity with that piece of the codebase. I think
that is often true and a factor to be considered both when allocating code
review and when setting expectations regarding code ownership and cross-
functional understanding.

I've also provided some suggestions for how to tune down the "95% style
nitpicks" elsewhere on this page - it is a problem, but with appropriate
tooling and expectation setting, it can be reduced to a small fraction of
total code review output. It would be dysfunctional for a team to spend time
on that stuff when we've all agreed it isn't high ROI. I agree that code
review _can_ be done very poorly, but your observed ratios are not a fixed
property of the world of code review.

Let me be clear: I am absolutely not advocating for a process that motivates
nerding for the sake of nerding - I push back on a whole wealth of that sort
of behavior. I am a business person motivated by producing sustainable teams
that produce real value in the form of solutions to problems. I just feel that
is a high-dimensional problem and certain kinds of technical debt can come at
extreme cost and can, at times, if done correctly, be mitigated with processes
like code review.

~~~
sharkdesk
That is a fair rebuttal - perhaps my opinions are shaded by the fact that I've
worked for several companies and none of them have a code review process which
isn't 95% style nitpicks - the idea of one which lacks those has always been a
hypothetical thing in my mind, and not something which I've seen in the real
world.

------
Morendil
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/0dd349ad701e1e4045282b...](https://pdfs.semanticscholar.org/feb0/0dd349ad701e1e4045282b1bd10073718a7c.pdf)

------
prepend
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
A real code review takes a lot of time. With the time available typically you
can only find superficial things like style.

~~~
valuearb
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
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.

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

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

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

