Hacker News Reader

Comment by crunchlibrarian | Hacker News Reader

Most code reviews I've participated in have devolved into the senior/loudest person making sure that people know who they are and what everyone's place in the hierarchy is.

Even in reviews where the reviewee had some amazing code that was kind of groundbreaking in one area, still have to find something to gripe about because I have 20 years on this kid. This seems to be quite common in science/engineering, not just in code reviews.

I wish I could find that company where nobody gives a shit about their place in the hierarchy and everyone isn't constantly trying to gun for other people's jobs to make their linkedin profiles look more impressive. It's been a looooong time now, still searching.

View on HN

We dont have that problem where I work so it does exist. I've sort of experienced something similar myself when I've been doing reviews though - the feeling that you've got to find something to comment on. I've also been "star-struck" by getting reviewed by someone very senior or "famous" in the open source/tech world and basically blindly doing what they suggest.

The problem we do have though is huge queues for qualified reviewers and getting randomly allocated a reviewer somewhere on the other side of the planet so that even if they have minor comments (e.g. trivial typos in comments) you're looking at a 48 hour turnaround to get your code checked in sometimes.

I guess for the age/hierarchy thing, it could be possible to make the reviews double-blind via the tooling? I.e. reviewers dont know who they are reviewing, and the reviewee doesn't know who is reviewing them. Obviously once the code is committed then everything would become clear and transparent. Might help in larger orgs, although I dont doubt that it would be fairly easy to make an educated guess who is writing the code a lot of the time, and you'd be able to dig into git/whatever to see the details, but just hiding it in the review tool might go a long way to prevent unconscious biases. If someone has a real axe to grind is deliberately going out of their way to find out who wrote the code during a double-blind review so they can make special comments then you probably have a bigger problem.

This is one failure mode that definitely exists. Another failure mode is less experienced folks who believe all their code is perfect and take criticism personally.

I honestly don't know which failure mode is more common. I do know that I had the latter problem in spades when I was earlier in my career and that I'm really paranoid now about being that senior person on a power trip.

But if everyone is trying really hard to both act in good faith themselves and assume good faith in their coworkers, careful reviews are the best way I'm aware of for getting great code into the tree.

> still have to find something to gripe about because I have 20 years on this kid

I wish I had someone with decades more experience than me taking the time to find areas where my code could improve. Regardless of their motivation, this sounds like an amazing resource that you're lucky to have.

I think their motivation still matters quite a bit. Just because someone has 20+ years of experience doesn't mean they're actually leveraging any of that for their review. If they're nitpicking something unimportant and they're only doing it for political reasons, then in that act they are contributing negative value to the team, regardless of experience.

If someone is that much more experienced than you are, it can be hard for you to tell the difference between nitpicking and something that experience has taught them is more important than you realize. Even if it is just nitpicking, that still means your code has nits to pick.

I don't believe this. Every time I've seen subtle problems that experience taught me were more important than first appeared, I've been able to explain why. And so I put that explanation in the review.

Usually people complain to me about nitpicking for small things I thought should have been obvious and didn't require explanation. When you point out a word in a comment is misspelled, it shouldn't require an explanation about why proper spelling is important for readability and clear communication.

Code reviews are generally public and visible to the entire team and often even to other teams. While it's true that the reviewee may not be able to distinguish the two cases being considered, there may be other developers on the team or in the company who witness the review and can make the distinction. In fact, this can cause the "toxic reviewing" practice to spread to other experienced senior developers, who may see others with similar positions doing the same thing and then feel they must do it too in order to signal that they are, in fact, senior.

Interesting - I thought you would end the argument differently. In my experience additional visibility helps curb the unwanted behaviour because other senior devs step in and help fight it - but I guess that depends on the general culture in the company. If that doesn't happen you have bigger problems anyway.

I do believe that code review culture ends up being a reflection of general company culture. In a healthy culture, visibility tends to spread the positive part of what's observable, whereas in a toxic culture visibility tends to spread the negative and political parts of what's observable.

And there's definitely a circularity here. In a healthy culture, code reviews tend not to be overly political in the first place, so it would stand out like a sore thumb if someone was causing a lot of unnecessary drama with their reviews. Likewise, in a toxic culture, code reviews may go from healthy to toxic and dramatic because developers have been incentivized to compete with one another in counterproductive ways.

As an engineer, how do you address this sort of mutually-reinforcing jerkery? I expect that complaining to the manager will not be effective because the culprits are not explicitly hostile.

Easiest way? Complain, look for another job, quit. There is no way you can change the culture from bottom up. Best you can achieve is make a bubble with your close coworkers where you cover each other - but even that is temporary and can be broken by management anytime. Sorry. But there are many companies with better culture out there...

I've never been in a work environment where senior developers are so insecure about their seniority. How can a junior developer tell the difference between this scenario and one where senior developers are making good comments on code reviews that junior developers don't see the value of?

I think the word "gripe" was meant to imply criticism that isn't constructive and has no actual value in improving the end product.

Let's say you send code out for review that is just about perfect how it is. The reviewer is faced with a choice. If they don't suggest any changes, they may feel it gives the impression that you are just good a coder as they are, that their input isn't needed, or that they didn't bother to read it and just rubber stamped it. All of these challenge their place in the organization as a technical thought leader and influencer.

Some people have the maturity and awareness to just say, "Great job. I see no reason to change any of this. Approved." Some people don't, so they will dig for something negative to say. Even if it is just "these variable names are excessively long, and it's distracting to read" or "you don't have enough parameters to warrant the use of the Builder pattern here, so take it out".

Yes. There shouldn't be any reviewee except the code itself. But this is hard to get right.

One of the questions I try to ask myself with any comment I write as a reviewer: "Would I accept this comment?".

Author here, I think that’s a great approach. We surface our guidelines for every PR by including a link to them in the description template. This acts as a reminder for people to be thoughtful about their review. I think the fact that our guidelines were based on input from the most senior to the most junior engineers says a lot about how much we value everyone’s opinions and perspective, not just the loudest voices.

Not sure why this is being down voted. This is exactly the number one thing I have experienced happening with code reviews, especially peer reviews.

This attitude often completely breaks the effectiveness of the whole process, making it a deal breaker for quick iterations and agile projects.

Maybe because many people haven't experienced this? The parent poster also said this:

> I wish I could find that company where nobody gives a shit about their place in the hierarchy and everyone isn't constantly trying to gun for other people's jobs to make their linkedin profiles look more impressive.

This does not resonate at all.

So "I haven't experienced this before therefore it doesn't exist. Have a downvote :)"?

I kind of agree that the parent poster's tone could have been chosen with a bit more care though.

OP is implying that this problem exists everywhere. This is simply not the case.

Agreeing on that.

But oh look, it's no longer being downvoted. Perhaps there's something to this after all?

>Maybe because many people haven't experienced this?

Oh god I hope not. What better way to create an echo chamber than to silence the voices of everyone who has different experiences than you...

I didn't downvote the comment, but maybe some people think it lacks perspective and concocts theories for why criticism of their code is actually the fault of reviewers rather than taking it as a learning experience. But I didn't downvote it because who knows, maybe the commenter has actually had a bunch of bad faith reviewers.

I agree, this is a very good point being raised. Code reviews should be geared towards the success of the team, not people's egos.

We recently instituted a code review process at my company. We're small, 4 engineers on the core system, plus our CTO. All 5 of us review every commit now. Personally, I take issue with any bug that makes it into production, especially if its mine. We inheritted a code base that was written by cheap off shore contractors and it shows. We're working on cleaning it up and improving it, but it's slow going with a small team and tons of new requirements. I personally just committed changes to +600 files...we're still reviewing after a week.

They definitely exist (I work at such a place) so I wish you the best of luck in your search!

We have a reasonable way of dealing with this

- bugs found in code reviews MUST be fixed

- coding style violations MUST be fixed (ideally all of these are caught by lint but not all are)

- everything else is treated as optional

Some code review tools even provide the ability to specify required vs. optional feedback. We now use github reviews which doesn't have this feature so we usually just add OPTIONAL to any review comment that isn't in the MUST fix category.

Occasionally a reviewer and reviewee will get into discussion/disagreement over the approach a particular code change. In those cases if it isn't clearly a bug or a style violation we err on the side of the reviewee.

My current company doesn’t have that problem (at least in my team) but I’m sure some teams do. I think this is everywhere and depends on team culture.

I have seen that in a lot of places also. There is that need for the reviewer to prove himself by nitpicking non-issues in the code.

If you're in NYC or SF apply to foursquare. We're not perfect, but we have a nice culture that is engineer driven and is very egalitarian.

Reply on news.ycombinator.com