[willio58]

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 issues you find.

Knock on wood but working at the company I do now where I, along with my team, have made quality PR reviews normal.. our codebase is now enjoyable and fun to work on. I highly recommend it!

One key aspect is being “kind, not nice”. Be helpful when leaving comments in PRs, but don’t be nice for the sake of avoiding conflict.

Also if you find code reviews to be a waste of time I can reccomend one thing I do often - give warnings. I approve and give comments around things I’d like to be fixed in the future for similar PRs. I don’t hold up the merge for little things, but at the same time I won’t let the little things slide forever

[mikepurvis]

"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, and there can't be any further development in this direction until X, Y, and Z have been rewritten to be in like with ABC.

I find it profoundly hard to get buy-in for those types of discussions. Everyone is happy to smile and nod and the appropriate tickets are filed with deadlines attached, but then the next release rolls around and there's new business-imperative stuff that's the focus and the cleanup tickets are quietly moved to backlog with the deadlines removed.

Seeing this repeated over a number of years has left me with kind of a cynicism about the process, where it feels like code review is at least partly an exercise in frustration; I don't have the backing required to insist on doing it right upfront, so instead I'm really just getting a preview of what is going to land in my lap a year or two from now.

[hakunin]

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, then the best time to clean up new code is during or immediately after writing it. A phrase I like to use is "while it's still fresh in memory". You're saving time and not adding new bugs, by not having to remember everything again, load all that context back into your head.

[godelski]

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

[pjungwir]

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

[BobaFloutist]

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.

[Viliam1234]

> Everyone is happy to smile and nod and the appropriate tickets are filed with deadlines attached

Once I did a final review of a backlog of a project that got decommissioned after a decade of development and use. Most of the "fix tech debt" tickets were still there in the backlog. And how could it be otherwise if the tech debt tickets always got assigned priority 2, and everything the management wanted done got assigned priority 1, and there were always more priority 1 tasks than we could fit into a spring?

Next time, I will have no confusion about what "priority 2" actually means. It's in the backlog to make you feel happy, but you are not supposed to actually ever do it. (If by some miracle all current "priority 1" tickets get done at some moment, some of your team members will be assigned to a different project.)

[klooney]

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

[pavel_lishin]

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.

[patrick451]

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.

[klooney]

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

[mikepurvis]

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

[jiggawatts]

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

[majikandy]

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

[mikepurvis]

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.

[servbot]

I have to favorite mantras related to this:

There is nothing more permanent than temporary.

... and ...

We don't have time to do it right, but we do have time to do it twice.

[jellyfishbeaver]

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 amount of effort for theirs.

- People blindly accept suggestions with no resistance or discussion to get the review over with.

- People send their MRs to side channels or other teams to get their changes merged in without resistance or back and forth. (I've had to revert several of these).

[ecshafer]

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 expected to get 2 reviewers from our team to okay.

2. Depending on the code project, we have to fill out a template for the PR, what is in it, what it changes, what to look for when we test the code, etc.

3. Some areas have code owners that might require an additional review from a specific team.

4. We are expected to check out, and test branches when we review them. So a quick read and LGTM is really discouraged outside of a few small cases.

I have seen a lot of places that do the blind PR acceptance, and its tough because without this really being enforced and encouraged that culture is hard to change.

[bobthepanda]

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 faster than if it had been one giant PR.

[anonymoushn]

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?

[withinboredom]

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

[anonymoushn]

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.

[moritzwarhier]

Ship of theseus in practise.

Kind of.

Atomic commits are hard.

[bobthepanda]

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

[ahepp]

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?

[bobthepanda]

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.

[mewpmewp2]

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.

[xmprt]

Culture for code reviews doesn't start out of thin air. Unless you have processes for CI/CD, testing, task estimation, retrospectives, incident postmortems, etc., there's never going to be a point where you will convince people that they're helpful. So start with those.

There's always going to be pushback from adding more process, but if there's an understanding amongst the team that keeping things working is P0 then these processes will slowly/naturally come up as the team realizes that investing in them proactively will save them time down the road.

[azov]

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 may be as simple as "we just want to have a second pair of eyes on every change" or "be aware what other team members are up to" - i.e. communication first, improving code second.

- Set up your tooling to make the process smooth. If somebody wants to just get it over with - it should be easier for them to use your official review process then to use some side channel. A vicious alternative is to make using side channels harder ;)

- Leverage automation. Run tests, linters, static checkers, etc. on your PRs so that developers get something useful even if no human leaves interesting comments.

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

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

- Make it less intimidating. Code reviews are not just for finding bugs and flaws, encourage reviewers to say positive things as well.

[majikandy]

This is quite good advice, I feel the “we have to do this anyway” line is more like… “so we might as well make it easy for ourselves”… eg write code that works, you self tested it through tests and manual if needed, so the reviewer doesn’t have to get bogged down in actually running it (start by adding screenshots for this but graduate to not needing them). Keep PRs as small as possible, aka multiple PRs streaming after each other for a single feature card, get the PRs in as soon as valuable and don’t block for nitpics but the shared expectation you start to agree on things that are better and they happen with the next changes.

The general mantra being that “if it works then it shouldn’t be blocked” and developer can choose to improve the maintainability there and then or delay it to next or later PRs at their discretion. After all you trust each other.

[Izkata]

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

[azov]

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.

[ElevenLathe]

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.

[seadan83]

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

[hakunin]

I've written up some rules that I've successfully applied in my teams. https://max.engineer/mindful-code-reviews

[spankalee]

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 from no-action-required to nits to blockers. Talk about the priority of reviews themselves. Reducing the latency of reviews has a huge positive effect, ime.

[seadan83]

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?

[01HNNWZ0MV43FF]

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

[shrimp_emoji]

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!

[01HNNWZ0MV43FF]

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

[godelski]

> The same people leave detailed comments on others' merge requests, but get discouraged when nobody else puts in the same amount of effort for theirs.

This is always a precarious situation. Because as soon as these people become jaded, your ability to make good PR culture will also vanish. And they can become jaded for many reasons. If these people are not explicitly or implicitly valued, they will know. If people who are doing the incorrect things are getting promoted first (or even at the same rate!), the same raises/bonuses, and on all accounts are treated equally, the employee will almost always converge to "well why am I putting in all this extra hard work if it's not benefiting me in any way?" And I don't think promises of early promotion or similar have a good effect because there's many employees who've had those promises made to them and it not follow through[0]. So there needs to be some, even if incredibly minor reward in the shorter term.

Also, do not underestimate the value of explicitly saying "good job." There's often a huge bias in communication where it is only made when something is wrong and when good work is done that it is left unsaid. You don't have to say it for everything, but I think you'll be surprised by how many people have never heard this from management.

[0] I wanted to share a story of an instance I had with this. I was a green (mechanical) engineer working at a startup. I had a physics degree instead of a ME, but have always been hands on. But because of this I was paid less and not valued as much. I asked my manager what I would need to do to get promoted and to be on par with everyone else. I got it in writing so I could refer back to it. At my next performance review I was just talked down to. Complaining about how I didn't do this or that (sometimes things that were impossible and sometimes they were weird like "your code may have been 20% faster but X couldn't understand it so we can't use it" -- X was a manager who had only been writing in C++ for < a year and I __heavily__ documented my use of functors). I asked about the things I did and the promises. They admitted I did all of them and even more. One of these being getting a contract (I think they put that there not expecting me to get it), and I was the only non-manager with one, bringing in 20% of company revenue while being the only person on that project. You can imagine I walked out of that meeting polishing up my resume and I was strictly a 9-to-5er doing the bare minimum from that point on. But the next manager I had, was liberal with complements and would critique instead of complain. Understood that there were unknown unknowns and all that and would actually tell me to go home when I was putting in overtime. I never worked harder in my life AND it was the happiest I had been. A manager can make or break an employee. And to part of this is that there may be ways to get back those broken employees, but you might need to figure out why they became broken in the first place. And if it is something you can fix or not. I believe environment has a big impact on employee attitudes and thus, efficiency/productivity. If passion is worth 10 IQ points, then happiness is at least a big factor in making an employee productive. Everyone can win because it isn't a zero sum game.

[locuscoeruleus]

Talk to people and be curious why they don't value code reviews.

[andirk]

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

[seadan83]

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.

[andirk]

I hear you but I have had to waste countless hours and endure increased stress because someone pushed code that is way more LOC than needed, difficult to read, no docs, against best practices, on and on. I have also dealt with chunks of garbage code that have worked for years and no need to modify it and I'm fine with that. Getting an MVP out the door or a Hack Day project is one thing. Literally not being able to write good code when the dev has plenty of time to due so because they lack the skillset and/or discipline to do so is not cool.

[Viliam1234]

I don't know the culture of your company, so I am just guessing here. In my experience, these are the most frequent obstacles to code reviews:

- The team is understaffed, there is not enough time to do things properly. I can write the code in a day, but it would take two days to make it really good (to do the code review carefully, to address the comments, to review the rewrites), and at the end I would get scolded because my measured productivity got too low. In other words, in theory the company wants code reviews, but in practice it rewards those who skip them, and punishes those who actually do them. Also, the more busy people are, the longer I probably need to wait until someone finally reviews my code.

- Big ego. Some people take a suggestion to improve their code as a personal offense; as if the other person was telling them "I am a better programmer and indeed a better person than you". (Often the same people are quite happy to give a lot of comments to their colleagues, some of them genuine improvements, many of them merely "I would have done it differently" except they insist that their way is always better.) If you have such people on your team, you are going to have problems. In my experience, about 10-20% of men are like that. (No idea about women; I would expect the fraction to be smaller, but I didn't have enough female colleagues to verify this.) I prefer teams where no one is like that, because one person like that can ruin the entire team's dynamic.

- Lack of experience. A junior developer reviewing another person's code sometimes simply doesn't know what is good and what is bad. "If the code compiles, it's probably good? Wow, there are even unit tests, and those pass too; what else is there to complain about?" But I think that even having the code reviewed by a junior is a good thing; at least this gives them an opportunity to ask why a particular approach was chosen.

- If you only have one person on a project, if someone working on a different project is supposed to review their code, they probably feel like "I don't know much about this project, I don't know what their constraints are, how the rest of the project looks like... I guess, unless I see an obvious horrible error, I will just approve it".

So basically it's either the ego or external pressure (or both). Depending on your position in the company, you may be unable to do anything about that.

Some people have an ego that you can't fix. You can avoid hiring them, but if they already are on the team and they otherwise do their job well... You just can give more attention to this in the future. For example, if you have a group of people who cooperate well, do not just randomly redistribute them to other projects once their project is over; instead maybe try giving a new project to the same group. When interviewing new team members, always ask for feedback from the existing team members.

Some companies create toxic environment by comparing developers against each other, sometimes with the explicit goal to fire the least performing 20% each year. In such environment, obviously the code reviews also become adversarial, and people will try to avoid having their code reviewed, and some will use the code review as an opportunity to harm their competitors. In a cooperative environment, code reviews work much better; it's people working together to achieve a common goal. To create a cooperative environment, you need to treat the team as a whole. (For example, never measure individual velocity, because people will obviously soon notice that helping their team members may hurt their personal metric. You don't want to fire a person just because they spend too much time helping their colleagues with their tasks.)

EDIT:

If you use some automatic system to highlight problems with code, it will probably hurt the ego less than a similar comment made by another human. Just make sure to configure the tool properly, for example to write comments but never actually block the commit, otherwise developers will get really angry about the false positives.

[zer00eyz]

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 your team know you have a stick?

> The same people leave detailed comments on others' merge requests

Call these people out, in public, for doing good work. Tell everyone they are setting the bar and others are not living up to it.

> People blindly accept suggestions

Coaching, lots of one on one coaching about finding and having a voice. Lots of "team building" where you level out the playing field with the strong vs weak voices. Figure out what those quiet ones excel at and do a fun activity around that. Let them find legs...

> People send their MRs to side channels or other teams

Stick. Harshly worded emails. Down dressing in public. Telling your team that in no uncertain terms that "this is unacceptable behavior"

As for the chair thrower... He was always fair, he always had his team first, I grew as a person, a manager and an engineer working for him. Its not growing happy go lucky good times while I get a pay check, its Growing pains, spreading that (pain) around is part of your job.

[lazyasciiart]

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

[zer00eyz]

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.

[lazyasciiart]

I'm shocked, shocked, that you actually were just lying about your experiences to make whatever point you wanted. The one time I worked with a guy who lied about throwing chairs at people it turned out he was a wanted serial killer, and the company almost went bankrupt. Of course none of that is true, but if it was, it would be a colorful way to back up my refusal to work with people like that!

[01HNNWZ0MV43FF]

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

[pavel_lishin]

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

[seadan83]

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

[danielmarkbruce]

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?

[bakje]

I’ve spoken with a CTO who was against them because they add too much overhead, partly because they’re too late in the process.

He encouraged his team to discuss an approach beforehand or to work on something together.

Other than that they had a lot of tests and a very structured codebase, I guess it worked for them.

[dkdbejwi383]

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.

[JackMorgan]

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.

[seadan83]

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.

[nickm12]

A number of these critiques seem like problems with the work culture rather than code reviews. Yes, nitpicking, cargo culting, gatekeeping, and favoritism are problems, but are they problems with _code reviews_ specifically?

Some of the others are actually desirable features of code reviews in my experience. Yes, we don't want needless explanations or the code reviews only going to a small number of people, but my experience is that code reviews are a good mechanism for encouraging authors to write self-explanatory code and for sharing expertise around the team.

This list just doesn't jibe with my experiences.

[danielmarkbruce]

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?

[seadan83]

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)

[giancarlostoro]

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

[rty32]

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.

[01HNNWZ0MV43FF]

It sounds like radical candor and I like it

[saulpw]

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.

[elcomet]

We prefix our comments with "minor:", and all employees know that this means it's something that would be nice but not necessary to merge

[seadan83]

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?

[audiodude]

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

[est31]

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.

[sfink]

Yes, even within a company my threshold for accepting a change can vary pretty widely depending on my experience and relationship with the author. For an external contributor or someone I've never collaborated with (by reviewing code or having my code reviewed), I don't accept the code until almost everything is worked out to my satisfaction. With someone I work with regularly, it's not uncommon to accept a change with a comment like "this is all good, but you need to take X into account which will change almost everything in this patch" (I exaggerate, but only slightly). I know whether an update could be problematic and whether it is necessary to see it again. Sometimes there are a couple of obvious ways that something could be done, they picked one but weren't tied to it if I had a reason for picking a different one, I picked a different one for $REASON.

Most are somewhere in between.

Though in some ways it works the other way around. For an unfamiliar open source contributor, I need to be confident that the change is worthwhile. I will be lenient on stylistic things, and I'll just land their patch and then fix it up afterwards. For someone I've worked with a bunch (whether a familiar contributor or a coworker), I will trust their opinion on the underlying quality of a change, but be less tolerant of unnecessary stylistic differences since they should have already come into alignment on those and it's more likely to be an oversight if they missed something. (Plus, I don't want to be fixing up their changes after the fact, given that >90% of patches will come from regular contributors.)

[seadan83]

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

[closeparen]

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

[fiddlerwoaroof]

Me too, I really dislike meetings that could have been handled by asking me three or four questions in slack or in the PR and then waiting fifteen minutes or so for me to answer.

[hyperadvanced]

It doesn’t need to be formal or very long. I personally enjoy a PR meeting where we can poke at the code and understand it over someone dumping 2,000 lines of code in my lap at lunch time and hoping to get their spaghetti to prod by dinner

[danielmarkbruce]

Livid? About a meeting to discuss work? Some comments in slack etc sound worse than intended, and people are aware of that and sometimes go out of their way to say it in a way to it's received as intended.

[willio58]

To be clear I make time to meet if the submitter wants to talk through things. I don’t require meeting on every PR. I meet as needed on PRs, pretty infrequently as people get up to speed

[seadan83]

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