Hacker News Reader

Comment by wickerman | Hacker News Reader

First rule about code reviewing is that you don't tell someone how you would've written it, but rather try to point out pros/cons of current vs proposed approach. It's as easy as that. Every time I see a piece of code I don't like I ask myself: why do I not like it?

I've gotten PR reviews with people literally writing "I don't like this" even after I wrote a very detailed technical argument about my reasoning. My only approach to that is to remind them that I really don't care about their personal preferences, because no discussion will come out of them addressing none of my technical arguments and instead relying on what their personal preference is like.

View on HN

Agreed. In these cases, the reviewer should always aim for learning as much as teaching (if not more so).

I've gone into code reviews where my reaction began with, "ugh, this is terrible", but evolved into, "well, I probably would have done it differently, but this approach handles an edge case I hadn't considered".

Completely agree with all of this. Some say be hard with the code, soft with the coder. Also don't nitpick and be nice. Simple.

In cases like this, I explicitly preface it with "nit: <text here>", explain how I would do it myself instead and why, and then (also explicitly) say that this is more of a preference thing and that they should feel free to leave it as is. It might turn out they like my approach more and change the code, but if not, it is still totally cool with me, as they learned of just one more way to do the same thing that might come useful later.

Same. I've heard it all. From "terrible", "You don't know what you're doing", "does it work?", to "ok, I see why you did it so try this way instead because X". It's honestly who you are working with.

This sounds similar to one of the best exercises I ever had during a sculpture degree: write about a piece of work I didn't like, and explain why I didn't like it. I ended up liking quite a few of them after that, and those that I still didn't like, I didn't feel the kind of animosity towards them that I did before. I found both of those pleasantly surprising outcomes.

This advice and the other commentary agreeing with it is surprising to me. Showing how you would have written it seems like an extremely useful part of concretely explaining why you don't like it. I agree that you also need to do the part where you explain why.

I almost do the opposite of this advice -- refrain from criticizing unless I can show precisely how I would prefer to do it and why I think my way is better.

My point is that, if there's a specific reason why the code needs to be rewritten (i.e. code needs to be clearer, or more performant, or adhere to coding standards) then you can point out what's wrong with it with a simple explanation. If you just write the code "as it's meant to be" then the other person a) will not get why their code is being rejected and b) you'll be writing the code for them.

Code reviews are not coding exercises. You ought to be able to explain using human language primarily. The only only instances where I've actually written code in my comments was because I was reviewing Scala code written by a very old person who had worked 20+ years in Java and it was very very difficult to get some points across - and only after several tries where he didn't get what I meant.

That way you learn something, even when you're right.

Reply on news.ycombinator.com