[billycorben2]

Strange, I’m on a team rn with people who just approve everything.

I’m craving some constructive feedback.

[jrumbut]

I think part of the problem with the bullying described in the article and the "approve everything" approach starts with the framing of code reviews.

Instead of "please write this code, submit a pull request, and I'll tell you what you did wrong," I have recently been saying to new coders "take a shot at this problem, and when you've got something working let's get together and refine it, there is some context it will take time to learn so we'll probably need to make a lot of changes or even rewrite it before release."

That way they know what's coming (a lot of changes), they know why it isn't a sign they're inadequate, and it's more about working together than passing judgment.

It has also helped reduce the massive delays while junior developers agonize over making something perfect.

[lolc]

Hm, we've discussed doing more smaller concept reviews. Doing "first try" reviews sounds like a good alternative.

[mywittyname]

Ask for some time to walk through what you've done with someone.

I've been on the receiving end of enough toxic code reviews to develop an aversion to do them. So when asked to do them, I really just ask, did you test it?, do you want to demo it to me?, then approve it.

When people demo to me, I can see their pain points, because they always point them out. "Oh yeah, I was having trouble getting this to work, as you can see, it's still a little slow" and it gives them an opportunity to ask for advice or merely to vent.

So if you want good, honest feedback, a demo is the way to go. Honestly, it's hard to really grok what a bit of code is doing just by reading it. And who has time to pull code and run through it solo when doing a review?

[backoncemore]

The sweet spot is somewhere in the middle. Too laissez-faire and you end up with a ball of mud.

[marktangotango]

Super critical code review culture can also end up with balls of mud. The two are not exclusive. Example is a team I was on that would critique the hell out of syntax and grammar in javadocs, formatting of annotations arguments, stuff like that, fine. But never look outside the source file for how any of it fit together. No design planning cause "code reviews". End result was tons of duplicated code everywhere.

[H8crilA]

It might mean you're doing it basically correct, or it might mean you're touching parts that people don't care about much. The second is probably more common.

(It might also mean your team doesn't care about anything any more, in which case look for a better opportunity, unless you want to just cruise along for a while for whatever reason, in which case turn into a bare minimum performer like the rest of your team).

I'm a senior engineer in a known company and my target is always to leave no comments on a patch. I only give question-comments if I absolutely must know something before merging, or please-fix comments if something is definitely wrong and I can prove it. Also, if it's a part of the system that poses little risk (for example, if it breaks I can assign the bug back to the author and patiently wait for a fix with no serious damage) my tolerance for bugs greatly increases. The last part I'm not too proud of but frankly I don't have time to review everything. A "fix" for the last part would be to find a lower rank reviewer-buddy who has more time to share knowledge.

[innomatics]

Thank you! I agree with so much in this, it really strikes the right balance of clean code vs productivity.

One of my peeves is a review that points out a bug/incorrectness without offering proof, or suggesting the fix/correct way. The reviewer might observe a code smell, but it's so much better dealt with a question-comment, e.g. "did you try?" vs "this looks wrong".

I generally aim to make every comment a question ("can we..."), and not make assumptions that the author didn't think of something already. Unless I just stating an objective fact that is clearly unknown e.g. "this duplicates library function x...".

[JKCalhoun]

For two decades I wrote professionally and there were no code reviews at all. I had no problem with that.

[OneEyedRobot]

I did it for three decades (3.5?) and the only code reviews were with friends of mine who worked at multiple companies together. The whole point of the review was to find outright errors, not to optimize. Scarcely any ego involvement.

It worked out fine I think and, mind you, it used to be a lot harder to push out a fix. A lot harder.

[armchairhacker]

I think the issue is when a code review is “do this”. Especially when it’s small things like formatting, and I have to wait a day for the PR to be approved (or they catch another formatting issue).

It’s better when code reviews are suggestions, the reviewer doesn’t nitpick (or at least fixes that stuff themselves), and small things like formatting are enforced by a linter.

[onlyrealcuzzo]

Not to be rude - but... have you asked for it?

[billycorben2]

I don’t think this is rude.

I haven’t asked them. I should ask for more feedback.

I haven’t asked because:

I was a latecomer to the team and they had an established process

Cultural differences between myself and them

Honestly, I’m not sure all of them are experienced enough to give a useful critique

[raffraffraff]

I don't think it's something you should have to ask for. I mean if you went to a nightclub and the doorman let you in, would you say "Hey, aren't my shoes a little out of fashion?". To beat the analogy to death, I suppose if I was walking into a nightclub naked, on fire and carrying a bomb, I'd be a bit worried about the establishment of the doorman didn't refuse me.

So: is your code naked, on fire and carrying a bomb?

[smoldesu]

It's much easier to ask someone to be critical than it is to ask someone to go easy on you.

[mmmpop]

Maybe you just write incredible code the first time, every time? Can't discount the possibility!

[sdenton4]

IME, if the tests don't fail the first time you run the code, it says that the tests aren't actually hitting the new behavior. (Maaaaybe 1 in 100 times the code is actually right the first time.)

Likewise, if there's no comments on a pull request, it hasn't been read...

[billycorben2]

I don’t think this is me, haha

[some_furry]

Ask a security engineer.