91–100 of 309 posts
A bit related - do you ever go from PR comments to DMs? Sometimes when discussions are needed, it feels much easier to just chat 1 on 1. Especially if it's a small team and everyone's close. Though that prevents others from experiencing the "interaction".
There's basically two categories here: - you have an opinion on how things ought to be done and want a dialog - you see some code that's wrong or violates an agreed-upon rule and so it should be changed with no discussion I'd switch the tone based on what you're addressing. Giving a rationale when you share an opinion or point out a mistake also softens the tone (for the better IMO). >* Should we extract this to a se…
This sounds really bad. Even if you’re their boss. You’re dealing with professionals and part of that is considering their input.
I’d start with the assumption that they know what they’re doing and might have had a good reason and phrase it from there. Even if not true it sets the right tone of respect.
Review: a formal assessment or examination of something with the possibility or intention of instituting change if necessary. None of your examples provide feedback as to why you want a change. That may be why you have been led to ask this question. Consider: * There is a potential overflow in this code. The library function xyz already does this and can log when the app is in debugging mode. * This portion duplicate…
If you're fluent in English, there's a clear difference in the tone of the listed comment forms. If you're working with colleagues that may struggle a little with English, you can easily read to much into the tone of a code review. The reviewer may not mean to come of direct or even aggressive, but because they literally just translated directly from German or Finnish the tone will be way off.
It's also up to the author of the code to assume good faith, and look beyond the language used. Try to be kind when reviewing the code of others, and assume that the reviewers is trying to help.
To a senior dev they might not need an explanation to justify the action of extracting the coded. It could just be oversight.
But to junior and intermediate you might need to explain the reasoning for requesting code to be abstracted.
Unless you're asking a question, posing it as a question comes off as passive-aggressive, even if it's not what you intended. A simple declarative statement of, "I would do it this way", is probably best. You don't know why he did it the other way, and he might have had a very good reason (for example, to be sure the subroutine would be inlined).
The best tone to use is one that is neutral and efficient; prefer active voice but avoid imperative mood unless you're 100% sure you're right, in which case it's usually fine.
It probably goes without saying, but you should assume the change was made in good faith. It's possible that the other programmer hasn't seen before what you consider to be good practices; it's also possible that she's great at her job but just made a mistake. It's also possible that you're wrong. So, for this reason, it's always best to be conservative with tone and stay as factual as you can.
Review: a formal assessment or examination of something with the possibility or intention of instituting change if necessary. None of your examples provide feedback as to why you want a change. That may be why you have been led to ask this question. Consider: * There is a potential overflow in this code. The library function xyz already does this and can log when the app is in debugging mode. * This portion duplicate…
These example notes are wonderful. They feel like an editor's notes, not a graded exam. Something you'd get from a colleague who is collaborating with you and not some infallible black-box oracle.
Review: a formal assessment or examination of something with the possibility or intention of instituting change if necessary. None of your examples provide feedback as to why you want a change. That may be why you have been led to ask this question. Consider: * There is a potential overflow in this code. The library function xyz already does this and can log when the app is in debugging mode. * This portion duplicate…
Great post! One quibble with your last point — your guide is probably most helpful to the devs that are too direct and blunt, and expect others to be that way to them.