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

111–120 of 169 posts

Re: Code reviews do find bugs

#111

Post by anonymoushn »

withinboredom wrote: post #109
anonymoushn wrote: post #103

Earlier quoted context omitted.

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

The question was specifically about scenarios in which this approach wouldn't work, for example because your team doesn't want to approve PRs containing only dead code or because any subset of the change won't compile or won't preserve correct behavior without the others pieces.

Re: Code reviews do find bugs

#112

Post by pavel_lishin »

klooney wrote: post #44
mikepurvis wrote: post #31

Earlier quoted context omitted.

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

The previews are valuable though, it makes you look like a wizard when you already know how something broke.

But it makes you feel like Cassandra when you keep warning people about the same problems, and keep running into them months and years down the line.

You can only post the Surprised Pikachu meme so many times before it loses its luster.

Re: Code reviews do find bugs

#113

Post by dkdbejwi383 »

anonymoushn wrote: post #111
withinboredom wrote: post #109

Earlier quoted context omitted.

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

The question was specifically about scenarios in which this approach wouldn't work, for example because your team doesn't want to approve PRs containing only dead code or because any subset of the change won't compile or won't preserve correct behavior without the others pieces.

It helps to have the right tooling in place to ship "incomplete" work, e.g. feature flags so that you can ship a very light and not ready for end-users version of some feature, and continue to iterate on it in smaller PRs.

e.g. first pass adds a new screen and just dumps the output

second pass adds input fields and controls

next pass adds validation

then add animations

etc

Re: Code reviews do find bugs

#114

Post by jt2190 »

gwd wrote: post #108
jt2190 wrote: 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…

If a comment points out a bug/defect [1], then it should block.

If you think about it, as bugs/defects are removed, the code becomes more correct and thus more stable because it doesn’t need additional changes to remove bugs, so removing bugs reduces the need for future maintenance.

If we block due to future maintenance concerns what we’re really asserting is that the requirements are unstable, and that removing today’s bugs is less valuable overall because requirement changes will remove the line of code with the bug and replace it with a new line of code with a new bug.

I suppose it depends on the code review process at at a given organization whether that’s the appropriate point at which to block code for architecture/design issues. In my experience the code review step is much too far downstream in the development process and much too narrowly focused on a subset of code to be an effective place for design changes that have significant impact on maintenance.

[1] The paper authors reviewed data in Microsoft’s internal code review tool, which is proprietary, so we can’t see what the specific bugs were.

Re: Code reviews do find bugs

#115

Post by dkdbejwi383 »

danielmarkbruce wrote: post #82
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…

Never realized this was a debated topic. Are there smart people who believe in not having code reviews? What's the best argument against code reviews?

I worked in a team that didn't do reviews because _everything_ including spikes, research, etc, was done by two engineers pairing. This was remote, cameras on, all day.

I found it utterly exhausting. I was somehow working at 100% capacity but producing output at 50% because so much of my cognitive bandwidth was taken up with the pairing process.

Re: Code reviews do find bugs

#116

Post by dkdbejwi383 »

poikroequ wrote: post #34

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

> But if you have a new guy on the team, or a junior, you'll definitely want to review their code.

Reviews _from_ juniors or new team members are also really valuable, as they don't have the history or tribal knowledge that others may have. They'll often spot things that have gone overlooked because "that's how it is".

Re: Code reviews do find bugs

#117

Post by giancarlostoro »

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…

> “kind, not nice”

Always when joining a team the first thing I tell devs is "I don't care how critical you are, just be honest" I think setting expectations early on is very critical. I think people not feeling attacked / too defensive of code is a good step forward. People who vehemently defend their code are bad developers imho.

Re: Code reviews do find bugs

#118

Post by pavel_lishin »

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…

> The office is not a safe space.

It well fucking should be a space where I'm safe from my boss throwing large projectiles at me.

Re: Code reviews do find bugs

#119

Post by pavel_lishin »

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'd love to explore that alternative, but I'm not sure how to actually make sure that any errors found/changes suggested after the commit is pushed and deployed actually get implemented.

Re: Code reviews do find bugs

#120

Post by mikepurvis »

klooney wrote: post #44
mikepurvis wrote: post #31

Earlier quoted context omitted.

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

The previews are valuable though, it makes you look like a wizard when you already know how something broke.

Often it's more subtle than just "this is clearly going to take down production in X way at some point." The issue is more like feeling that the logic is too entangled and it's going to be hard to maintain later on, or that a library should or should not have been used, or something done with threads should have been async.

So yeah, not as cut and dried as "I said it would happen and it did" but more like "I had a feeling this was going to turn out to be a pain and sure enough here I am reviewing code that represents that pain."

Post reply on HN