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

131–140 of 169 posts

Re: Code reviews do find bugs

#131

Post by bobthepanda »

jappgar wrote: post #129
dkdbejwi383 wrote: post #113

Earlier quoted context omitted.

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

IMO this is a terrible approach, and why I hate the way feature-flags are used nowadays. For example, I'm not approving anything without input validation (frontend or backend). I have no idea if you're actually going to add validation later before the fflag is removed. "Trust me bro" doesn't work for me.

I mean you can have validation for the features you’ve written already behind the feature flag, while holding off on the stuff that doesn’t exist yet.

Feature flags don’t mean throwing the baby out with the bathwater.

Re: Code reviews do find bugs

#132

Post by Izkata »

azov wrote: post #42
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…

It takes time. For a team not used to code reviews, they might seem more trouble than they're worth at first. Most likely they will be more trouble than they're worth for the first few months. Keep doing them and eventually your smart developers will figure "if we have to do this anyway, we may as well find something useful to say" :) A few things you can do to make it smoother: - Manage expectations. Initially it ma…

> Set up your tooling to make the process smooth.

> Leverage automation. Run [..] linters, static checkers [..]

These don't make the process smooth unless you set them up to simply give a warning rather than block the build/merge. And with that they'll likely get ignored anyway.

I think linters/etc should be avoided until you already have buy-in from the team.

Re: Code reviews do find bugs

#133

Post by klooney »

patrick451 wrote: post #84
klooney wrote: post #44

Earlier quoted context omitted.

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

Or incompetent. If you knew this was going to break, why did you approve it? Your only defense is "there was a lot pressure to get into the release xyz". There's not much sympathy for that defense. The animal spirits that thought the broken feature were the most important thing ever are long gone, and frustrations about the new outage caused by the previously most important feature ever have taken over.

Don't approve it! That's actually a thing you can do.

Re: Code reviews do find bugs

#134

Post by shrimp_emoji »

01HNNWZ0MV43FF wrote: post #130
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…

What's the social atmosphere like? I ask because I had this one job, where the tech team was a few nerdy programmers in one office, before COVID, and a bunch of people in a friend group I wasn't part of, after COVID. By that I mean, before COVID it was common for the founder to take us out for lunch or tennis as like official team building time. I loved this because I'm a picky eater and it's hard for me to make frie…

Can juniors even be friends with seniors? I feel like it's a "professor-student"/"private-lieutenant" relationship.

I spend all day being mean in code reviews too, and I'm a relative junior compared to most of my team! >:] They do not see me as human because I am not human. I do not have their human emotions and concerns. My only concern is code. They still like and respect me though, it feels like!

Re: Code reviews do find bugs

#135

Post by azov »

Izkata wrote: post #132
azov wrote: post #42

Earlier quoted context omitted.

It takes time. For a team not used to code reviews, they might seem more trouble than they're worth at first. Most likely they will be more trouble than they're worth for the first few months. Keep doing them and eventually your smart developers will figure "if we have to do this anyway, we may as well find something useful to say" :) A few things you can do to make it smoother: - Manage expectations. Initially it ma…

> Set up your tooling to make the process smooth. > Leverage automation. Run [..] linters, static checkers [..] These don't make the process smooth unless you set them up to simply give a warning rather than block the build/merge. And with that they'll likely get ignored anyway. I think linters/etc should be avoided until you already have buy-in from the team.

It depends. If your codebase is already free of lint warnings - adding a blocking check to prevent new ones is no big deal. But if your blocking check means that everyone has to drop everything and spend a week fixing code - of course this won't be smooth.

PS. Also, it’s a good idea to have manual override for whatever autoblocks you set up. Most linters already come with this feature.

Re: Code reviews do find bugs

#136

Post by banish-m4 »

Eliminating bugs requires sustained, vigilant, holistic, overlapping approaches:

- Code reviews prior to acceptance of commits (Facebook does this)

- Refactoring crap that manages to get through

- Removing features

- Higher-order languages with less code

- Removal of tech debt

- More eyeballs

- Wiser engineers

- Dedicating more time to better engineering

- Coding guidelines that optimize for straightforward code while not being so strict as to hinder strategic exceptions

- Negative LoC as a KPI

Re: Code reviews do find bugs

#137

Post by dakiol »

The only thing I don’t like about code reviews are nitpick comments. Everyone has their own subjective way of writing code, if my code works and looks good enough, let it be.

Re: Code reviews do find bugs

#138

Post by dakiol »

nitwit005 wrote: post #47

> Developers spend six hours per week reviewing. This is a bit too much It's extremely difficult to adjust the time spent on reviews. The options are unattractive. Do you start blindly accepting changes when you hit the limit, or just stop and not let people merge code?

Why should one block merging code? The idea of someone spending hours working on some code changes being blocked by another individual that doesn’t allocate time for reviewing is alien to me. We are all professionals, let people merge their changes and make sure you find time to review them. If you can’t review them and they still need your approval, then approve.

Unless you don’t trust your colleagues. If that’s the case, then code review is doomed anyway

Re: Code reviews do find bugs

#139

Post by rty32 »

giancarlostoro wrote: post #117
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.

Well, I have a colleague who spends more time defending his code by giving a list of wrong reasons, rather than actually fixing them. Happened more than once.

I just move on. At some time he will realize how bad his code is.

Re: Code reviews do find bugs

#140

Post by JackMorgan »

dkdbejwi383 wrote: post #115
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 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.

I've been on full time pairing teams but noticed 2-5x more throughput because there's fewer meetings, code review, and surfing the web. It's exhausting because suddenly I'm actually writing code 7+ hours a day and that's really hard.

Every other team I've been on without pairing, writing code is at most 3 hours a day. The rest of the time just gets chewed up by other communication channels.

Post reply on HN