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

121–130 of 169 posts

Re: Code reviews do find bugs

#121

Post by mikepurvis »

majikandy wrote: post #91
jiggawatts wrote: post #80

Earlier quoted context omitted.

"There is no later." is my new mantra.

That reminds of codebases littered with Todos… where I like to Yoda it… do or do not, there is no todo.

A TODO is not inherently bad, but I think intent is important— how likely is it that someone will come back here purely with an intention to address that comment? If not likely, then the TODO will be taken up in the context of future refactoring and in that case it's a gift to the person eventually contemplating that work, helping them understand something about the code or context that you realised too late in the project to be able to act on it.

Re: Code reviews do find bugs

#122

Post by mewpmewp2 »

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…

We have these things as well, but usually people treat these are bureaucratic obstacles and don't actually perform the steps. E.g. template is ignored, and reviewer doesn't check out, just LGTM and good to go. Few people actually take a more serious look.

Re: Code reviews do find bugs

#123

Post by mewpmewp2 »

dkdbejwi383 wrote: post #113
anonymoushn wrote: post #111

Earlier quoted context omitted.

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

It sounds so good in theory, but in practice:

1. Frequently old code needs to be touched or refactored. Feature flag would not be enough.

2. Even feature flag itself can be a risky addition, and might affect existing customer usage.

Most of the time old code does need to be touched, there really aren't those perfect new isolated features, at least in my experience.

Re: Code reviews do find bugs

#124

Post by pjungwir »

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.

Indeed, if you read The Goal or The Phoenix Project, they call this "slack". There is a whole theory about why slack matters.

Re: Code reviews do find bugs

#125

Post by alex_lav »

sadops wrote: post #63
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.

Everyone that writes software writes bugs.

Re: Code reviews do find bugs

#126

Post by bobthepanda »

mewpmewp2 wrote: post #123
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

It sounds so good in theory, but in practice: 1. Frequently old code needs to be touched or refactored. Feature flag would not be enough. 2. Even feature flag itself can be a risky addition, and might affect existing customer usage. Most of the time old code does need to be touched, there really aren't those perfect new isolated features, at least in my experience.

If anything refactors should be behind feature flags *because* they are so disruptive.

Re: Code reviews do find bugs

#127

Post by 01HNNWZ0MV43FF »

zer00eyz wrote: post #77
lazyasciiart wrote: post #66

Earlier quoted context omitted.

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

The "chair" in question was 3 coat hangers and 2 frisbees... I have no idea how it held a human up. The "throw" was more of a shove and the thing went flying in my general direction. The only thing that chair was going to hurt was my feelings. It was far less scary than the office where I sat on an ammo shipment.

How courageous of you to do a scary job, not to protect other people from having to live hard lives, but so you can be a prick to them on bulletin boards. Maybe I should have done that lol

Re: Code reviews do find bugs

#128

Post by 01HNNWZ0MV43FF »

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.

It sounds like radical candor and I like it

Re: Code reviews do find bugs

#129

Post by jappgar »

dkdbejwi383 wrote: post #113
anonymoushn wrote: post #111

Earlier quoted context omitted.

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

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.

Re: Code reviews do find bugs

#130

Post by 01HNNWZ0MV43FF »

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…

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 friends, so if the company makes official initiatives, it's easier for me to fit in.

After COVID, the official initiatives weakened. The team was too big to take everyone out, and I didn't join the friend groups who naturally found ways to socialize.

In that new environment I no longer felt like an equal member of the team, I felt like an outsider who had authority on paper but didn't have any of the camaraderie needed to get things done and survive a work day.

Even though everyone repeatedly said I was respected and valued as the most senior programmer, I found it impossible to be a good teammate in that new environment, I felt like I was just spending all day being mean and nobody got a chance to see me as human. That was part of why I quit.

In that environment my code reviews sucked.

Now I'm at a remote company where once again it feels like everyone is equally non-social, and I'm just gonna ride that as far as it goes. If they get an office I'll probably cash out and go on vacation for a year

Edit: almost forgot, the other woman who was part of the original "nerdy programmer" team, ended up also burning out and quitting about the same time as I did. She also didn't really make friends in the new environment, and seems much happier pursuing her hobbies and taking it easy between jobs

Post reply on HN