This is an anonymous post sent in by a member who does not want their name disclosed. Please be thoughtful with your responses, as these are usuall...
For further actions, you may consider blocking this person and/or reporting abuse
Thank you for your detailed response
Give constructive feedback, like: „Did you think of doing this way? This could be more… . Is there a specific reason why you chose x over y?“
Not just: „This is bad. We should not do it this way.“
Stay neutral in tone.
As usual, any detailed answers will have 'it depends' at the start... 😁
That said:
The above assumes there has already been much automation to help with syntax / code style and such like applied before human review!
Not getting personal and not getting lost in the details.
It depends largely on the guidelines you have.
If there are guidelines saying, you should always use early returns and you need good reasons for not to, you should mark it as a problem when reviewing.
But if you don't have such guidelines, many things come down to taste. If you see a potential bug or edge-case, tell people, otherwise, leave their code as it is.
The most important thing of all is to remember that it is not a competitive sport, it is a collaborative process. Be polite and kind in your collaboration. If you can't express your point politely and kindly then you have nothing worthwhile to say.
Use it to point out excellence as well as areas for improvement - the person upon whose code you are commenting will not be the only reader, your other colleagues will be too, and especially good practice should be pointed out to them.
I guess the first point is: getting used to the existing guidelines! You should be basing any PR review on those to avoid personal views and, if they do not exist, question Senior Devs about it and write them down, maybe even document those guidelines yourself (and ask for their approval when done).
Other than that it's basically looking for business logic inconsistencies (which you'll only know if you know the context of the changes) or more wide topics, such as security and performance.
I would recommend this post and it’s second part. This is really helpful.
Awesome Read, Thanks for posting it here.
Assert good intentions and that the author was trying their best :)
It is important to separate facts from personal opinion for psychological safety. Maybe we need a template for review comments for that. I haven't been able to do it.