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

[dkdbejwi383]

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

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