Comment by TheCapn | Hacker News Reader

Comment by TheCapn | Hacker News Reader

Parent Full thread

I'm kind of shocked at how many recommendations are merely, "have tact and don't be a dick to your coworkers." How many code reviews are shitslinging mudfests where you guys work??

Now admittedly I mostly work on projects where I'm the sole developer so structured code reviews aren't part of my daily routine, but during the period where I managed interns we'd do code reviews before every merge which mostly involved sitting down and going through the functions. I'd point out deficiencies and work to teach the student developers new tools/libraries/resources they weren't aware of. At no point did we exchange shit like "This change sucks", "Can’t you see that this is obviously wrong?", or "This is stupid code written by a stupid person."... all examples from an article which is also toting the "Avoid hyperbole" mantra. Even a single one of those snide remarks would be grounds for discipline or potentially termination if it were consistent.

My tip? Separate your ego from your code. If you can't discuss criticism objectively and professionally you shouldn't be involved in a professional career. Hostile workplaces are shit to be part of and I will never stand for that among my peers.

After 2 years of developing at a scrappy design&dev shop, I got a new job that was a big step up at a hip SAAS company. I was in my mid-20s and I still hadn't worked through a lot of my anxiety issues, and I started getting code review from these two intimidating brothers-- one an engineering manager and one a principal dev. Their review style was very terse, and they called out _every instance_ of a syntax 'mistake' I made, and it just made me feel like absolute shit.

It wasn't just me, several of my coworkers felt the same about just getting hammered on by them.

This has kind of permanently changed my approach to code review. I always call out the things I like in a batch of code. I acknowledge that there are some code styles preferences that are arbitrary, but that I have a preference in and it's good to have a consistent codebase. If the person is working for me, or if a coworker took a nasty task that nobody wanted, I thank them for the code with the LGTM.

This sounds like a case where having an agreed-upon code style can remove a lot of conflict. Even if you personally don't prefer a particular way of doing things, if it's in the style guide, then there shouldn't be any argument.

Same things go for automatic code-formatters. If every file has to be formatted according to the canonical formatter, then you remove tons of chances for nitpicking. Your code doesn't match what the formatter outputs? You can't send your code out for review.

That way it's not personal; you're not saying "my way is best", you're saying, "this is OUR way".

There are probably a fair amount of code reviews where constructive feedback is needed but not given because the reviewer adhered to the old addage "If you don't have anything nice to say, don't say anything at all."

Tact takes effort and discipline, just like programming. To draw an analogy, maybe it's like when a dev foresees a bug but can't figure a good way to address it (or isn't motivated to address it, or doesn't have the confidence, isn't fully convinced it's really an issue, &c) decides to turn a blind eye and figures it'll get resolved further down the line if it becomes a serious problem.

Additionally, I think github can do more harm than good here.

There is so much code-review you can do before it hits a PR. But then the PR invites nitpicking, over-thinking, and mistaken assumptions. The PR was good when it was submitted but the consensus has all manner of tweaks that are, most of the time, stylistic. Only rarely do you get a serious issue and it gets drowned out in the noise.

This isn’t separating ego from code, it’s accepting that maybe you can do peer review on your code before the fact and any requirement to approve/comment after that must offer something more insightful.

also remember that the person doing the harsh reviews is probably suffering from developer defensiveness and anxiety too. A strong attack can be cover for a weak defence.

I heard this from a therapist: "everyone is just trying to feel safe and loved". It really helps me understand and empathise with people in conflict situations ("how is this action helping this person to feel safe and loved?"). It might help with understanding the code reviews too ;)

You actually bring up a question I've wondered about for along time. To most people here does "code review" mean a synchronous meeting (either in person around a conference room, or remotely via screen share) where you talk out loud about the code? Or does it just mean setting up "review/approval" requirements for pull requests in your GitHub/GitLab/BitBucket instance?

I have participated in multiple types.

1:1 code review in person, sort of like pairing at that point. 1:1 remote via PR, 1:N in person in front of a screen or projector, 1:N remote where you need multiple approvals and different sections may interest different people, and N:M where a small team worked on a feature and request another teams review.

I think of review broadly as the remote/pr kind. But all of these are valuable in different situations.

My favorite is N:M since it gets the most eyes on a problem. Hard to scale without slowing down the process (eg. standards bodies).

Yeah, several of those 'bad' comments are completely inappropriate. I can't imagine how the author even thought of those in the first place.

Some classic rants and raves of the recently trying-to-do-better Linus Torvalds come to mind!

Reply on news.ycombinator.com