4 ms·
Great points. Regarding point 2: > 2. The reviewer concedes that there can be equally valid approaches to a given problem or taste wrt. aesthetics. They do not
by falsedan 8y ago
Great points. Regarding point 2:
> 2. The reviewer concedes that there can be equally valid approaches to a given problem or taste wrt. aesthetics. They do not try to gratuitously force their style upon the reviewee. The reciprocal must hold as well.
If my code is being reviewed, and the reviewer has opinions about how they would do it, I like it when they share their preference but also
1. consider if the code as submitted has the same result
2. share their personal preference for how they would have written it
3. explain why they prefer it over the code I wrote
4. not block the review on me rewriting the code to match their preference
I think it’s fine (and valuable) to comment “I hate this code, shipit” or (better) “ship it, if you change foo to bar then <explaination of why they prefer it>”
The worst code review comments are “this isn’t performant/idiomatic/maintainable, fix it” or other statements of opinion presented as facts. This kind of faux-objective feedback is terrible, since a fact can be argued against (or could be wrong). A statement of opinion like “I don’t like it” or (best) “I had to think really hard before I understood it” is great, since that’s what someone diving into the code for a maintenance ticket/incident will be thinking but won’t have the original author on hand to walk them through the code.
Linking to articles or references that explain why you prefer a particular implementation also gives the reviewer a learning opportunity & lets them save face when they post a follow up CR “thanks, didn’t know that”.
- pytester 8y ago>The worst code review comments are “this isn’t performant/idiomatic/maintainable, fix it” or other statements of opinion presented as facts. This behavior is usually symptomatic of an excessively dogmatic outlook. That is, I don't think it's the review style per se that's the problem - it's the person. I find developers like this nearly impossible to work with. They won't just unknowingly write poor code themselves (dogmatism typically leads to bad decisions which leads to really poor code) they will likely try and force you to do the same.
- danmaz74 8y agoEven dogmatic people can learn not to be dogmatic.
- kochthesecond 8y agoA few comments. Very rarely I get stuff that is so badly made, or naively implemented that I just have to say no, then go help the person redesign the solution. This happens rarely though, maybe twice a year with a fresh employee. If the code is not formatted to the linter, it should not have been submitted for review and I will probably reject it. We have to hold our work to some kind of standard. Otherwise I do the "I did not understand this, can you please explain?" Quite a bit if a part is confusing. I try to invite to a discussion, not demand doing it to my taste and make it clear a lot of the comments are suggestions for future work.
- dvlsg 8y ago> If the code is not formatted to the linter, it should not have been submitted for review and I will probably reject it. Better yet, make a tool (CI) automatically reject it. That way it doesn't waste your time, and no one has to be the "bad guy". Not until developers start disabling the linter with inline comments, anyways.
- beatgammit 8y agoI try to never block correct code. I can usually find a few things that should definitely be changed (needs more comments, delete unreachable code, remove unnecessary/unhelpful extra variables, etc), so everything else is suggestion. Usually the submitter takes my advice, but sometimes I am educated in the process (I usually ask for better comments in this case).