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

11–20 of 169 posts

Re: Code reviews do find bugs

#11

Post by jellyfishbeaver »

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 amount of effort for theirs.

- People blindly accept suggestions with no resistance or discussion to get the review over with.

- People send their MRs to side channels or other teams to get their changes merged in without resistance or back and forth. (I've had to revert several of these).

Re: Code reviews do find bugs

#12

Post by audiodude »

saulpw wrote: post #9
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…

How do you phrase these warnings? "Next time.."? I have a hard time being serious with my own warnings if it's fine enough for now.

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

Re: Code reviews do find bugs

#13

Post by dgb23 »

sarchertech wrote: post #5

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…

Both code reviews and pair programming can be very useful if they serve a specific purpose.

Getting someone up to speed with unfamiliar code, disentangling hairy code so it becomes clearer, hunting down bugs or finding unknown unknowns such as bugs or unnecessary complexity.

However in many cases not looking at the screen when doing these kinds of things is more helpful. It's often more beneficial to build a mental model in your head and then riff off each other. Rather drawing things on a board or writing down stuff in a markdown file, explaining things in simple terms, than actually coding or reading actual code.

Not sure if that still counts as pair programming or code reviewing but this free form way of talking about code is very effective.

Re: Code reviews do find bugs

#14

Post by spankalee »

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.

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 reenforcing all kinds of cohesion would be a big mistake in my opinion.

Re: Code reviews do find bugs

#15

Post by spankalee »

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 is made, it's not accidental.

I would raise all these issues and more in group meetings. Try to get people to understand the many different benefits review brings to both the committer and reviewer - by having them state the benefits they want or could see getting. Talk about various kinds of comments (clear bugs, performance, style, robustness, factoring and organization, etc.), and the various priorities from no-action-required to nits to blockers. Talk about the priority of reviews themselves. Reducing the latency of reviews has a huge positive effect, ime.

Re: Code reviews do find bugs

#16

Post by est31 »

audiodude wrote: post #12
saulpw wrote: post #9

Earlier quoted context omitted.

How do you phrase these warnings? "Next time.."? I have a hard time being serious with my own warnings if it's fine enough for now.

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

Re: Code reviews do find bugs

#17

Post by david2ndaccount »

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

Re: Code reviews do find bugs

#18

Post by ehsankia »

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

Re: Code reviews do find bugs

#19

Post by closeparen »

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.

Re: Code reviews do find bugs

#20

Post by alex_lav »

spankalee wrote: post #14
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.

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.
Post reply on HN