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

141–150 of 169 posts

Re: Code reviews do find bugs

#141

Post by ahepp »

bobthepanda wrote: post #90
ecshafer wrote: post #27

Earlier quoted context omitted.

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…

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?

Re: Code reviews do find bugs

#142

Post by OpenDrapery »

I like to see working software alongside a PR. GitLab has the concept of a Review Environment, and it spoiled me. Ephemeral, dynamically provisioned environments that are deployed from a feature branch, are absolutely amazing productivity boosts.

It gives so much more context to a PR.

Re: Code reviews do find bugs

#143

Post by 01HNNWZ0MV43FF »

shrimp_emoji wrote: post #134
01HNNWZ0MV43FF wrote: post #130

Earlier quoted context omitted.

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!

I guess I didn't like being the only lieutenant then lol

It felt like I had authority on paper but not in effect. And nothing was organized and nobody wrote anything down...

Re: Code reviews do find bugs

#144

Post by ElevenLathe »

azov wrote: post #135
Izkata wrote: post #132

Earlier quoted context omitted.

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

IME even better than linters is something like `go fmt` (I work mainly in Python and use https://github.com/psf/black). Rather than forcing you to manually jimmy the code to satisfy the robot, just let the robot make it look the way it wants.

Re: Code reviews do find bugs

#145

Post by moritzwarhier »

anonymoushn wrote: post #103
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…

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?

Ship of theseus in practise.

Kind of.

Atomic commits are hard.

Re: Code reviews do find bugs

#146

Post by bobthepanda »

moritzwarhier wrote: post #145
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?

Ship of theseus in practise. Kind of. Atomic commits are hard.

This is so common that it is an architecture pattern: https://learn.microsoft.com/en-us/azure/architecture/pattern...

The original post describing it: https://martinfowler.com/bliki/StranglerFigApplication.html

Re: Code reviews do find bugs

#147

Post by seadan83 »

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…

> A vicious alternative is to make using side channels harder

If people are looking to side channels, that is a signal. Ignoring that, ignoring that people are trying to find ways to do their jobs effectively - I think seems to be missing the point.

> If some team members already have experience with code reviews - adjust their workload so that they can do more reviews. They are effectively training others by example.

This can backfire. Suddenly it's just a few (potentially just one or two) team members doing all of the code reviews. They feel pressure to not be the blocking part of shipping, their reviews are then done hastily. They LGTM rubber stamp stuff from the other seniors and probably punt on reviewing the other reviews until the next day. They struggle to get their own work done, 2 hours a day code reviewing is a huge impact (if reviewing work from 2 other developers, who have been jamming out code for 6, 8, 10 hours in one day - it will take 2 hours to review that the next day).

> - Make sure that code changes under review are reasonably sized. Encourage submitting changes for review early and often. "Here is the feature I worked on for 3 months, and it's due on Friday, please review" won't make anybody happy.

On the other side of the coin, "Hey, review this really quickly so I can give you a series of 4 more changes in the next two hours, before finally sending the update that does the thing."

Re: Code reviews do find bugs

#148

Post by seadan83 »

spankalee wrote: post #15
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…

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

People are smart - how about listen to their concerns? Do that before hitting them over the head with a list of 'best practices' and 'desired outcomes'.

I'm starting to come to the opinion that nits are entirely and outright counterproductive in code reviews.

Since I learned about "Ship/Show/Ask" - it's drastically changed my view of code reviews: https://martinfowler.com/articles/ship-show-ask.html; I no longer believe reviewing every change is healthy, seeing the difference of other models I can tell it's actively harmful now. Another good approach IMO is allowing for post-merge reviews, 'ship/show/ask' is better, but why is it written in stone that a code review MUST happen before merge?

Re: Code reviews do find bugs

#149

Post by seadan83 »

andirk wrote: post #52
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…

Please continue to be diligent in your PRs! Garbage code belongs in the garbage.

Plenty of garbage code is making billions for all of the big tech companies that exist. It's mostly garbage code at Amazon, Facebook, etc.. Let alone the smaller companies that were "bootstrapped", had founder level code be built and then are in that constant "fixing and scaling" phase.

More though, what kind of culture develops when someone works on something - and then it's called garbage? On the other hand, when someone is looking at their rate of progress, and they make a tactical decision that perhaps something is fine, not great, but it's f'in fine - and they are going to spend the time saved getting the project done; and then what happens when the reviewer decides the PR is "garbage"?

I've also come to learn that I need to be more intentional about letting various fires burn. Can't fix everything, and sometimes simple & low quality is best.

Re: Code reviews do find bugs

#150

Post by seadan83 »

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…

Some work cultures are too conflict adverse. I think it comes down to personalities of course, I want to generalize it to West coast people being conflict adverse. In these examples, it's all the ra-ra cheerleading you'd find on Linked-In, everything is awesome and super fast shipping squirrel

In these situations I find conflict is buried rather than resolved in a calm & understanding way. Without the latter being the example, the incentives are to go along to get along - meanwhile there is no longer a mechanism to resolve a disagreement. Suddenly then in review, you're "that guy" that is delaying things, and the both reviewer and reviewee don't have a healthy culture to talk through the issues.

Sorry for the rant, I think the downvotes are a reaction to the shock that some teams can have healthy conflict and work through them - and it can look like yelling sometimes (often yelling is just that, yelling.. but when a team is actual friends with another - they will have their own unique ways to address conflict).

Post reply on HN