You're right about the "unexpected" part. I think it really depends on the context and the person. Both factors play a big role here.
Imagine being in a spot to review a new hire's PR and you've barely interacted with them before this encounter.
A reply of "Your method technically works but next time try xyz instead. It's more efficient. ;)" is extremely hostile in my mind. That phrasing is already over the top horrendous but the ;) amplifies it by 10x.
As for a breakdown of why I think it's horrendous:
"Your method" <-- You're already tying it directly to them
"technically" <-- An unnecessary word, it's adding salt to the wound and makes you come off as arrogant
"Next time" <-- Majorly condescending
"Try xyz instead" <-- A bit direct
"It's more efficient" <-- At least it describes the 'why' but there's 100 other ways to say this
";)" <-- Keyser Söze tier ruthlessness
I think if I were really in this spot I'd probably reply with something like: "Hey, I ran your solution and it's working nicely. Thanks a lot! By the way, recently I've been working on profiling our code so performance is on the top of my mind. I ran the profiler against this PR and noticed a dip in performance in one of the functions. Do you want to hop on a call sometime to explore a few ways we can refactor xyz function? Really curious to hear what you think and after thinking about this for a while I have an idea or 2."
the fact that neither of your wordings makes it clear whether I actually have to address your comment to get my code merged is much worse than the tone of the first version.
like it or not, you are temporarily in a position of power when you are a required reviewer on someone else's change. they need to satisfy you to move their ticket into the "done" column, and you both know it. rather than dance around this uncomfortable situation with soft wordings, just focus on making it clear what is mandatory and what is optional.
> Hey, I ran your solution and it's working nicely. Thanks a lot! By the way, recently I've been working on profiling our code so performance is on the top of my mind. I ran the profiler against this PR and noticed a dip in performance in one of the functions. Do you want to hop on a call sometime to explore a few ways we can refactor xyz function? Really curious to hear what you think and after thinking about this for a while I have an idea or 2.
But that's making a PR comment into a meeting, which is something I'd kinda dread and want to avoid.
That said, I agree that this
> Your method technically works but next time try xyz instead. It's more efficient. ;)
is awful. But it's not just the use of an emoticon. It's that it's the wrong emoticon.
> this works, but xyz would be better (more efficient) :)
Would be a bit ambiguous in tone, but I think at least invites an optimistic reading as well.
For me I'd rather the niceties in a PR (as well as proposals to meet) live in a comment at the beginning or end of a PR. Individuak requests for specific changes within a PR review should be clear and simple, not bogged down in formal crap or fluff, imo.
Yeah, for me at least. I feel like that would make a meeting more awkward, not less. Also, if I were in the new hire's shoes, scheduling a meeting that's essentially about criticism would make me more anxious, not less. And the time gap between
the scheduling of the meeting and the start of the meeting might be pretty fretful. If someone just lays out what they want changed in 1 or 2 sentences, I can immediately know that it's not some big problem they want to meet about.
At the same time, I would feel bad if I found out that I had really distressed a new hire in a code review. And I do try to soften things when I say something that I anticipate might be perceived as harsh in a way that I don't intend. (I use emoji a lot, I DM people, and I tend not to use standalone sentences that end in periods except in email. I use parentheticals and qualifiers, make jokes, etc.) I do schedule meetings to go over things including criticisms/problems/objections, but only when I feel like it would be faster and more natural, or I think it's really essential that I make a realtime demand on someone's attention to get their input.
Imagine being in a spot to review a new hire's PR and you've barely interacted with them before this encounter.
A reply of "Your method technically works but next time try xyz instead. It's more efficient. ;)" is extremely hostile in my mind. That phrasing is already over the top horrendous but the ;) amplifies it by 10x.
As for a breakdown of why I think it's horrendous:
I think if I were really in this spot I'd probably reply with something like: "Hey, I ran your solution and it's working nicely. Thanks a lot! By the way, recently I've been working on profiling our code so performance is on the top of my mind. I ran the profiler against this PR and noticed a dip in performance in one of the functions. Do you want to hop on a call sometime to explore a few ways we can refactor xyz function? Really curious to hear what you think and after thinking about this for a while I have an idea or 2."