Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Code reviews do find bugs

two-wrongs.com

Post reply on HN
Print view View original

31–40 of 169 posts

Re: Code reviews do find bugs

#31

Post by mikepurvis »

willio58 wrote: post #3

Agreed. I mainly manage and review code at this point in my career. I find many bugs, every once in a while finding something that would have caused an outage or notable problem for users. What I find more though is code that isn't thought through. Tech debt and code smell are real, and they affect the performance of a team. Nipping that in the bud takes quality PR reviews and time to meet with submitters around issu…

"Tech debt and code smell are real"

I think what I struggle most with is that often times there's a valid business reason to "just ship it ASAP", but the missing piece is the accountability around the conditions attached to that. Like, okay, if we don't want to fix this now because it needs to be in the next release then we can merge it as-is, but you can't document this externally, it can't because part of an API, and there can't be any further development in this direction until X, Y, and Z have been rewritten to be in like with ABC.

I find it profoundly hard to get buy-in for those types of discussions. Everyone is happy to smile and nod and the appropriate tickets are filed with deadlines attached, but then the next release rolls around and there's new business-imperative stuff that's the focus and the cleanup tickets are quietly moved to backlog with the deadlines removed.

Seeing this repeated over a number of years has left me with kind of a cynicism about the process, where it feels like code review is at least partly an exercise in frustration; I don't have the backing required to insist on doing it right upfront, so instead I'm really just getting a preview of what is going to land in my lap a year or two from now.

Re: Code reviews do find bugs

#32

Post by cjriley »

david2ndaccount wrote: post #17

In my experience, code reviews catch a lot of bugs. However, if you find yourself catching the same kind of bugs over and over again in review you should be finding ways to catch them automatically without involving a reviewer (static analysis, tests, linters, etc.)

Completely agree on utilizing static analysis as much as possible. My first instinct when finding an issue in a code review is to think, "could we have caught this with a of some kind?"

Re: Code reviews do find bugs

#34

Post by poikroequ »

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. Maybe you can refactor some bit of code to make it more robust. Maybe you should put a logging statement here. This method name is confusing, may I suggest renaming it to xyz?

Re: Code reviews do find bugs

#35

Post by xmprt »

jellyfishbeaver wrote: post #11
willio58 wrote: post #3

Agreed. I mainly manage and review code at this point in my career. I find many bugs, every once in a while finding something that would have caused an outage or notable problem for users. What I find more though is code that isn't thought through. Tech debt and code smell are real, and they affect the performance of a team. Nipping that in the bud takes quality PR reviews and time to meet with submitters around issu…

I am a new manager and I am struggling to get my team to understand the value in code reviews. I have been through so many rewrites and re-re-writes of spaghetti code, I am much more critical now reviewing code, and I am trying to promote this culture on my team. Do you have any suggestions? - The same people leave detailed comments on others' merge requests, but get discouraged when nobody else puts in the same amou…

Culture for code reviews doesn't start out of thin air. Unless you have processes for CI/CD, testing, task estimation, retrospectives, incident postmortems, etc., there's never going to be a point where you will convince people that they're helpful. So start with those.

There's always going to be pushback from adding more process, but if there's an understanding amongst the team that keeping things working is P0 then these processes will slowly/naturally come up as the team realizes that investing in them proactively will save them time down the road.

Re: Code reviews do find bugs

#36

Post by some_furry »

> During the first 60 minutes of code review of the day, the reviewer finds roughly one defect per ten minutes of reviewing – as long as they review less than about 50 lines of code per ten minutes.

Oh.

It normally takes me a few seconds to find bugs in code.

I always felt this was average performance for assessing software. If the average time is ten minutes per defect, I need to recalibrate my expectations for myself.

Re: Code reviews do find bugs

#37

Post by godelski »

ehsankia wrote: post #18
godelski wrote: post #4

I think this is really important in that it is bigger than "code reviews." It does show how people greatly misunderstand statistics[0]. And what's even funny is at surface level the claim that code review "does nothing" __sounds__ ludicrous. But people "believe" because they are annoyed with code review, not because they "actually" believe the results. But statistics are tricky. With the example given in the article…

Basically, code reviews also happen to find a lot of other non-bug stuff (probably nits and style issues). That's why looking at % is dangerous. You could be finding 5 bugs per code review, which is a lot, but if you also make 30 other non-bug comments, suddenly "only 15% of comments are bugs".

Oh I completely agree. There are just a lot of things that can't so easily be measured and many things that can never be. But that doesn't mean they don't matter. Following the point you're making, enforcing good style can result in bugs not happening later on or even save a lot of future time as your code doesn't slowly spaghetti. And I think that's one where people often miss. That spaghetification happens generally through a slower process. By dozens of commits, not by a handful.

Re: Code reviews do find bugs

#38

Post by sfink »

est31 wrote: post #16
audiodude wrote: post #12

Earlier quoted context omitted.

"This is okay for now, but we should think about how we want to serialize these objects. Feel free to remove the N^2 algorithm in a follow up."

That works great in a setting where you are both employees of the same company, and you respect each other, but it often doesn't work in the open source world, people just disappear and you never hear from them again. It is possible that they do file follow-ups, but in my experience it's rare.

Yes, even within a company my threshold for accepting a change can vary pretty widely depending on my experience and relationship with the author. For an external contributor or someone I've never collaborated with (by reviewing code or having my code reviewed), I don't accept the code until almost everything is worked out to my satisfaction. With someone I work with regularly, it's not uncommon to accept a change with a comment like "this is all good, but you need to take X into account which will change almost everything in this patch" (I exaggerate, but only slightly). I know whether an update could be problematic and whether it is necessary to see it again. Sometimes there are a couple of obvious ways that something could be done, they picked one but weren't tied to it if I had a reason for picking a different one, I picked a different one for $REASON.

Most are somewhere in between.

Though in some ways it works the other way around. For an unfamiliar open source contributor, I need to be confident that the change is worthwhile. I will be lenient on stylistic things, and I'll just land their patch and then fix it up afterwards. For someone I've worked with a bunch (whether a familiar contributor or a coworker), I will trust their opinion on the underlying quality of a change, but be less tolerant of unnecessary stylistic differences since they should have already come into alignment on those and it's more likely to be an oversight if they missed something. (Plus, I don't want to be fixing up their changes after the fact, given that >90% of patches will come from regular contributors.)

Re: Code reviews do find bugs

#39

Post by interactivecode »

simonw wrote: post #8
epolanski wrote: post #2

My beef with code reviews is that often they lead to tremendous amounts of wasted time, that's many thousands spent in a single week sometimes for simple pull requests. Working from 6 years, not much, and not in as many places like others, I have built the opinion that code reviews are like tests, they should be used as a tool when they are necessary, they shouldn't be the default for every change. In best case scena…

This is certainly true for blocking code reviews. I'm interested in exploring the alternative, which is review-after-commit. There's an article describing those here: https://copyconstruct.medium.com/post-commit-reviews-b4cc216... Code still gets reviewed, but you don't end up with PRs languishing for hours, days or even weeks waiting to get a review from someone.

I review a lot of code with the mindset of yes and…

Basically when I start the PR is approved in my head until I find something blocking. I.e. major problem that causes dataloss, big performance issue or breaks other code. Anything else is a minor comment at most. The PR is approved by default. This gives the dev responsibility and ownership. Plus it increases release cadence

Doing a follow up PR to improve or fix something is just as fast as blocking the MR, but blocking the MR can be bad for morale.

This strategy might work better in young startups where having the feature _exist_ is better than not shipping. in my experience this builds up responsibility and ownership and removes the whole policing of other peoples work vibe around code review.

Also decisions and discussion around formatting, when to test, design, features, functionality, architecture should not happen during code review, they should have happened way before coding, or collaboratively while working. Code review is the worst time for that stuff, imho it should be to sanity check implementation.

Re: Code reviews do find bugs

#40

Post by marcosdumay »

jiggawatts wrote: post #6
godelski wrote: post #4

I think this is really important in that it is bigger than "code reviews." It does show how people greatly misunderstand statistics[0]. And what's even funny is at surface level the claim that code review "does nothing" __sounds__ ludicrous. But people "believe" because they are annoyed with code review, not because they "actually" believe the results. But statistics are tricky. With the example given in the article…

> support sends you to their page that requests you to "upvote" a "feature" or bug issue. Microsoft does this for enterprise products where customers might be paying $100K/mo or even millions. “We hear you, but your complaint is just not popular enough so go away.” “Sure it’s a catastrophic data loss bug that ate your finance transactions, but if other people can’t identify that their seemingly unrelated crash is the…

You mean they don't censor the bug reports and try to gaslight you into believing their software is flawless anymore?

That's a tremendous improvement when compared to the time I interacted with them.

Post reply on HN