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

51–60 of 169 posts

Re: Code reviews do find bugs

#51

Post by swatcoder »

As with most processes, the dilemma with code reviews is in figuring out how they impact your team and your organization.

In a huge org, with thousands of engineers that's already burdened by hours per day of interruptions and process overhead, and release runways that already involve six stamps of bureaucracy, mandatory code revies have very little downside (it's in the noise) but highly variable return (many people are just droning under the weight of process). The org loses nothing much for mandating it, but only certain teams will see a lot of value for it.

On the other extreme, a startup with five engineers will get backlogged with reviews (which then get shortchanged) because everbody either is under pressure to either stay in their high-productivity flow or put out some pressing fire. The reviews probably could catch issues and share critical knowledge very regularly, but the org pays a pronounced penalty for the overhead and interruptions.

People long for "one size fits all" rules, crafting essays and publishing research papers to justify them, but the reality of what's right is often far more idiosyncratic.

Re: Code reviews do find bugs

#52

Post by andirk »

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…

Please continue to be diligent in your PRs! Garbage code belongs in the garbage.

Re: Code reviews do find bugs

#53

Post by jonobird1 »

some_furry wrote: post #36

> 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 myse…

It really depends on the code. To find a CSS bug, yes easy peasy. To find a logic hole in a payment integration of what someone has missed or should have implemented but didn't (eg webhooks), then this requires a lot more time and the developer basically has to sit down properly to work out exactly what should have been implemented / how they would have developed it, and then cross-check it against what has been done, otherwise you won't be able to easily find those logical holes which effectively are bugs, just not simple code bugs like a missing semicolon.

Re: Code reviews do find bugs

#54

Post by some_furry »

lazyasciiart wrote: post #48
some_furry wrote: post #36

> 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 myse…

Presumably you understand that how long it takes to find bugs in code depends on the code. If not, then I hope you've read the code for Linux and SSL, etc.

Yes, of course it depends a lot on context.

I've never had an incentive to read the Linux kernel code. I routinely find and disclosed cryptography library bugs, though usually mostly hobby projects like the "I thought it would be cool if there was a PHP implementation of GHASH" sort rather than like OpenSSL.

Re: Code reviews do find bugs

#55

Post by zer00eyz »

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…

Years ago I had a boss who, in a moment, threw a chair at me. (this is much less dramatic than it sounds).

I would work for that man again in a heart beat. Because for as much as he was apt to yell, or dress me down, he was also willing to give good advice, to elevate, to teach.

The office is not a safe space. You seem to know what's wrong, IM sure you have asked nicely. I am sure you offered the carrot, but does your team know you have a stick?

> The same people leave detailed comments on others' merge requests

Call these people out, in public, for doing good work. Tell everyone they are setting the bar and others are not living up to it.

> People blindly accept suggestions

Coaching, lots of one on one coaching about finding and having a voice. Lots of "team building" where you level out the playing field with the strong vs weak voices. Figure out what those quiet ones excel at and do a fun activity around that. Let them find legs...

> People send their MRs to side channels or other teams

Stick. Harshly worded emails. Down dressing in public. Telling your team that in no uncertain terms that "this is unacceptable behavior"

As for the chair thrower... He was always fair, he always had his team first, I grew as a person, a manager and an engineer working for him. Its not growing happy go lucky good times while I get a pay check, its Growing pains, spreading that (pain) around is part of your job.

Re: Code reviews do find bugs

#56

Post by skywhopper »

jonobird1 wrote: post #49

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…

Nah, automated testing cover basic functionality. For most PRs, a senior familiar with the code wouldn’t need to check it out and manually test anything, that’s not what “code review” is most of the time. If you need them to look at the code in a running state, that should be part of the CI process, not a manual task for the developer.

A good reviewer can call out bad strategic coding decisions or misinterpretations of the requirements. QA is another layer of review entirely.

Re: Code reviews do find bugs

#57

Post by hyperadvanced »

closeparen wrote: post #19
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…

>time to meet with submitters around issues you find. What! I would be livid if someone scheduled a meeting with me about a PR. We have way too many meetings already, this is one of the only processes that is mercifully async.

It doesn’t need to be formal or very long. I personally enjoy a PR meeting where we can poke at the code and understand it over someone dumping 2,000 lines of code in my lap at lunch time and hoping to get their spaghetti to prod by dinner

Re: Code reviews do find bugs

#58

Post by hakunin »

jonobird1 wrote: post #49

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…

The problem here is not that developers are too busy, but that code reviews are considered second class citizens to churning out new code. It's like saying "many devs are super busy working on feature A so they just write quick and dirty code for feature B". If reviews are integral part of feature production pipeline, there should be no issue to sit down and spend a day reviewing code. For bigger, more complex things it could be a few rounds of reviews.

There is an approximate non linear relationship between time it takes to produce the first PR and time it takes to go through all rounds of review. This time can be pretty reliably calculated and taken into account.

Re: Code reviews do find bugs

#59

Post by some_furry »

jonobird1 wrote: post #53
some_furry wrote: post #36

> 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 myse…

It really depends on the code. To find a CSS bug, yes easy peasy. To find a logic hole in a payment integration of what someone has missed or should have implemented but didn't (eg webhooks), then this requires a lot more time and the developer basically has to sit down properly to work out exactly what should have been implemented / how they would have developed it, and then cross-check it against what has been done…

My day job is auditing cryptography. I'd probably be slower to find the root cause of a CSS bug than most of the folks that read HN. :3

Re: Code reviews do find bugs

#60

Post by jiggawatts »

marcosdumay wrote: post #40
jiggawatts wrote: post #6

Earlier quoted context omitted.

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

> don't censor the bug reports

They do, but eventually even the polite but grumpy comments build up to the point that it looks bad. These comments are public -- that's the whole point -- so the only way to hide them is to delete them. Normally this upsets users even more, so the "trick" is to "improve" the service by dropping the entire forum on the floor and starting over with a new piece of software. Not because it's better in any way, but because it is an implicit DELETE * FROM "BUGS".

Microsoft is on their... what... third forum now? I lost count.

Post reply on HN