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

61–70 of 169 posts

Re: Code reviews do find bugs

#61

Post by jiggawatts »

godelski wrote: post #41
lhamil64 wrote: post #25

Earlier quoted context omitted.

My company/team has very different processes for bugs vs feature requests. If a customer opens a ticket and we determine it's a bug, we will generally fix it in the reported release and later (unless it's a security vulnerability or other major problem). But for feature requests we just tell them to submit it to a community and we evaluate it to see if it's valid and something we'd likely implement given the other wo…

Sometimes feature requests are actually bugs and can be illustrative of one not properly understanding design. But I think it is important how user feature requests are interpreted. They have a frustration that you might not be aware of but they aren't aware of all the code and constraints. It can even be in design, which is still important. Very often there is a way to resolve a feature request that is not what the…

> Sometimes feature requests are actually bugs

You can have two missing features that add up to a bug in total. For example, I worked with two cloud products from the same vendor where a missing back-end HTTP feature of the CDN product interacted with a missing HTTP front-end feature of the PaaS service such that the two products that have a "natural fit" together couldn't actually be used in combination.

This made many architectures that ought to have worked a no-go, forcing customers into contorted design patterns or third-party products.

IMHO this is a bug ("Can't use your products"), but each team individually marked it as a missing feature and then they just ignored this for about three years.

Also: not enough people voted the missing features up because not enough people were using the products... because they couldn't.

I know this is a bit off-topic here, but it circles back to the "statistics is hard" intro in the original blog article. You can make catastrophic business mistakes relying on statistics you don't full understand, such as this example of "you won't get many complaints for unusable products".

You will get many complaints however for the usable products... they have users to complain.

https://en.wikipedia.org/wiki/Survivorship_bias

Re: Code reviews do find bugs

#62

Post by grumple »

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 throughout the process.

I’ve met a lot of developers who assume their code will just work right after they write it. They don’t test it, via code or manual qa. Then they act surprised when the stakeholder tells them it doesn’t work. Do the job right the first time. Slow is smooth and smooth is fast.

Re: Code reviews do find bugs

#63

Post by sadops »

alex_lav wrote: post #10

Code reviews can find bugs. More often, code reviews become opportunities for team members to bikeshed. Worse, an opportunity for a non-team member to exert power over a project.

From where I sit, it's usually the people writing the bugs who are so averse to code reviews.

Re: Code reviews do find bugs

#64

Post by sadops »

alex_lav wrote: post #20
spankalee wrote: post #14

Earlier quoted context omitted.

Bikeshedding in a team can be good. If you're all painting the shed, it helps to agree on the color. More generally, code review is a great opportunity for incrementally gaining or encouraging alignment across the team. What the team chooses to align on and how strongly are left up to it, so hopefully they choose to not get bogged down in inconsequential details, but completely skipping the pretty cheap chance for re…

You're making a lot of positive-upsided assertions about code review. My point is there is too much opportunity for negative behavior. It's the same as everything in tech, in life, "It can be good if everyone does their part to keep it good". And yet, most don't.

[flagged]

Re: Code reviews do find bugs

#65

Post by rebeccaskinner »

I think the article is taking the wrong view. The statistic cited by the article that 15% of comments were about a bug seems in line with expectations, and I think it would only really be worth discussing if the number were _much higher_ or _much lower_.

Instead, I think there are two far more interesting questions to ask:

1. Is the rate at which code review identifies defects sufficient to use code review as a detection mechanism for defects?

After nearly 20 years of writing software, I'm pretty convinced that the answer here is no. Some reviewers are better than others, and some circumstances are more favorable to finding defects than others, but we should generally try to build processes that don't assume defects will be caught at a substantial rate by code review. It's nice when it works, but it's not a reliable enough way to catch errors to be a load bearing part of the process.

2. Is mandatory review of all code justified?

This is the one I'm on the fence about. In an environment where code reviews are high priority, people are trained to review effectively, and there are minimal organizational politics at play, then I hypothesize that allowing PR authors to decide whether to get a review or not would generally improve quality and velocity because code would ship more quickly and code that would benefit from a review would still be reviewed. In that scenario, I think we'd see the benefits of getting things shipped more quickly when they don't require a review, and reviews would be higher quality because code being flagged for review would be a positive sign to pay more attention.

Unfortunately, I could be wrong, and it's not the sort of experiment anyone wants to risk their reputation pushing for, so I doubt we're likely to see an experiment at a large enough scale to know for sure. If we're going to fail one way or another, I'd prefer to fail by doing too much code review rather than not enough.

Re: Code reviews do find bugs

#66

Post by lazyasciiart »

zer00eyz wrote: post #55
jellyfishbeaver wrote: post #11

Earlier quoted context omitted.

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

As far as dodging projectiles goes: yes the office is supposed to be a safe space, and if someone threw a chair at me, one of us would not work there the next day. (Add normal caveats for "maybe they threw the chair to save you from the ninja creeping up behind you".)

Re: Code reviews do find bugs

#67

Post by hakunin »

mikepurvis wrote: post #31
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, a…

Couple of points on this.

1. A lot of problems arise from too few people working on too many things. If it's one-two devs and backlog is growing, the problem is not that you have no time to fix things, but that you're understaffed. If you have enough people, then from the business perspective it shouldn't even be that noticeable that someone is refining previous work, while someone else is building the next thing.

2. If you're not understaffed, then the best time to clean up new code is during or immediately after writing it. A phrase I like to use is "while it's still fresh in memory". You're saving time and not adding new bugs, by not having to remember everything again, load all that context back into your head.

Re: Code reviews do find bugs

#68

Post by hakunin »

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…

I've written up some rules that I've successfully applied in my teams. https://max.engineer/mindful-code-reviews

Re: Code reviews do find bugs

#69

Post by goosejuice »

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

The discipline of putting up small, coherent, explained, tested, and review able units of change, that you have looked over and feel comfortable showing off to other people as your work product, is 80% of the value for me. Whether anyone else actually thinks about it deeply or has something useful to say about it is secondary.

Indeed. It's kind of like rubberducking.

Re: Code reviews do find bugs

#70

Post by [deleted] »

ecshafer wrote: post #27
jellyfishbeaver wrote: post #11

Earlier quoted context omitted.

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…

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…

[deleted]
Post reply on HN