3 ms·
I think it depends on your level of seniority and the level of seniority of whom you're reviewing code for. It also depends on if the code is indisputably incor
by kypro 4y ago
I think it depends on your level of seniority and the level of seniority of whom you're reviewing code for. It also depends on if the code is indisputably incorrect or if there is some grey area.
We don't have any junior devs at my current place so I'm reviewing code from devs with similar experience as myself, therefore I ask more questions. "Could this be improved by doing x?", "Was there a reason you did x over y?", etc. Then when I'm done with the review I normally send a message to the reviewee just letting them know I'm done and happy to discuss if they have any questions or thoughts. Most of the time I find there is some reason why decisions were made and we can discuss that if we need to. Alternatively they'll agree with my suggestion and just go ahead with making the changes.
The reason I like questions is because I'm not perfect and the person who wrote the code spent a lot more time thinking about it than I have. As a reviewer of another senior dev's code I see myself as a fresh set of eyes, not as someone with more ability or authority.
Some obvious things like typos or formatting issues I'll just note without a question. "Typo here", etc. There's really no discussion to be had in those cases. What I wouldn't do is say something like, "Typo here. Fix this please". The "fix this" is unnecessary and comes across as a bit rude imo. I know when my code is being reviewed I just want to be shown what I've done wrong / what I can improve. I don't want orders. If I've made a mistake I'll fix it and I trust others will do the same. I think that's generally the kind of mutual respect experienced devs expect from each other.
With junior devs there's room to be more authoritative, although tone is important. When I'm working with a junior dev and I think they've gone about something the wrong way I prefer to just have a chat with them and explain how I think they could have done something better. I think it's good to make it clear that you're not trying to be difficult, but you know there's a better way to do something and you want to help guide them. I know when I've been a junior dev on projects I've received some overly critical and demoralising code reviews. Realising the thing you've spent the last week thinking about and working on is wrong isn't a fun experience as it is, but a review with an overly critical and unconstructive tone can make it even worse.
That's just my thoughts anyway. What's right in your case likely to depend on the context of whos code you're reviewing and what you're reviewing. The only constants I'd suggest are in tone. If you're not being perceived as being respectfully constructive in your code reviews you're probably doing something wrong.