How do you give real code review feedback without sounding bossy?
Posted by Ill_Captain_8031@reddit | ExperiencedDevs | View on Reddit | 165 comments
Lately l've been trying to level up my code review game. But wow, giving thoughtful, constructive feedback without sounding like I'm nitpicking or lecturing?
Backstory: junior dev on our team pushed a PR for a new service. Logic worked, but it had like... zero error handling and was missing some tracing. I thought for 20 minutes before finally writing something like:
“This works! One thing to maybe consider: what would happen if this call fails mid-request? Wondering if wrapping it in a retry + logging block might help.”
She replied:
“Oh no good catch, thanks!”
All good, but I still spiraled after. Am I being too nice and vague? Too nitpicky? Should I just rewrite the comment in code and push a suggestion?
So how do y'all give feedback that points out real risks / missing stuff, especially in production code, without sounding like you've got a god complex?
Bonus points if you've got templates, one-liners, or "feedback sandwich" tricks.
No_Interaction_5206@reddit
I’ll to the rescue, I take for ever to word things, now I’m just like says this, now say in a way that doesn’t sound bossy buts gets the point across (or more often I’m like say the same thing but make it so I don’t sound like a dick).
It is a game changer for me. Takes like 1/4 of the time and makes me feel better because once good enough it’s like you don’t sound like a dick — awe thanks llm, send.
Strus@reddit
I've never understood Americans that are afraid to be direct. Just write comments, if something should be done then write it should be done and that's it. If someone feelings will be hurt because they cannot disassociate their code from themselves then it's not really your problem. They will learn eventually.
It's just pain in the ass to tip-toe around everyone every time.
Also, there is no such thing as "too nitpicky". Best codebases I've worked in were the codebases were every single comment was a blocking comment.
ALAS_POOR_YORICK_LOL@reddit
Give compliments. Point out when you're nitpicking. Pick your battles.
official_business@reddit
I tend to be fairly terse in my feedback and I don't really leave much room for interpretation.
I really hate gently worded feedback as I find it vague and confusing. Just say what you have to say. Make your feedback clear so the recipient can easily understand what you want.
I'd just say
The thing is, this wording:
Makes it sound like you are uncertain what the correct solution is, when really I don't think you are uncertain.
I think you're worrying too much. I've only been accused of something like this once, and the guy was a sook to everyone else on the team. He didn't last long.
Organic_Battle_597@reddit
I just get direct and to the point, while trying to phrase things neutral when possible. I don't attack, and I try not to talk about everything as "you did this" or "you did that." But I refuse to approve a PR that contains code I don't want to maintain.
One thing we do is have strong team agreements. Then I can just point out that the code is missing things we've agreed in advance it must have.
FortuneIIIPick@reddit
Where I work no one uses comments in the GitHub PR itself. We just use Slack and chat with the person or maybe in a tiny team channel.
Xsiah@reddit
I wouldn't play question games personally. If you actually know the answer to the question - say it. A smart person will probably feel like you're being condescending, and a dumb person will answer your question wrong and then you've created another problem for yourself.
I would say: this works, but it needs [this additional thing for this additional reason.] And if this is my first time giving out this advice I would list some solutions on how it could be fixed. Subsequent times I would just say what the issue is in the same language as the time with the explanation so they would know to use one of the aforementioned solutions.
LickLickLigma@reddit
https://conventionalcomments.org/
This is what you're looking for. You're welcome.
Novel_Log_6876@reddit
Am I the only one who thinks this is way too complicated?
SokeiKodora@reddit
I literally pulled up the site on my own with the intention to share that very link here in the comments. Great to see it already recommended!
This is my go-to for formatting my comments.
Beyond that, I try to keep my wording collaborative instead of instructive. I will ask questions when I can, include praise where I can, and when possible sandwich praise and suggestions together.
I have one co-worker who recommends sticking to "we" wording instead of "I" wording. (I'm still not fully sold on that one, but it can't hurt to share multiple suggestions.)
Ultimately, I just try to keep in mind that our team are all adults, all professionals (that, is doing it for our jobs), and it's the professional thing to do to call out any concerns that we may have while reviewing code. It's not personal, and we're all human so at times wording may not come across exactly as we intend, and if we keep that collaborative culture in mind we can discuss it rationally. Or at the very least, I can choose to stay rational and not let things feel personal. (Kind of difficult to get others to do the same if they are in a more emotional state, but that's also just a matter of practice, repetition, and building team trust.)
CPSiegen@reddit
"We" wording is great. I use it in PRs, code comments, and emails. The work is so often collaborative, if even to a small degree, and ownership over things like code quality, milestone success or failures, and the team's impression on managers and owners is very much a shared resource. Code switching to being "a facet of the team" makes feedback more collaborative and makes you appear more humble and less defensive/evasive, imo.
daraeje7@reddit
What the heck, this is great. I have autism and have been warned before for coming off as too blunt. I love this
GeekRunner1@reddit
Saw this suggested in a post a while back and I’ve been referring to it since. It’s great!
Awric@reddit
Oh wow I love this
LedinKun@reddit
This is exactly it.
In the conversation mentioned by OP, it all went fine. OP raised a point, mentioned it might be nitpicky.
When I get such a review, it means I check it out again, and often, I want to fix that. In other cases, a discussion might come about. Or it's just nitpicky and can be submitted as is.
An important distinction: If the PR is approved with such a comment, I take it it's optional, so I decide for myself if I want to handle the comment. If not, I might ask you back how important it is (because you might just have forgotten to approve), or we get into a discussion about how important that is.
In any case, being nice is paramount IMO. It's hard to be too nice as long as you still point out things that might be issues. And on a more practical note, if you're not nice at all, you are lowering your chances that people will fix that thing if you still approved the PR. That's just psychology to me.
The conventional comments help everyone to quickly see the intent of something you wrote. But you can still be nice, don't be too "technical", because this is still communication between human beings, not between robots or AIs. And I'd like to err on the "nice" side, especially when writing with people I don't talk to every week, like members of different teams.
It especially helps to discern what is optional, what's background information, what's a suggestion and what is something that should really be fixed.
woodie3@reddit
found the comment. OP get your team to standardize feedback using conventional comments. After some time, you personalize your feedback while conveying your thoughts or concerns properly.
yolk_sac_placenta@reddit
I agree with this. Stereotyped comments make it clear what is being asked for without transparently patronizing "Hey good buddy doncha know ..." overcorrection towards tact; or a feedback sandwich.
Giving positive feedback just so you can say something critical, rather than because it's actually positive, is something I find depressing and disrespectful.
gnuban@reddit
This is yet another reason why I prefer face-to-face reviews. It's so much easier to spin up a casual conversation about the general approach and inject a question or raise a point like this in a natural way.
If I'm doing a PR and end up in this situation, I usually approach the author in person. And in the worst case where its not possible, although it can be a bit confrontational, I would just get to the point, something like
"Code and logic looks good, nice job. Error handling and tracing still needs to be added, though."
NopileosX2@reddit
Code review tools can majorly improve the process over talking to someone. If I had to talk to someone each time I get a review or do a review I would not get half of my current work done.
It should be an asynchronous process for the most part. You open a PR and someone will, without you even needing to tell, them review it, put their comments and you will later include them, do an update and so on. Discussion can be done directly at the code in text, fully visible for also all other reviewers and normally this is the place most effective, since you got the code diff and then comments from both sides. So if you think some error checking would be good at a certain place, you put it directly where you think it is needed as a comment. I sometimes also just put in a code snippet to show just directly show how I think something could be done differently or what might be missing.
Saying to someone "error handling is still needed" compared with directly showing them in the code where it is still needed (maybe even with an example) is a big difference in efficiency and also overall traceability.
Only when thing in code review arise which need more lengthy discussion one should go to face-to-face.
Ofc for this to work you need to be able to be direct in code review and not have anyone take it personally. In my team comments in code review are rarely positive, because code review in the end is mainly about finding errors of some sort. So it will be biased to more "negative§ comments and that is fine and everyone does understand it.
gnuban@reddit
Sure, PR reviews are great for linting and code style type review comments, or slightly more, but they're terrible for discussing bigger things like details around what problem was being solved, considered alternative approaches etc.
If something fairly big, like completely missing error handling, comes up, it's in my opinion rather pointless to try to hash that out inside a PR.
You can argue that this shouldn't happen and that PRs shouldn't be about discussing bigger questions. But it does happen quite frequently in my experience that people put up PRs too early and expect seniors to be able to feedback on the solution by essentially only looking at the code. And that's not how things work.
If you have a manual review process, it's a million times simpler as a senior to have a casual conversation to assess how well the author did their homework. For instance, did I understand the problem at hand correctly, did they test the flow manually, did they consider corner cases, did they evaluate performance, were they aware of possible other approaches or systems, did they talk to person X, etc etc.
In today's landscape I think we have way too much mediocrity because we assume that you can assess all this from staring at the code, but that's just not true in most cases. So people rubber stamp PRs or give feedback on what's being presented, which usually end up being very narrowly focused on linting and style-type feedback.
PR reviews have their place as the very final nitpick review before integrating a change that everyone already agreed on and understood, but in my experience they're being used for far more than that, which in my opinion is an anti-pattern.
Specific_Body8930@reddit
just write design docs, it's that simple. synchronous work and face to face interactions are dysfunctional patterns
gnuban@reddit
I can't believe that's your answer to what I wrote. I specifically adressed your point. We do write design docs at the place I'm working if you're wondering. And to be completely frank, maybe 20% of the docs have the quality required to actually convey what's being proposed, including considered alternatives etc. Most people focus just as much on the code of the one solution they conjured up as they would when putting up a PR.
The people who can write design docs are the ones who are already senior. So in the end the design docs present the exact same problems to me as PRs. You need to coach people to write them right. And the only working way to do that is to talk to them.
HotMud9713@reddit
I usually use the suggestion code feature of the source control tool
Middle_Ask_5716@reddit
I live by a simple rule.
If people don’t ask for advice I don’t give it.
There’s a special place in hell for people giving unsolicited advice.
bwmat@reddit
They implicitly ask for it by accepting employment in a team which requires code reviews
Better-Olive1193@reddit
Using conventionnal comment. I enforced this in mu team (pizza size) three years ago, i’ll never go back
https://conventionalcomments.org
sendintheotherclowns@reddit
That was very benign, you're being too hard on yourself. As a senior you're expected to give actionable feedback, what you've done here is given her the chance to go away and research/up skill and find out how best to do something. You've left the door open for her to ask questions or possibly even dazzle you.
I'd have been more blunt to ensure there's no ambiguity, instead of suggesting those things, stating that they're standards that aren't optional for example. And offering a one on one mentoring session of course.
lord_braleigh@reddit
I think the most important thing is to choose your battles. I point out real bugs, mistakes we can't undo, or risks that aren't behind feature flags. If there aren't any problems like that, then I approve. When I approve, that's when the nitpicks and lower-priority feedback comes out.
thekwoka@reddit
but doesn't that basically mean "none of that other stuff will be handled at all"?
So, why bother saying it?
It's like "why have warnings? Just have Errors or nothing."
lord_braleigh@reddit
Errors are problems that block a build from compiling, or a commit from being merged. In code review, that’s analogous to any problem that stops you from approving the PR.
Warnings are problems that don’t block compilation or merges. In code review, that’s analogous to a nitpick.
The idea I’m putting forth is that if there’s a Big Problem, you only mention the Big Problem. That may be a big enough problem that the author needs to rewrite the PR, which means the nitpicks won’t exist anyway. If there is no Big Problem, then you accept but also point out your nits.
thekwoka@reddit
Yes, my point is that warnings are generally useless, since everyone always ignores them.
If it is worth addressing at all, it should be an error, otherwise ignore it.
Qinistral@reddit
My coworkers often fix things I comment as “nit: I prefer x” while approving pr. Maybe they didn’t think of it and agree or just want to learn and follow advice.
darkveins2@reddit
That’s good advice. If you believe there’s limited clout and energy that can be used to tell someone what to do without adverse effect in a team environment, then you would never tell someone to make a non-functional alteration. Just look for bugs and obvious performance problems.
Secretly_Tall@reddit
Exactly, and in this case OP picked the right battle.
In terms of your feedback style, OP, you’ve earned the right to be more direct. You don’t sound like a jerk by making a recommendation, and engineers generally are eager to learn. On the other hand, pretending you’re not sure whether or not this needs to be done isn’t gaining you the brownie points you may be hoping, your team member can probably read between the lines.
There’s nothing wrong with stating your thoughts clearly, something as simple as “Great job with the core implementation. Before we send this to production, let’s add some failsafe logic and tracing to ensure we catch bugs, otherwise good to go!”
puzzleheaded-comp@reddit
What about something even more direct and just to the point? I’d comment on the PR “Let’s add some error handling here with a try catch” , then ping them and say hey I left a couple comments, let me know if you want to go over anything.
I don’t like the tip toeing around it, just get to it
RandomUsernameNotBot@reddit
Agreed. I don’t want to play guess who. Just tell me what you think is wrong.
WearMediocre6830@reddit
Out of curiosity. What feature flag service are you using? Homemade or some external solutions?
lord_braleigh@reddit
Homemade. We're talking about revamping it, and maybe about buying an external solution, but I'm loathe to pay money to introduce a networked dependency on our critical path.
snejk47@reddit
Typically with such solutions you have offline cache built in in the SDK and fetching features only once a while, so doesn't matter long term if something goes offline.
WearMediocre6830@reddit
Yes agreed. Asking as we are currently evaluating moving to external providers hence my out of topic question ahah. Some nice solutions out there, harder to find for on-prem. Homemade was nice for on/off use cases but too much overhead now that we want to progressive rollouts/segmentations.
lord_braleigh@reddit
We already allow people to roll out to a percentage of organizations. You just hash the organization’s ID to a number, then you take that hash mod 100, then that’s the threshold that your percentage rollout needs to be to include this org.
That’s just some code and math - seems easier than routing an airgapped onprem instance to
$THIRD_PARTY_SAAS_COMPANYwould be.human_eyes@reddit
Yep, any time anyone talks about some 3rd part feature flag management I'm always perplexed as to why it has to be any more complicated than this.
WaveySquid@reddit
The actual feature flagging part isn’t where the complexity is. Scale up any internal tooling up to 1000 engineers and now feature flagging requires rbac, version history, safe guards etc. Moving past simple A/B and want to run a multi armed bandit experiment to move a tiny metric 0.x% and need to make sure it’s statistically powerful. Live reloading of feature flags without stopping the service.
Just look at what statsig offers and the value added is everything on top of controlled releases.
Excellent_League8475@reddit
I used to use gitlabs feature flag service. We had a bug in our UI that caused the app to crash if there was a failure fetching the feature flags. We found this bug when gitlab went down... When gitlab went down, so did we.. And we couldn't push a fix because.. well... Not sure if rolling our own wouldve been better, but we were all thinking it at the time.
Exact-Guidance-3051@reddit
Code review is great tool to teach juniors. Keep the code in good condition, avoid technical debt so juniors learn from good code and how to write code.
You are teaching. Take it that way.
It's in everybody interest to have good code and learn do make good code.
silentjet@reddit
I'm doing a code review for more then 15 years. I still have no idea how to gently point to a fact that mixed spaces and intendations is not OK. Removing LF at the end of file as well as adding 5 new empty lines is not OK. Mixing braces style (new line, same line, with/without spaces) is not OK... Would like to hear best practices here...
yolk_sac_placenta@reddit
This shouldn't ever be part of a code review. Use linters and formatters to remove this from the equation. They should run in your hooks and be checked by your CI build before anyone other than the requester even sees it.
silentjet@reddit
Fully agree with you on that. But that's not how typically it is in RL. At least in project I was working on :(
yolk_sac_placenta@reddit
You probably know better then anyone why it sucks, then!
Honestly, these days, it's so obviously an industry standard I think I would just do it. Choose a hook setup, document how to install in the README (bonus: document how to run it in your team's most popular IDE) and add a check that fails if any of the formatters have a mismatch.
When
go fmtshowed the way, the saying was "it's nobody's favorite format, so it's everybody's favorite format." I don't like it--in particular I don't like tab characters in source code--but I love not arguing about it.Linting is the same way. I don't "agree" with rubocop or prettify on everything but one of the things about being "senior" is realizing what a pointless distraction it is to deviate from a standard for style reasons.
silentjet@reddit
I found go fmt just blessing... And in my area an industry standard is still C++ mishmash bsp code
birdparty44@reddit
I think devs are generally the poorest communicators out there. Also many of them never really learned to come out of their shell. The lack of social skills and social self-awareness hurts them most in code reviews. (Or just the tendency to have introvert’s anxiety when having to interact with humans.)
Whenever I’ve been on a team where I have any sort of leadership / seniority, I book a 30 min meeting to get everyone aligned about what code review is, why it helps, and how people need to get their mindset in the right place for it.
That already will solve a lot of this.
“It’s not criticism of you as a person, nor of your abilities in general. As soon as you divorce your ego from the code you produce, the more enjoyable you will be to work with. It’s just code. There are many ways to do it. According to our project docs, we do try to keep some standards and assumptions about a shared codebase so that it’s easy enough for all to maintain and extend.
Your colleagues want to talk about the code without worrying too much about the way those comments are received. Grant them that. If you feel they are being malicious, come to me with it and I’ll mediate.
That being said, as a reviewer, here are some guidelines about the tone you should aim for. Just because we talk about code, don’t forget there still IS someone reading those comments. It’s not a license to be a jerk, or equally, divorce your ego from what you do; you’re not here to flex you are here to collaborate with your fellow human colleague who took a different path than you to get where they are.”
And every so often I check in with my teammates to ask how they are finding the process. In my last job they appreciated I took the time to look after everyone’s well being.
thisFishSmellsAboutD@reddit
The best review comments I've seen and received were super nice, but also concise and actionable.
Maybe your team wants to agree on some prefixes like MH for must have (and won't approve unless 10p% addressed) and nit for nitpick? Then you can educate in ego-friendly language.
blokelahoman@reddit
Ugh, the “good catch” response.
When you leave comments you’re saying something about their work, and rightly or wrongly they may take it badly especially if they don’t know you well.
Lead by example. If you also commit code, include them as reviewers, so they can learn from your patterns - checking your inputs, etc.
Absolutely lead feedback, but make sure you’re consistent and then suggestion is actionable. Just don’t hit them on every single thing.
In tandem with that, if there is a broader point, such as input validation, security, best practices, etc, put together something for the whole team so it doesn’t feel directed at one person. Back up the examples with links to further reading.
Give them a way to grow and keep your own contributions on point and maybe some of it will stick. Some might still throw it in your face but at least you did what you could.
Good luck.
OddWriter7199@reddit
Sounds diplomatic to me. Well done.
Jesinski@reddit
I use decorators on my reviews https://conventionalcomments.org/
It’s been great.
Key_Lorde@reddit
Honestly I think you should just be straight forward. If people sense you are trying to help then it builds trust and that opens the door for that person to ask questions they may be hesitant to ask-- or perhaps that person doesn't even know WHO to ask.
Personally I love learning and I'm willing to accept better ideas and help when it is aligned with goodness. I accept that there is a lot that I do not know, and the information field that exists is overwhelming sometimes.
gasbow@reddit
> “This works! One thing to maybe consider: what would happen if this call fails mid-request? Wondering if wrapping it in a retry + logging block might help.”
This is pretty good already in my opinion and pretty much how I would do it.
I have been fairly successful as a reviewer with the following techniques:
* Do say positive things about the change!
It doesn't have to be a "feedback sandwich", but if the first comment the committer reads is something positive, it will help form a positive atmosphere.
* Phrase comments as questions. "Why do you do x?", "What do you think about doing Y?" etc.
It preserves the agency of the author and makes it seem less like an order or a test.
* add all comments together if the system allows it (like github or gitlab), and try to complete the review in one go. A constant trickle of comments or multiple rounds can be very frustrating.
* If I feel like the colleague might not have a good idea how to approach this yet; i will add a rough draft of how they could code it, to point them into the right direction.
* If I see serious structural issues in the change, I take it offline and try to talk to them in person. Then I can better judge how they take the news that I effectively want them to do it again, make sure they still feel taken serious as a team member, and guide them to the better solution. I find it hard to do that over a code review system.
polypolip@reddit
This also gives them a chance to have a discussion with you to present their point of view and logic. It's a large part of not feeling bosser around.
grimcuzzer@reddit
Asking questions doesn't work in all teams, sadly. Some people just skip the discussion part and just implement what they think you implied.
roscopcoletrane@reddit
Yeah, that’s been my experience with this “Socratic method” style of review comment. I find it better to be direct and say what I’m actually thinking, instead of forcing the dev to figure out what the right answer is. I’ll still do my best to be nice about it - although I vary my comment based on how experienced I think the dev is (and how familiar they are with the quirks of our codebase). Assume reviewing OP’s example scenario. If the dev is an experienced senior and should know better, I’m more comfortable being pretty terse: “this is missing error handling, can you please add some? (plus test coverage)” But if it’s a junior dev or someone who’s new, I’ll go into more detail: “it looks like this isn’t handling the case where the call fails mid request. That means that if ABC thing fails, then XYZ will happen which wouldn’t be good and could cause ZYX. Can you please update to handle that error scenario, and also be sure to add some tests that verify it’s covered?”
I try to be equally polite-but-clear with everyone. I’ve used the Socratic method before and in my experience it’s rare that a junior dev correctly interprets what I am getting at without more back-and-forth, which is exhausting for all parties. And as a senior dev, when I get Socratic-style comments, I know they’re trying to be nice but it feels condescending, like they think I’m a junior dev that needs guidance instead of assuming I just overlooked it (which is probably the reality).
brodeo23@reddit
Honestly have a good relationship with people. You shouldn't be worried about being bossy. And you shouldn't be worried about people pushing back against your feedback. Be a human. Have a conversation about it. Don't make it a "my way or the highway" situation unless it has valid, real architectural or error implications. People should not feel afraid to say what needs to be said and not take things personally, and it should go both ways.
darkveins2@reddit
What you’re doing is fine. As long as you: -Don’t nitpick on trivial non-functional stuff. -Don’t leave paragraphs of feedback. -Phrase it as a request rather than a command.
And if you care about style guidelines, then use the language guidelines via a static analyzer
finders-keepers214@reddit
Tactics like the feedback sandwich are just creating unnecessary bloat and confusion. If the psychological safety is on point and the team knows that code reviews are nothing personal then there should be no problem to be direct.
Existential_Owl@reddit
But it's still a good idea to point out good things in someone's code. Forcing folks to do the feedback sandwich is bad, but the team should still be explicitly encouraged to give praise whenever it's warranted.
Positive reinforcement is a great tool, but it's very human to forget to give that feedback when no one is reminding us to do so.
diablo1128@reddit
I personally want clear, direct, and to the point communication. I don't need or want people to sugar coat things as I think that can add confusion, especially when using hedge words. If you think there is an error just say "I think this breaks when X happens". I'll investigate and let you know.
Saying that I know everybody doesn't think like me. A once size fits all solution can work on the right team, but it's not going to be perfect for anybody. I tend to customize how I give feedback to the person as some people are just less confident and being too direct can cause them anxiety.
NopileosX2@reddit
Also code review is also for teaching. So if I see something I think could be done better I will note it but I will always be clear that things in code review are not hard "you have to do it" but more recommendations and I am fine with someone just hitting an ACK on it and not doing it, if my comment was a minor thing.
Also there should be no ego involved in code review from both parties, so no taking any comments personally and not trying to push your style onto someone else. In the end it has to be a clear and constructive process without any sugarcoating needed. Just explain your points and if there are bigger issues set up a meeting and discuss it.
AvailableFalconn@reddit
Part of the job is incorporating feedback. Like don’t be rude or an asshole, try to be specific and evidence based. But besides that, save the word smithing for your PMs and EMs.
gasbow@reddit
But good communication - and yes good communication techniques, are a critical part of forming a team culture where psychological safety is on point.
And this culture needs to be constantly refreshed.
RicketyRekt69@reddit
PR’s are where you have a chance to prevent bad code from being merged in. It shouldn’t be personal. I think being overly nice can come across condescending too.
Errvalunia@reddit
Be the most indirect about it—make them go add more unit tests and they’ll figure out the lack of error handling real quick!
JK obviously, but yeah you’re trying way too hard to downplay your suggestion—‘maybe’ ‘consider’ ‘wondering’ ‘might’. There’s sooooo much waffle in here, it sounds like you are the junior and you see an obvious glaring problem but you’re scared to question someone more senior.
gergo254@reddit
In this situation I would write something like: "The logic looks very good, but please add error handling and some tracing for better visibility!"
I prefer to he direct, but not rude. You can compliment the good part, but I would run this many circles to ask for fixin the missing parts.
Errvalunia@reddit
This is pretty much what I would write, though without the exclamation point (exclamation points are in be eye of the beholder—does it come across as chipper and upbeat or like I’m yelling at you?)
NopileosX2@reddit
Also best thing is to directly show in the code where error handling is needed in your opinion. This is imo the beauty of it.
Getting a general comment "add more error handling" might not even be useful if it is unclear where exactly, but getting a comment right at the place in the code where it should be added is great. As little as possible should be left for interpretation and in code review you normally can directly discuss the lines of code they are about and it is visible to everyone contributing.
jakesboy2@reddit
In the case of a junior dev here too, it would be helpful to provide a specific done state of what it would look like. So the top level review comment is what you said, then individual comments of handle X error here so the dev has a checklist to work off of.
mxldevs@reddit
I just ask what happens if a certain input or situation occurs.
Ideally they'll realize they didn't consider that case, or worse, they say they don't know which might suggest there are other issues at play.
Lopsided_Judge_5921@reddit
I like to give them code examples, blogs to illustrate concepts, or sometimes I'll just start a zoom call.
CypherBob@reddit
I'm much more direct, as some will take "maybe consider" and just ignore it.
"Excellent start. Let's add some error handling, retry logic, and logging. Let me know if you want me to look it over before PR"
Or something like that, depending on how junior the dev is.
Mediocre-Brain9051@reddit
Start some comments with "nit" for nitpicking. If only those exist, approve the PR anyway and clearly state that they are suggestions.
Never use code reviews for office politics, and think twice whether you are doing so. You there's the slight chance that you are... Think thrice.
UntestedMethod@reddit
Some comments are direct change requests, other comments are more about opening a discussion about the reasoning. Having those reasoning discussions documented in PR review can provide a reference in the future in case someone wonders why something is how it is.
ConDar15@reddit
Something I've found really useful is tagging all my PR comments with one of the following:
By using the above and making sure the author is aware of what I'm meaning, specifically when it comes to Suggestion, I've not really had much problem with dumping a lot of comments on a PR.
Ok-Key-6049@reddit
As with most things in life, remain objective
Temporary_Pie2733@reddit
The logic may have worked for ideal input, but I consider a complete lack of error handling to be faulty logic.
nanotree@reddit
Picking the right battles and establishing back-and-forth. Make sure that to have them participate in your PRs if possible. You can be plainly critical when people trust you aren't being petty and abusive. So make sure to explain your criticisms. And if you suspect someone new is especially sensitive, ask if they'd like to review their code in real time together. Sometimes a neutral critical tone is better conveyed by your voice than by text.
supercoach@reddit
I'd say you're a bit too nice. It's better to be blunt and direct than too nice and let them think the code is ok when you obviously think it needs work.
If you want to be nice, explain why you're asking for improvements to be made or do as you did and pose a question, but let them come up with the answer instead of spoon feeding it.
loumf@reddit
Point to other code in your system to emulate and your coding guides. Make it objective.
If none of your other code is correct and your guides don’t mandate a technique, fix that first.
chaoism@reddit
I use "do you think" to start the sentence
Do you think it's worth a try to use ABCD instead
Do you think making this efgh would be more efficient
Do you think it would be cleaner if we change it to ijkl here
CyberneticLiadan@reddit
I like to clearly differentiate between: (1) requested changes, (2) suggestions, (3) ideas which might be taken up or added to the backlog, and (4) nits which are personal preferences and might come down to style.
I think it also helps if the team develops explicit expectations for when to apply different levels of quality. If the new service is being stood up fast as a prototype, then maybe tracing and comprehensive error handling is a good idea but not critical. But if this is supposed to be production-ready maintainable code then those things are requirements.
Lastly, developing a good team dynamic of solidarity and humility helps keep one from second guessing. Encourage your juniors to review senior code and make sure seniors don't get defensive. IMO code review is about the team collectively owning any mistakes, and if team members believe with confidence that code review prevents a culture of blame, then everyone feels better about iterating quickly.
biofio@reddit
If there’s a clear cut issue like in the example you gave I would feel fine just saying it straight up, without any niceties. If there’s was something that was more opinionated, like “no I think we should organize the code this way” I would approach it with more tact, and be much more open to opposing arguments.
I guess the way I think about it is that if there’s a real problem in the code, I’m not calling it out to feed my ego, I’m calling it out because I want our team to have good code and I want other people to be able to write good code. So if I’m withholding genuinely useful feedback, I’m really doing more harm than good.
(Funnily enough I rewrote this comment a few times thinking about the best way to phrase it 😝)
MrEs@reddit
Just talk like a human with some level of empathy with good intentions, it's not that hard
tehfrod@reddit
This is very much a "now draw the rest of the owl" answer.
MrEs@reddit
Human communication is so dynamic and situational.
Some of my peers I could tell simply them they "fucked something up", other reports I'd have to give clear examples of alternatives, describe the pros and cons, and then let them chose or express there thoughts and opinions.
As long as you have a legit to blame culture, and you legitimately are looking out for them (by helping fix things), helping them understand, lifring them when they need help, hearing their point of view and leaving the floor open for two way communication. Then your doing the right things. You win as a team, engineering is a team sport, we succeed together, egos need to be left at the door.
tehfrod@reddit
Absolutely true.
JimDabell@reddit
You’ve gone way overboard. This is far too indirect. You make it sounds like it’s good work and you’re wondering about a nice-to-have. You don’t have to be scared and apologetic for giving something other than 100% praise.
This is bad communication and bad mentorship. Juniors need clear signals to understand where they are going wrong. She skipped error handling altogether! That is not a minor thing and you shouldn’t be teaching her it is. What she needed to hear from you is that skipping error handling is unacceptable, but you told her “maybe consider” adding it.
There are many people who would read what you wrote and interpret the job as finished and believe you are happy with it as it is. This is especially important if you are managing somebody with a different cultural background to yourself.
You shouldn’t be harsh or unkind, but you do need to be clear.
If something is missing error handling, just write “This needs error handling”. If it’s not clear which scenario would trigger it, then describe it. That is not “having a god complex”, that’s communicating like two adults who are working towards the same goal.
Absolutely do not write the code for her. Your job is to point her in the right direction, not to do her work for her.
Mammoth-Clock-8173@reddit
Agree. One of our squad reviewers keeps phrasing things as, “do we need a test for scenario?” (Yes, we do, it was in the AC, I forgot, my bad). But don’t ask me for an opinion when my opinion is not what you want to hear.
elnelsonperez@reddit
Yeah this is a culture issue. Having to be indirect in efforts to not hurt someones feelings is a time waster. PR reviews are about code and people are not what they write.
Sulleyy@reddit
There is certain phrasing you can use. We are a team and you're making a suggestion (even if you know you are right and it's more of a command, that's not how it should come across)
"Consider putting this in a try catch"
"Can we put this in a try catch? If x is null we'll have an uncaught exception"
"Have you tried testing x? I suggest wrapping in a try catch because blah"
verb_name@reddit
Be direct while respecting the author.
"Needs error handling and tracing for x and y. For example, see or retry the requestand log at FATAL level if the retry fails. Also needs tracing for z. Otherwise, looks good."
Pleasant_Fennel_5573@reddit
Did she understand your review well enough to resubmit something better? If so, you did great! If not, you may need to be more direct and specific about the missing elements.
I would not just write the code for her. You gave her the feedback, it sounds like she took it well, and it’s on her to ask for clarification if it is needed.
templar4522@reddit
Also, ask and accept feedback from people that don't like your comments. Ask which comments and why, and what can you do to improve. Showing you are trying (and then actually trying) will make a difference in how you are perceived.
As for the nitpicking, I find that what can be automated should be (stuff like formatting, coding standards, and other issues that can be found with static analysis tools). Once that is in place, your nitpicking will decrease immensely.
Subjective stuff like naming will still be a point of contention occasionally. Just phrase things like this: "I think naming this X isn't very readable. Can we name it Y, I think it communicates better what this piece of code does."
And sometimes it's ok to give up when opinions differ. Just like when you are forced to use tabs when you prefer spaces, working in a team means you have to let go of some of your own preferences.
RandolfWitherspoon@reddit
Put the word “Consider” in front of all of your demands lol
RandolfWitherspoon@reddit
Then when they don’t apply the change in their next update, reply, “did you consider ___” to enforce that “consider” is the polite version of “fix it” lolol
YetMoreSpaceDust@reddit
I always stop and ask myself, "does this really matter?" Retrying does. Thread safety does. Escaping content does. Sanitizing user input does. Using a loop instead of a lambda function probably doesn't. Breaking a long function into smaller ones probably doesn't. Variable naming probably doesn't. If I give any feedback, I make sure to back it up with why it actually makes a difference - I'm not trying to teach them to code the way I would, I'm just trying to make sure nothing breaks in prod.
Sounds like you're in the clear here.
ccb621@reddit
I like using emoji to help classify the feedback: https://devblogs.microsoft.com/appcenter/how-the-visual-studio-mobile-center-team-does-code-review/.
That said, some things are just part of the "definition of done". If the tasks/features aren't complete, the service/project is is not complete. I include error handling and observability in my definition of done. For simplicity, our definition of done is documented, so we can easily point to the section that folks need to address.
kilkil@reddit
personally I have the same issue, and I approach it by using language like "I suggest", "I recommend", "I advise", etc. If it's something serious (like no error handling), I would add a "strongly" in there, e.g. "I strongly advise". I always make sure to include an objective explanation to appeal to.
The most forceful language I've used has been slight variations of "you should do X", but with:
Other times go for a more passive phrasing of simply stating something as a fact: "currently, this code will do X in situation Y, which will lead to Z. If you make changes A, B, C, it will instead do D, which is more desirable."
teerre@reddit
Personally, after trying many approaches, including conventional commits, I learned that the best way is to make sure that people know your intentions a priori. Training your team to accept feedback, having defined - whatever it is - design and development guidelines works the best and having clear review etiquette (e.g. must review in less than X time, if answered, must reply in less than Y time etc.). Just like with the code itself, by the time there's a code review, people should already know what's expected
Nowadays I often just block reviews with straight to the point comments and others, including more junior people, do the same to mine
Conventional commits is specially misleading in my opinion because they just end up transferring the sarcasm to another level, it becomes a game of how to be toxic while using the guidelines. It's also just avoids the problem, if you check their main page, everything is about minor or suggestions, but that's the whole issue, people will gravitate towards a non-confrontational comment even when it should be a blocker
mattrowe9@reddit
Google “conventional comments”
scceberscoo@reddit
Your feedback honestly sounds just fine in terms of tone. You may benefit from being more specific, and I would definitely recommend being less hesitant to offer feedback. Good code reviews are essential for a healthy codebase!
I personally try to focus on collaborative wording. It really helps with tone and reinforces that even individual PR's are part of a team's codebase. This may be your (or my) code, but once it's merged, it belongs to all of us.
I also try to approach reviews with curiosity and phrase feedback as a question to invite collaboration. (ex: "What do you think about pulling this logic into a reusable function? I think we do something similar in xyz component so it would be nice to DRY things up.")
And finally, always provide feedback on issues that will cause problems down the road, but if you want to provide non-blocking feedback, simply call that out (and don't overdo it). I tend to preface this stuff with "nitpick:" or something like "This may be out of the scope of this PR, but I was wondering what you think about refactoring this to "
Awric@reddit
I used to leave comments like yours all the time in code review, but sometimes it’s worth being more assertive for teammates who have a habit of saying “Nice, good catch! I’ll address this in the future! never actually follows up”
If it’s a direct teammate, I’m assertive, but I establish in face to face conversations that it’s not written with attitude or anything. If it’s a coworker from a different team, I continue to go with the friendlier / passive review style for nitpicks because they likely don’t have time to sync over it on zoom with me
TonyAtReddit1@reddit
Review pairing sessions. I never leave more than 4 or so comments on anything. If I have 5 or more things to say about some code it's better to just talk through it rather than trade github comments
armahillo@reddit
Personal preference but I loathe passive-aggressive questions in PR reviews. It feels infantilizing.
If you think something should be different, say so and say why. You dont have to write a novel; state your opinion, provide links if needed, maybe illustrate a suggestion.
If you actually dont understand why they made a choice, ask about that.
PaleCommander@reddit
My coworkers are smart, and hopefully yours are too? I would have dropped an "If an IllegalDanceMoveException gets thrown here, what happens?" comment, maybe with some additional "Likewise, what happens if this throws a SadPandaException?" comments if there are multiple places that need updating.
If they know what to do and just weren't thinking, they'll go "Oh, right, thanks. Added error handling".
If it's a teachable moment, they won't know and you get to explain what the default error handling behavior is and how they should handle errors here instead.
If you're wrong and there's a catch block you missed in a wrapper method elsewhere, they'll point it out, and you were polite, so all is well.
Your version isn't bad. It's just a little too soft for my taste, but that's just taste. The bigger issue is taking 20 minutes to wordsmith it, but you'll get better at using whatever voice you choose confidently as you do more code reviews and get feedback.
Yogi_DMT@reddit
Don't nit pick. No need to add emotion like sorry or please, this good is subpar, etc. just state the points that are actually blocking to you as matter of fact as possible. Sprinkle in some best practice type of feedback here and there but try to stay focused mainly on risks and functionality.
anonsaltine@reddit
i just ask questions. “what are your thoughts on doing xyz?” “if we do x, i believe y would happen. would you agree or am i missing something” etc
Cube00@reddit
I'd stop here or expand a bit to "how do we report this error?"
I've found asking the question and letting them join the dots can open up a friendlier time which makes it seem less like an order or a lecture to implement things in a certain way.
It depends on the dev but worth a try at least once if you are still open minded on their abilities.
PerspectiveLower7266@reddit
In Github I use the Make a suggestion button to give actual feedback. When a pattern is bad I say, you should look into this design pattern. When it doesn't match the rest of the repository. I say consider refactoring to make it look more like XYZ code. If you think it's really messed up, say I am not sure I understand what you're doing hear, can we get on a call and you walk me through it. If it's scope of the changes I'll say soething like, next time let's submit this in more than one PR.
jhaand@reddit
The best thing to keep in mind. "Be hard on the subject. Soft on people."
As an example.
We had a review process for documents once where every reviewer.sent in a spreadsheet with comments. Aggregating comments was always tough but doable.
So for once I thought: "let's do proper time management and aggregate the comments just before the review meeting." That was a very grueling experience to manage a deluge of comments, just before the review meeting. During the meeting everyone was very understanding, so it all worked out.
It was better to integrate the comments in the big sheet as they come in. Then you only have to manage bits and pieces of review comments. And you have more time to process the comments before the meeting.
Sweet_Television2685@reddit
call your review points as suggestions, indicate the reason why you highlighted it, and indicate severity level and also justify with a remark
ctrl2@reddit
Almost all of my code review comments are written as questions. "Have you considered [this]?" "What if we did it this way?" "Could the implementation be changed..." Etc.
There are other cases where they are statements, where something is more obvious or egregious and you need to take a different tone.
I think your comment was fine, i would remove the "compliment sandwich" just because it is unnecessary fluff.
gomihako_@reddit
Socratic method
Dexterus@reddit
I would have written: "Logic looks sound. Add some more error handling and tracing. Check this, that and that other one. See how it was done"
I would be a little more personal/nice in an informal
WaitingForTheClouds@reddit
You weren't vague, imho, the question feels a little patronizing, in reviews I prefer being direct and matter of fact, I'm reviewing code not to demean someone nor to tutor the dev, I review it so that we catch problems with the code and fix them before merging, that's it, nothing personal. I also tend to provide reasoning behind why I want the change unless the problem is obviously just a typo or forgetting some basic check. If the problem is more complex and I am able, I suggest a rough idea on what to do. And tbh I don't really think about these things, I just write them off the cuff with the mindset that I'm doing this to make sure our code is good, not to judge someone's work.
Now, my TL gives vague and imho bossy sounding reviews but they look like this: "this is still wrong", "this makes no sense" or "doesn't work" and I shit you not this is word-for-word and usually these usually turn out to be something like an unhandled error check or an obscure, undocumented use-case that's hard to track down. There's no way to know what's wrong from his comments and he says the same kind of shit for actual major issues, minor mistakes and even just shit he doesn't like in "his" codebase. So yeah, you're good bro.
Prestigious_Dare7734@reddit
I try to give some elaborate examples, like some quick google search to come up with a medium or dev.to article where someone else talks in depth about the issue I am trying to explain. 1, it helps as the article might explore more in depth than I can do in a PR comment. 2, it wont look like MY preference but something that others also follow.
Doctuh@reddit
Try this: https://www.manning.com/books/looks-good-to-me
Barktweetspeak@reddit
Get into the mindset that this is your code too and you're solving it together. "We can't do this because of principle x. How can we follow best practise here etc"
Comment when it's really good code too.
Have some take or leave things.
Anything else that's code you wouldn't deploy youself and really have major concerns - "I can't approve because of reason X. You'll need to get someone else to approve it." If you approve it, it's now your problem too. I've never had this happen that said as most people are reasonable I've found.
Unless your writing life critical code though, maybe compromise is worth it for the most part.
Historical_Ad4384@reddit
Missing obvious things like exception handling, telemetry, tests, missing edge cases and divergence from team patterns should be pointed out in day light especially if the commiter is new and there is enough historical background to support your view.
IMO this is what code review is all about, especially if you are already an experienced working member of the team.
Nitpicking on variable names, file names, tabs vs space or even design decisions to some extent not so much unless it breaks CI/CD or produces a big ball of mud vs future goals.
bigkahuna1uk@reddit
Teach not preach. Ask her questions on why orthogonal concerns are missing or if the code is resilient to exceptional scenarios. A good engineer will not see it as criticism and ideally be receptive to your suggestions.
nagyerzsi@reddit
Honestly without judgement
ToTooThenThan@reddit
Be direct, no "wondering" or "might help"
kenflingnor@reddit
Conventional comments
Howler052@reddit
Better to be too nice than too harsh.
RusticBucket2@reddit
I had a guy who spoke Russian and English, but his English was not good. I’m pretty sure he used a translator for stuff like comments, etc.
On one PR, I told him that something “smelled”.
Code smells are a concept that we should all be familiar with. It means something seems wrong, but I can’t say exactly what.
He reached out to me on Teams and told me that it was highly offensive (I think he said “vulgar”) and then he said, “I require you to remove this comment.”
Thankfully, he did it with a very collaborative spirit.
lol
teratron27@reddit
Basically: be professional. Don't need to be overly nice and don't want to be rude/negative.
WearMediocre6830@reddit
100%. Hard to not have some emotions when receiving a PR but at the end of the day, it's just a team trying to get their daily working tool clean. Important to work together on it than just making it a "who has the biggest one" competition style
cat-in-da-box@reddit
AlexandraLinnea@reddit
A good way to think about code review is as a process of adding value to existing code. So any comment you plan to make had better do exactly that. Here are a few ways to phrase and frame the different kinds of reactions you may have when reviewing someone else’s code:
Not my style. Everyone has their own style: their particular favourite way of naming things, arranging things, and expressing them syntactically. If you didn’t write this code, it won’t be in your style, but that’s okay. You don’t need to comment about that; changing the code to match your style wouldn’t add value to it. Just leave it be.
Don’t understand what this does. If you’re not sure what the code actually says, that’s your problem. If you don’t know what a particular piece of language syntax means, or what a certain function does, look it up. The author is trying to get their work done, not teach you how to program.
Don’t understand why it does that. On the other hand, if you can’t work out why the code says what it says, you can ask a question: “I’m not quite clear what the intent of this is. Is there something I’m not seeing?” Usually there is, so ask for clarification rather than flagging it as “wrong”.
Could be better. If the code is basically okay, but you think there’s a better way to write it that’s not just a style issue, turn your suggestion into a question. “Would it be clearer to write…? Do you think X is a more logical name for…? Would it be faster to re-use this variable, or doesn’t that matter here?”
Something to consider. Sometimes you have an idea that might be helpful, but you’re not sure. Maybe the author already thought of that idea and rejected it, or maybe they just didn’t think of it. But your comment could easily be interpreted as criticism, so make it tentative and gentle: “It occurred to me that it might be a slight improvement to use a sync.Pool here, but maybe that’s just overkill. What do you think?”
Don’t think this is right. If it seems to you like the code is incorrect, or shouldn’t be there, or there’s some code missing that should be there, again, make it a question, not a rebuke. “Wouldn’t we normally want to check this error? Is there some reason why it’s not necessary here?” If you’re wrong, you’ve left yourself a graceful way to retreat. If you’re right, you’ve tactfully made a point without making an enemy.
Missed something out. The code is fine as far as it goes, but there are cases the author hasn’t considered, or some important issues they’re overlooking. Use the “yes, and…” technique: “This looks great for the normal case, but I wonder what would happen if this input were really large, for example? Would it be a good idea to…?”
This is definitely wrong. The author has just made a slip, or there’s something you know that they don’t know. This is your opportunity to enlighten them, with all due kindness and humility. Don’t just rattle off what’s wrong; take the time to phrase your response carefully, gracefully. Again, use questions and suggestions. “It looks like we log the error here, but continue anyway. Is it really safe to do that, if the result is nil? What do you think about returning the error here instead?”
—Death by a thousand nits
Livid-Ad-2207@reddit
Use suggestive language like: "consider doing this... " "try fixing this"
If they don't change it and it doesn't entirely break the system, let it pass, take ownership of the issue and fix it yourself in a further iteration.
the300bros@reddit
You ask questions or point to existing examples/coding standards instead of giving orders. You lead by example. If you don’t have examples in the codebase or coding standards it’s going to be real difficult because it may seem unfair from their POV.
I don’t remember ever thinking someone was being bossy towards me but I did think it was knit picking when they complained about something minor which they themselves also had pushed out. My response was just to refuse to approve the same stuff in their code. Code quality improved. It seemed personal for a second but wasn’t really. Lol.
kyriosity-at-github@reddit
just be yourself, look how Ivan Vanko gives soft feedback:
https://www.youtube.com/watch?v=seKLsHOkN1U
Sensanaty@reddit
I think your style is fine. I think what's more important is making it clear outside of PRs that feedback is feedback on the code and not the person, and none of it is personal or should be taken as such.
I'm pretty blunt, but I always try to back everything I say with objective facts. I'll link to the relevant documentation, or other pieces of code, stuff like that. If it's something that's purely my opinion, I make sure to mark it as such: "Nitpick/Subjective, but what if we approached this in X way instead? I've found that when Y or Z happens, this method doesn't work as well because... Feel free to disregard though".
In my experience it's never been an issue whether it was an intern or the head of the company, there's room for sensible discussion and everyone knows it's not personal.
Comprehensive-Pin667@reddit
I'd probably hust comment "Please add error handling" on the line that needs error handling. We're all adults, she'd probably be just as happy if you let her know without any of the filler
RabbitDev@reddit
For juniors I tend to write comments as teachable stories.
So for code that has not good error handling, I the "this covers the happy path, but complete code isn't just correct in the sunshine days, and must also be resilient (if there's error recovery) or predicable in the face of errors.
I tend to write a couple of questions to point them in the right direction, or pull out some of the standard literature on how to write error handling in production code.
The nice thing about junior devs is that they are predictable and are going to make similar mistakes out of inexperience or bad teaching from their university or college. So over the years I have collected a set of standard responses and books for the common problems that I simply repeat and adapt as necessary.
I find that mix of pointing out the wider problem and guiding them to solutions by showing context means they pick up the knowledge of knowing why those things are important and how to approach them in the design phase.
Inside_Dimension5308@reddit
You did good.
The ideal way is to frame it as a suggestion.
"I think there is a better way to do this..." "Please think about optimizations.."
This doesn't look offensive. Framing it as a command is a big no.
"Refactor this..". "Why has this not be done?.."
What to review is a different problem? I usually try to ignore things which can be easily done by integrating linters. I check for optimizations and error handling.
EnderMB@reddit
For nits, I just do
Nit: We could move this to the other fileand approve. If they care enough, just create a separate code review later.chrisza4@reddit
Don’t ask us. Ask the person who received feedback.
There is no one right way to communicate code review. In fact, the most effective code reviewer adjust communication style based on receiver. For some you would need to be softer and for some they would prefer direct.
Let us be human and communicate like a human being.
teratron27@reddit
I usually try to follow this: https://conventionalcomments.org I try to remove any ambiguity that could come from tone or differences in language. (mainly as I've always worked in teams with lots of different nationalities and native languages, all trying to parse their thoughts into English)
Full-Watercress-1699@reddit
I ask ChatGPT to paraphrase my points, and make it sound friendly and kind. I then compare both and write a warmer version of my feedback.
The contents of code reviews are essentially "I don't know if this is a good idea." Keeping friendly and kind tone helps to make it a good conversation, not a critique/debate/"battle" per se.
gasbow@reddit
Are you not worried that using generated text, you will come across as inauthentic over time?
If your colleagues feel like "this feedback was written by ChatGPT and not Full-Watercross", that would be very bad.
Full-Watercress-1699@reddit
Oh yeah, that's why I "write" a warmer version, not just copy ChatGPT's version.
Thoguth@reddit
it looks too nice and vague.
"feedback sandwich" is dumb.
Treat them like actual, lovable humans that you respect. Do this all the time.
And be clear to them when you say what needs to get better.
The main thing is, don't be fake. And don't gold-plate. Is the feature, or the standard you're coding to, explicitly supposed to have logging and failure-recovery built in at that level? Architecturally it seems better to build logging and failure recovery at a higher/more-abstract level that all the services can use without having to explicitly do it every time you set up a new service (or make a call to a service?) And also ... that seems like a feature to plan and execute on its own, as it becomes recognized as valuable.
m1nkeh@reddit
You make suggestions not demands.
MangoTamer@reddit
Your writing style includes unnecessary niceties. You don't need to say "maybe" because that makes you sound uncertain. You aren't uncertain, you are positive that you are correct so just own it.
thclark@reddit
You may be over-thinking it. You need to step back and think of the four principles of great communication: it needs to be Kind, Helpful, Truthful and Timely.
The only one of these that could do with further expansion is ‘timely’ - in this context, that means “is the junior dev ready to hear this feedback and learn from it”. You want developers stretched but not overwhelmed, so it can be “timely” for example if the dev has been struggling with the logic and finally got there, instead of piling on and stretching out a PR, instead make a tech debt issue to add error handling, and assign it to the same developer in a few sprints time when they’ve leveled up a touch.
VRT303@reddit
Just add one nice comment about the good thing and then under it create tasks for everything you consider necessary to be made, then mark it as needs work?
In a good review I find at least 1-3 tasks, even for people with more experience than me
Knaapje@reddit
My 2 cents: still mention your nitpicks, but add a disclaimer that they are free to disagree or disregard them. They shouldn't be blocking for merging.
reallybrutallyhonest@reddit
We try to use a feedback ladder. Helps clearly define what needs to be addressed and what is just preference/nitpicking. Something like this.
Unless there’s a ‘boulder’ or worse, I’ll usually approve.
Kolt56@reddit
Juniors aren’t fragile. Bad production code is. Block the CR. Spell out the risk. Offer help. Then move on, you are protecting customers, not auditioning for Most Friendly Reviewer. If that reads as being a dick? That’s called doing your job.
twnbay76@reddit
Use conventional commits. Takes the feelings out of the equation. It's a standard system.
Sandwich method. Good+bad+good
Keep it as succinct as possible.
Single pass through filter for offensive wording. Dont overthink. Someone once tried to abstract an http library's methods in a case statement and I said "maybe we can trim this module down, this method isn't doing anything intelligent" and I got "maybe you shouldnt be saying something like not being intelligent in a PR".
randomInterest92@reddit
If people take code reviews personal then it's their problem, especially if it's in a professional environment.
You should focus on communicating with efficiency. You don't need to be overly nice about it. As long as you can always find reasoning and you're open for discussion, you're good
WearMediocre6830@reddit
Your approach was good imo. As someone stated above. With lot of comments to address, it's nice to couple it with a direct message. I personally like working in a team where the situation is healthy at the point that PR becomes a place for discussion. Meaning its not a all or nothing submission, but it serves to debate, get people feedback early enough to end up with a useful contribution for the codebase overall.
I dont remember, i think it was google' PR rules of thumbs. Deep down a PR needs to improve the overall quality of the codebase, not be perfect all the time
tip2663@reddit
Ask them to review yours, check in tandem.
Exac@reddit
As others have said, make your feedback friendly. You want to accept code that you're comfortable maintaining, but you also want to make sure they know if you're just nitpicking, or if you use words to make sure they know you don't think a change _must_ happen when you're offering suggestions or questions.
If you want to point out that something is done another way somewhere else, take the time to find a link to the file and line/character for your colleague.
If your colleague has had trouble splitting up the PR into small PRs and you only have time to review part of it, let them know when you'll review the remainder.
Also, as someone who gets a review on their work, you should leave a comment if you make a suggested change that includes the commit hash (which creates a link to the commit on GitHub). It is frustrating to review someone's work and see they agreed about a change, but then re-requested your review without making the change. Sometimes they simply forgot to push.
Strict-Soup@reddit
This is a good question, and you're right to question how we deliver feedback. We are in an industry where we are expected to give feedback but there has been no training on doing so, in a constructive and courteous way in the same way teachers are given training.
First, I would say the way you delivered that feedback was spot on.
On my comments I have found that I ask questions more than anything. "Do you think this would be better putting in a method?", "do you think it would be beneficial to add some logging?".
This way I'm not demanding, I'm starting a conversation about what might be better and making them a part of it. People hate to be "told" what to do.
In teacher training they talk about the compliment sandwich, start with a positive, a negative then finish with another positive if possible which you did in your example.
The very fact that you are thinking about this, reflecting shows that you aren't being bossy and I doubt you have a god complex. You just know the code and you want to share your knowledge.
I'm sure if you carry on like this the junior will very much appreciate it, they are lucky to have you tbh.
I would however never shy away and let things pass just for you to fix them yourself later. (I've done this) It's not fair.
But great question op
BogdanPradatu@reddit
I don't have any templates myself and I don't use tricks. I just point out what I see, it's a code review, that's what they added me for. We're using bitbucket so we have things like comments, tasks, suggestions. Plain comments and suggestions are usually just that: opinions that can be ignored. Tasks are things that I expect to be treated, but the author can also justify why they won't do it and it's all fine.
Sometime I leave comments, suggestions and even tasks (giving the author the chance to argue against it) and still approve. Otherwise there are things I insist on (e.g. logic flaws, error handling, clear descriptive logging etc.)
Thommasc@reddit
Criticize the code, not who wrote it.
Always refer to a violated guideline or principle with a link to it every time you raise a concern. This way it's not just you, it's the entire industry that wouldn't like this piece of code.
Tasty_Mode_8218@reddit
I point out whats needed or wrong or what can be done better. Im probably to blunt or nitpicking, i epuldnt be if they followed the coding style and standard set out.