Hacker News new | ask | show | jobs
by msoad 3694 days ago
Code reviews can easily become a tool for people with huge egos to prove their smartness. I get code review comments for grammar of my comments or very small code style preferences that Google's anal style guide can't enforce (yet).

I like code reviews, don't get me wrong. But there should be a way to respond with "you just shut up, you're only trying to make yourself look smart".

It's all because higher up people mostly look at code review conversations this is happening. The other day I asked my peer how do I write this code? A or B? He said I don't care, A seems fine. Then in code review he commented it should be done in B way.

It's all politics.

7 comments

The easiest is just fix it, even though its annoying.

Slightly clearer comments isn't a bad thing? Why not fix it in the two seconds it takes to.

No one's going to repremanded or promoted on code reviews.

Im a senior dev(ops) and don't mind if I get code review full of really minor issues by junior devs try to prove themselves. Every little issue fixed, makes a it a little bit better.

And those same ones commit stuff like the LoC I'm looking at right now that returns the results of a ternary, and there's a fucking bang symbol in front of every boolean, topped off with a bang preceding the parens around that ternary (IOW, some of those bangs should cancel out, and aren't needed). I ask myself, "how did this escape code review?" and the answer is probably that the senior dev who wrote it thumped the junior dev reviewing it with something along the lines of "that's how real Java devs do it", or something. I dunno, just guessing based on personalities. Same dev that self-admits he hasn't written unit tests before, yet argues with me (who has written thousands) about how to write unit tests.

Though I wonder if what you describe isn't just plain ol' bike shedding. I've seen it plenty in code reviews. Super sharp dev who generally writes great code puts a commit up for review, and someone feels like they ought to have some input, and because the code is otherwise solid they pick on grammar.

There's a line to walk for sure. When I code review a line that looks like this:

  $foo=fn1($bar) &&$baz = fn2 ($qux)
I always send it back. Do I look pedantic? Probably, but to me consistency is important. It's about caring about what you're doing. If I see a line like this, I assume the person who wrote it doesn't pay a lot of attention to detail and I pay closer attention to the rest of their commit. I mean, if you can't be troubled to set your IDE preferences to the team/project's rules, what's that say about you?
Isn't that in part a linting issue?

I figure code reviews are for reviewing stuff that can't be corrected by a computer.

I always complain about code comments that aren't complete sentences. Why should comments be misspelled or grammatically incorrect?
I also do the same thing. This is could sent he read down the rabbit hole if the comments is misleading which can be caused by misspelling or incorrect grammatical errors. Especially important if the team does not share the same language (ie English, German, French, or Mandarin etc).
Taking the time and effort to comment on grammar or code style preferences is anything but attempting to prove smartness; it's stupid work that shouldn't be necessary if the one submitting the code for review paid more attention to those basics.

If you can't write a coherent sentence or consider a style guide 'anal', you have no place developing for a project that is serious enough to warrant code reviews.

Another thing, if someone submits a PR with basic errors like that, they distract from the things that should be looked for in a code review - bugs, implementation details, etc.

I don't really see what your peer did wrong. Sometimes you need to see the actual implementation to understand how different approaches work out. There are definitely plenty of times I've started implementing A, then realised it doesn't work out that well.
That sounds like a culture problem. You don't really want people who are out to prove how smart they are, versus people that are trying to help the other engineers they work with get better.
Those two things can be presented as the same thing