Live data from Hacker News

Code reviews do find bugs

two-wrongs.com

101–110 of 169 posts

Re: Code reviews do find bugs

#101

I'm not sure code reviews hold much merit. I've been a web developer for around 12 years and I've worked in companies big and small. I think there should be a manual QA process to test the functionality of what the developer is pushing out. The issue with code reviews is always that they take so much time for another developer and many devs are super busy so they just have a quick review of the PR and approve or feel…

I'm a little surprised to hear this. Would you mind sending an email to hn@xkqr.org so that I can ask some follow-up questions, please?

Re: Code reviews do find bugs

#102

I remember a time before you needed an approval to merge a PR (I also remember a time before PRs or any widespread version control system). I can count on one hand the number of times someone has caught a bug in my code that should have stopped deployment. Not that I haven’t deployed serious bugs to deployment, but they’ve almost never been caught by someone reviewing my code. Occasionally someone suggests a better w…

It certainly sounds like you write seriously high-quality code! And judging from your profile, I'd be inclined to think you know what you are talking about. I'd like to ask a little more around your experience here. Do you mind sending an email to hn@xkqr.org so that I can ask some follow-ups?

Re: Code reviews do find bugs

#103

Earlier quoted context omitted.

Good PR culture is definitely something that has to be built from the ground up, and supported top down. At Shopify, who I think has a really good PR culture we have a few things that I think help (beyond a good CICD, and static analysis tools): 1. PRs are supposed to wait for 2 acceptances, can be shipped with 1, and can be emergency shipped with 0. So the barrier is low, but the culture supports more. We are expect…

Also there is something to be said that code reviews also work well with code that is meant to be reviewed. The worst kind of peer review happens on PRs that are thousands of lines because nobody wants to read all that and things will be missed. Where I have seen successful code review is where people break code into reviewable bits, and those individual reviews are so fast that they actually end up bring completed f…

How much additional time is needed to break a self-contained change that's the smallest it can reasonably be without breaking anything into a bunch of smaller changes though?

Re: Code reviews do find bugs

#104
Bear in mind I am pro code review, but...

There is a trick in pharmaceutical research where you test a potential candidate drug against placebo to yield a bad study that seems to show benefit. The reason it is a trick is because in many cases the alternative isn't placebo, it is an existing treatment. Then doctors learn about a more "modern" treatment, favor it for being modern and the better treatment may not be prescribed.

The alternatives to code review aren't doing nothing. The article claims that code reviews find a defect per 10 minutes--but only in the first ten minutes. By this same argument (ignore qualifications, extrapolate the numeric result), fast automated testing can potentially find thousands of defects in a second--if they run that quickly and the defects were already tested for. Static analysers, pair programming, documentation these are all alternatives and there are many more.

If you're spending an hour a day reviewing code then you are spending 12.5% of your time doing it. Using it that way comes with an opportunity cost that may be better spent depending on your particular organization and code base. Of course, analysing everything to death also has an opportunity cost, but not analysing it generally leads to moving goal posts where the supposed rationale for doing something keeps changing. Today its purpose is uncovering defects, tomorrow it is knowledge sharing, the day after it is security. It is all of those things, but other practices may achieve these goals with more effective use of time and people's patience.

So why am I pro code review? Because choosing to interact and work together as a team, to learn about and compromise with your colleagues makes for good team building while serving other technical purposes. I do think that pair programming can achieve this to a greater level while also being more taxing on individuals. This assumes you control the process and own it though, if it has just become a rote ceremony then my feelings are you probably aren't net benefitting from it: you are simply doing it because you have no choice, not because you believe it to be a valuable use of time. If you have experienced both, a culture where people choose and find value in code reviews and a culture where people are forced to do it unquestioningly, then you may have witnessed how a dicta can destroy the prosocial value of a practice.

Re: Code reviews do find bugs

#105
post #62

I find things wrong with virtually every nontrivial pull request when I’m the reviewer. Sometimes these are minor issues, but I spot bugs and edge cases all the time. I see some comments about time. How long does a code review take? I can review hundreds of lines of code in a few minutes. It is much easier to review code than to write code imo, especially as you gain experience. For bigger efforts, get eyes on it thr…

When I was working with Django, those who added code ran it in their systems. With lambda, which we mostly deploy and test in the cloud, people tend to dump code and leave.

Re: Code reviews do find bugs

#106

If you have a spellchecker, code formatter and a linter, code reviews improve significantly. Much better than having to do that work by hand, or reviewing it by hand, leaving code reviews for higher level ideas.

Exactly. Code reviews shouldn't be about code formatting or anything that can be automated away with linters, formatters, code coverage limits and static analysis. If the build is green for the PR, then all that is already acceptable.

Re: Code reviews do find bugs

#107

The value of code reviews really depends on the code and the person working on the code. For a team who have spent years working on the same repo, code reviews may not hold much value. But if you have a new guy on the team, or a junior, you'll definitely want to review their code. Code reviews can also do more than just find bugs. You can point out a better way of doing things. Maybe this SQL could be more efficient.…

> For a team who have spent years working on the same repo, code reviews may not hold much value.

I have definitely found bugs [ETA during code review] in code written by very senior developers in code they've been familiar with for over a decade.

Re: Code reviews do find bugs

#108
post #46

I’m not sure why the author ignores the “… that should block a submisson” part. The abstract of the paper: > Because of its many uses and benefits, code reviews are a standard part of the modern software engineering workflow. Since they require involvement of people, code reviewing is often the longest part of the code integration activities. Using experience gained at Microsoft and with support of data, we posit (1)…

The "that should block submission" is always one of the trickiest parts. There's a saying: "Everyone that drives slower than you is an idiot, and everyone that drives faster than you is a maniac." But it is true that going faster increases danger, and there is a speed that appropriately balances benefit against risk; but everyone perceives it differently.

The same is true of "code smell" issues: Everyone who asks you to change things is a pedant who's slowing down the project for pointless aesthetics, and everyone who pushes back against changes you've requested is a cowboy who is going to make the code harder to maintain in the future.

So in the paper, how did they decide whether a non-bug change "should block submission" or not?

Re: Code reviews do find bugs

#109

Earlier quoted context omitted.

Also there is something to be said that code reviews also work well with code that is meant to be reviewed. The worst kind of peer review happens on PRs that are thousands of lines because nobody wants to read all that and things will be missed. Where I have seen successful code review is where people break code into reviewable bits, and those individual reviews are so fast that they actually end up bring completed f…

How much additional time is needed to break a self-contained change that's the smallest it can reasonably be without breaking anything into a bunch of smaller changes though?

Like 10-15 minutes ....

    git co master
    git co my-branch -- .
    git add -up . # select changes relevant to first pr
    git commit
    git reset HEAD --hard
    # and again...

Re: Code reviews do find bugs

#110

The value of code reviews really depends on the code and the person working on the code. For a team who have spent years working on the same repo, code reviews may not hold much value. But if you have a new guy on the team, or a junior, you'll definitely want to review their code. Code reviews can also do more than just find bugs. You can point out a better way of doing things. Maybe this SQL could be more efficient.…

Code reviews also keep the team up to date with what is changing in the code
Post reply on HN