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

151–160 of 169 posts

Re: Code reviews do find bugs

#151

Post by seadan83 »

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'd guess I've done somewhere in the ballpark of 1000 to 2000 code reviews. After that time, my opinion of them has drastically changed. I can give something of a long laundry list (my apologies for it being kinda ranty...):

CR slows things down & often unnecessarily. Overly time consuming

Time lost with needless explanations.

Somewhat impossible when reviewer pool is one or two people. A bigger reviewer pool is probably not going to have to have context.

Creates a bias to not ship. "waiting for CR" can be a big impediment for shipping. Perhaps that tonight was the good time for you to get something into prod, not tomorrow - but you're waiting for a CR.

It's an example of process over people (AKA: cargo-culting). The context of when/where/how/why CR is important and is situational. CR best practices are going to have different results in different situations. Often CR is just done because it is a best practice, blindly. It would be preferable to think deeply about what is important & why - rather than just a "oh yeah, here is another hoop we jump through because #AmazonDidIt"

Stratifies the team. The senior/elite/favored people invariably get easier times during review.

CR can get political. Scrum masters & managers scrambling to unblock a team member and get something reviewed. Which is great for that one time, but reveals an utterly broken process otherwise. When a CR is "escalated", what's the chance the reviewer will actually spend the time needed and the back-and-forths to get things "properly" reviewed?

Conducive to nitting, which are are useless and counter-productive. Time spent on nits is waste and draining to keep coming back to something to then tweak "is it good enough yet?"

Drive by reviews without context

Coming to agreement during CR is difficult. Not everyone is able to observe/experience and/or resolve conflict.

CR is late in the code development process; it's one of the worst times to get feedback after something has been done, polished, tested, made production ready - and suddenly then someone thinks it should be done differently (which is a lot easier to see when something is already done and working). It's akin to writing code 3 times, once to get it to right, a second time to get it nice, and a third time for whatever the hell the reviewer wants.

Shows lack of trust in team. It is gatekeeping.

Does not scale well. I was once told by a reviewer that I write code faster than they could read it. (At the time I reviewed about 6 PRs every day, and sent out about 3. I was doing 80% of reviews, it was a shit-show; the reviews I received were slow and useless - the team members were focused on trying to deliver their over-stretched projects; I was too streched to give proper feedback and not work 100 hours a week).

Better options exist, namely the "ship/show/ask" strategy: https://martinfowler.com/articles/ship-show-ask.html

That latter branching strategy puts it up to the code author to decide, "does this comment typo fix need a review? Does this fix in this code that I authored originally and know everything about - really need a blocking review?" In that last case, if a person can just merge a bunch of precursor refactors right away, they can get that knocked out - the PR they send out for review is then clean & interesting; and there is no time lost managing branches or pestering someone

A second option to help make things better is to allow "after-merge" reviews too. A few teams I've been on, we did that enough where we learned what kinds of things are good to ship on their own. To another extent, there wasn't the bandwidth to review everything. It was not a bad thing.

Re: Code reviews do find bugs

#152

Post by seadan83 »

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.

Use the delete key. (My apologies for the snark, but I'm kinda serious, don't send those type of review comments at all).

If it's fine for now, it's fine. If it's not fine - then speak up.

If you do have such concerns, "fine for now" - that is something for an offline conversation - outside of the context of a CR. Which is also a good litmus test for how important a comment is. Would you schedule a 15 minute meeting to discuss the issue with your colleague? If not, then why is it okay to do that to someone during a CR?

Re: Code reviews do find bugs

#153

Post by seadan83 »

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.

To add to this, "no time like now" is real. Follow-ups rarely happen, tracking them is a pain. Making someone accountable for it is unfair, particularly if they disagree with the follow up (and presumably they do, otherwise they would have done the thing in the first place).

Re: Code reviews do find bugs

#154

Post by seadan83 »

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.

"in-person" review can be the best I find. If a CR is going to have a dozen comments about everything, it's a disaster to do that async - the author is going to feel attacked. The dialog to resolve the conflict & disagreements is probably not going to play out well.

Personally I like it when teams start with in-person CRs if they are not used to them. Only after healthy relationships and conflict resolution mechanisms established, do the PRs go async. I also like a rule that there should never be more than 2 round trips on communications in a CR, otherwise it should be done at the same time.

Delaying small changes to allow for such round-trip times is not feasible in a lot of environments. There's too much to be done. Not worth it. When projects do fail, it's very rarely that some manager slaps a big "failed" label on it. I bring that up to say that projects fail all the damn time because the team is moving too slow - and individuals often don't appreciate when they actually did need to move faster (the feedback loop is often missing, of, "oh, we actually were too slow, we needed to ship faster than we actually did. The JIRA, planning, estimation, CR, best practices, time spent fixing linting - it all doesn't matter cause this project actually failed - it took too long.")

Re: Code reviews do find bugs

#155

Post by bobthepanda »

ahepp wrote: post #141
bobthepanda wrote: post #90

Earlier quoted context omitted.

Also there is something to be said that code reviews also work well with code that is meant to be reviewed. The worst kind of peer review happens on PRs that are thousands of lines because nobody wants to read all that and things will be missed. Where I have seen successful code review is where people break code into reviewable bits, and those individual reviews are so fast that they actually end up bring completed f…

do you think this can be done at the level of commits in a PR, or is there a big advantage to making it multiple PRs?

If each commit can be exposed to a separate review then that works.

The main issue is humans,

1. Do not want to read long things, slowing down approvals since no one wants to touch it

2. Cannot effectively review long things and something is more likely to slip past.

Re: Code reviews do find bugs

#157

Post by danielmarkbruce »

seadan83 wrote: post #151
danielmarkbruce wrote: post #82

Earlier quoted context omitted.

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'd guess I've done somewhere in the ballpark of 1000 to 2000 code reviews. After that time, my opinion of them has drastically changed. I can give something of a long laundry list (my apologies for it being kinda ranty...): CR slows things down & often unnecessarily. Overly time consuming Time lost with needless explanations. Somewhat impossible when reviewer pool is one or two people. A bigger reviewer pool is prob…

This is a bunch of downsides, but I guess I'm asking, does anyone intelligent think that given the alternatives, on balance, it's better to not do them? You seem quite against them - what would you propose as a concrete alternative and what would the result likely be?

Re: Code reviews do find bugs

#158

Post by BobaFloutist »

godelski wrote: post #76
hakunin wrote: post #67

Earlier quoted context omitted.

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 And it's worth noting that having some fat is good. I can get it when you're a startup and you're trying to pull yourself up by your bootstraps, but at some point of time you need some fat. Too much fat is bad, but no fat is also bad. Startups run lean because they have to but when big businesses run too learn, it's called anorexia.

Another way of looking at it is that fat serves a purpose in the body. It's true that if you're optimizing for a very narrow outcome, very low (but not no) fat bodies can look ideal, but for one thing, that's actually a pretty unhealthy body, and for another, the best weight lifters in the world actually have pretty substantial fat reserves to support and sustain their muscles.

No fat can help for going fast and efficient, but it prevents you from going strong.

Re: Code reviews do find bugs

#159

Post by seadan83 »

danielmarkbruce wrote: post #157
seadan83 wrote: post #151

Earlier quoted context omitted.

I'd guess I've done somewhere in the ballpark of 1000 to 2000 code reviews. After that time, my opinion of them has drastically changed. I can give something of a long laundry list (my apologies for it being kinda ranty...): CR slows things down & often unnecessarily. Overly time consuming Time lost with needless explanations. Somewhat impossible when reviewer pool is one or two people. A bigger reviewer pool is prob…

This is a bunch of downsides, but I guess I'm asking, does anyone intelligent think that given the alternatives, on balance, it's better to not do them? You seem quite against them - what would you propose as a concrete alternative and what would the result likely be?

Just as there is no single strongest "for-argument", same with the "against-argument" -> context matters more than 'best practice'.

FWIW, I noted:

> Better options exist, namely the "ship/show/ask" strategy: https://martinfowler.com/articles/ship-show-ask.html

AT the end of the day, IMO it's like Agile & Scrum. You actually have to think hard about your context, can't just go in and apply a boiler-plate one-sized fits all solution. (I've got a chip on my shoulder these days as I feel too many in IT don't really want to think hard about their context. Too much of: "I want the magic formula for my team to 'go fast!'". The mantra: just do the stand-ups, do the retro, do the code review, do the planning poker, add checkstyle (a code syntax linter) - it'll all go great! I've now learned to ask - but did the project ship in time? Were any of those items actually useful in this context? If so, tell me exactly why and give details. I've also learned that if a team is far in the stone age, trying to adopt all of the best practices is likely to take longer than the remaining lifetime of that team and/or company. Which means they will do nothing further useful other than churn on process changes. Which brings me back to CRs, how many of them are actually useful? Some CRs are useful for sure. Though, how can a team spend more time doing useful CRs and less time doing non-useful CRs. What's more, every PR is a blocking activity, if we are blocking development for a usually not-useful activity -> that is a problem. The alternative does not have to be to throw out CR completely - I already mentioned two: "ship/show/ask", & post-merge review)

Post reply on HN