[HN Gopher] Ask HN: What tone to use in code review suggestions?
___________________________________________________________________
Ask HN: What tone to use in code review suggestions?
When writing suggestions in code reviews I have used all of these
forms: * Should we extract this to a separate function? * Could
you extract this to a separate function? * I would extract this to
a separate function. * This could be extracted to a separate
function. * This should be extracted to a separate function. *
Extract this to a separate function. As you can see, these have
very different tones and I would like to be more consistent and as
constructive as possible. Is there some general best practice for
this? Are you or your team using a set of rules or guidelines?
Author : zorr
Score : 213 points
Date : 2022-06-24 07:09 UTC (3 days ago)
| nailer wrote:
| I normally say why - even if they're 'experienced' sometimes
| people can sometimes come from, say in this case, cultures where
| copypasted code is normal. So say why:
|
| * "We're repeating this in a few places. Could you extract this
| to a separate function? It'll keep the codebase smaller and more
| readable, and if we update it, there's only one place we need to
| change."
| anticensor wrote:
| * This shall be extracted to a separate function.
| aard wrote:
| The tone of a code review suggestion reflects an underlying
| assumption about what a code review is (or should be). Often
| developers disagree on this point. It seems like we fall into one
| of two camps.
|
| 1. Reviewing code is like proofreading a paper. Suggestions are
| welcome, but there isn't an expectation that all suggestions be
| followed. The author of the code makes the final decision based
| on their objectives.
|
| 2. A code review is a gate that is closed until all comments have
| been fully addressed. The author must comply to the liking of all
| who choose to comment, or they are not authorized to continue.
|
| The tone of review comments will naturally follow from their
| understanding. Understanding #1 yields comments aimed at
| persuasion. (Should we, Could we, You might consider) While #2
| yields more compulsory language (Change ..., You should). Or
| passive aggressive language that is meant to be compulsory but
| softened to sound less aggressive (I would, Could you, shouldn't
| you).
|
| It is my opinion that understanding #1 is most appropriate. It
| prevents unnecessary bottlenecks and is more respectful or
| differing opinions. If developers are peers, than #1 is the
| understanding the most accurately reflects that.
| racl101 wrote:
| Your examples lack a reasoning or explanation as to why code
| should be abstracted to a separate function.
|
| 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.
| scrapheap wrote:
| Personally it all depends on the rest of the comments in the
| review, if you use one style all the time then you lose the
| ability to indicate which changes are the ones you feel are more
| important than others.
|
| I'd use the more question style comments for the minor changes,
| and save the stronger comment style for those changes that are
| really important.
|
| So I'd word a comment about changing a variable name to something
| slightly better as a question. e.g. Would accounts be a better
| name for this array?
|
| While a comment addressing a security concern would be a direct
| statement. e.g. Use a parameterised query here, as it'll be
| harder for someone to exploit.
|
| One thing to note is that when making the strong statements I
| like to back them up with a reason, that way it helps me to think
| through the comment itself, helps a more junior developer
| understand why they should change it and helps more senior
| developers to not take the comment personally.
|
| The other side to this though is that as senior developers we
| need to set an example for our more junior colleagues on how to
| take review comments. I always try to take them in a good mood,
| make the changes when I agree with them and have a constructive
| conversation about those that I don't agree with.
| marstall wrote:
| I wouldn't personally use any of these, but I'm a bit of a code
| review heretic.
|
| I view (peer) code reviews mostly as an opportunity to discuss
| and familiarize your team with your code, and I think they should
| conform to the "social rules" that prevail in the rest of the
| world. If a peer in the real world was sharing something with me,
| I would very rarely command or strongly suggest they change this
| or that thing about it. Instead I would strive to create a
| collegial atmosphere by being receptive and supportive 90% of the
| time so that 10% of the time a suggestion I offered would be
| received gratefully and gracefully.
|
| None of this can happen in a github pull request.
|
| There was an alternative that I used to see before the PR
| monoculture took over that I liked. It could look like a weekly
| "code review" in-person meeting where a random developer would
| put together a presentation about a new module they'd written and
| project it on a wall. Mostly the other devs would listen and
| learn, and any suggestions they made would be strictly optional.
| The dev might leave feeling a bit embarrassed about something -
| but never feel "under the thumb" of a peer who was forcing an
| issue.
| phyzome wrote:
| Some of my coworkers have a convention of prefixing messages with
| [request], [suggestion], [question], etc. in order to make the
| intent extremely explicit. This is particularly useful if you
| have people coming from multiple cultures with different levels
| of forthrightness.
| cs137 wrote:
| I like, "I would extract this to a separate function."
|
| 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.
| AdrianB1 wrote:
| Tone: if you write, cultural differences will always remove any
| tone; the subtleties will be lost in translation. If you talk to
| the people, the tone of the voice and the body language is more
| important than the words.
| cdnsteve wrote:
| I feel like this is a problem ML could solve for assisted
| feedback focused on positive tone, ml models scans your response
| and offers suggestions on phrasing.
| balves wrote:
| I think the only correct answer is "it depends"
|
| It depends on the relationship with the person who's code you are
| reviewing, it depends on the relative seniority differences, it
| depends on the project, it depends on the rest of the team
| culture, it depends on the rest of the company culture, it
| depends on how long you've had a working relationship with this
| person, it depends on who else might see the review.
|
| I've had working relationships with people where it was
| beneficial/easier to be super curt and to the point, because
| there was sufficient 2-sided trust that best interests were at
| heart. I've had working relationships where I had to take having
| "open-ended genuine curiosity" to the extreme because of fear of
| feelings being hurt.
|
| That being said, by default I take a question-asking tone to the
| code review process, with the intention of possibly learning
| something. It opens up the door to "being wrong" and allowing
| things to progress without incessant arguing. There are times
| when this approach isn't appropriate, but I think it's a
| reasonable default. Again, it depends.
| aristofun wrote:
| I wouldn't be happy working in the team where people constantly
| consider #6 as rudeness.
|
| This looks so unproductive and unprofessional to care too much
| about the tone vs content in programming business.
| marcinzm wrote:
| That's fair if you have the same stance for all social
| interactions. If you don't then it's odd since nothing about
| programming makes people less human and less impacted by the
| usual social considerations. Although even if that is your
| stance for everything it's likely not the stance of everyone on
| the team.
| izacus wrote:
| I've seen people hurt by 1-4 versions though - it's not clear
| for a lot of people (especially non-native speakers) that
| those questions coming from Americans actually represent a
| veiled command shrouded in faux politeness.
|
| I've seen junior devs from other countries get into trouble
| because they ignored these kind of "questions" since they
| actually thought they were optional questions and not
| expectations by other developers.
| swagtricker wrote:
| People can go ahead mod me down for this, but I honestly don't
| care at this point. Here's the best ways to avoid this:
|
| 1.) Quit doing PRs for feedback. Start doing pair or mob
| programming. Bonus points if you ditch PRs completely and do
| https://trunkbaseddevelopment.com/ instead while you're at it.
| Asynchronous code review via PRs is waste (in the Lean sense).
| 2.) If you still want PRs despite pairing & mobbing (or you're
| about to tell me you've never done it, you're team won't and/or
| whatever insert lame excuse here as to slam/dismiss it without
| trying), spend a faster 5 min via screen share and audio AND
| CAMERA! Do the PR like an old skool code review session
| live/remote + camera, but have the author ANNOTATE for their own
| notes what you've reviewed when they go back to revise. 80% of
| human communication is non-verbal - interact w/ voice & camera.
| Help them learn & build confidence, then you won't worry about
| your writing tone. 3.)
| withinboredom wrote:
| I feel like this is often overlooked.
|
| One morning, I did a code review before having any coffee. I
| basically used the latter two throughout the whole thing. I came
| into the office to find my coworker literally crying. Later in
| the day, someone else said it was the best code review they'd
| ever read... and asked me to come to their team...
|
| There was so much drama from that code review. That code got
| merged as-is to appease the author without a single one of my
| statements addressed (including security considerations), and no
| one else would review it. My manager was in a tight spot, felt
| sorry for him.
|
| I was literally just my pre-coffee blunt self. I usually use the
| first few versions in your list, which is probably why it hit my
| coworker so hard. We chatted and made up, but our relationship
| was weird after that.
|
| So, I'd say, just be consistent. Changing tones unexpectedly
| might affect your coworkers and politics in interesting ways that
| you might not expect.
| rjdamore wrote:
| People are sensitive. Directness is underappreciated. Inability
| to take critique is killing code quality. Try going to an art
| school. Critique is how we get better. Check your ego at the
| door and you'll go far as a programmer
| 59nadir wrote:
| > I was literally just my pre-coffee blunt self.
|
| I have a co-worker who's socially retarded as well and I can
| say that the vast majority of his comments that tend to rub
| people the wrong way are just badly phrased versions of
| legitimate opinions (that he sometimes should just keep to
| himself because no one asked). He's a better reviewer than you
| seem to be, though; probably because he can take the time to
| type out better versions of things he'd normally just blurt
| out.
|
| Edited to add:
|
| It's a massive chore to have a teammate that causes stuff like
| this and I don't envy your teammates or lead (even worse if
| you're the lead, obviously).
| withinboredom wrote:
| Considering this story is over a decade old, and I was in my
| late teens ... I'd hope that I've grown quite a bit even
| though you're using the present tense like you know me.
| fknorangesite wrote:
| The way you told the story it comes off as much more
| recent.
|
| > I'd hope that I've grown quite a bit
|
| I hope so too, because "I hadn't had my coffee yet har har"
| is a pretty thin excuse.
| withinboredom wrote:
| > because "I hadn't had my coffee yet har har" is a
| pretty thin excuse.
|
| I'm pretty blunt/rude in the mornings, within half an
| hour of waking up, even these days. Back then, I did not
| know that about myself -- or rather, just how rude I came
| across.
|
| Most people I interact with never see me like that, but
| it generally takes me 10-15 minutes to get some coffee
| going. Hence why I said "before coffee" since I assume
| nearly everyone is a little weird within the first 30
| minutes of waking up, which also tends to be before a
| morning coffee.
|
| People who wake up "ready to go" seem to be pretty rare.
| Granted, my number of people I've had the opportunity to
| be around when they wake up is only a few hundred people
| (basic training, etc).
| daveevad wrote:
| > socially retarded
|
| I used this term all the time growing up and I'm trying to
| stop because it's definitely offensive to some people. Have a
| good day.
| rjdamore wrote:
| That's retarded
| tayo42 wrote:
| Crying? Did you leave some Linus Torvalds level feedback, like
| tell them to find a new career or something?
| ianmcgowan wrote:
| You sound like a Dutch person. Working with different cultures
| I've had to adapt to varying levels of directness but like to
| think it's helped me become more up-front. I've learned to
| appreciate people that say what they're thinking, so you can
| avoid the 2nd and 3rd order analysis about intent and thoughts
| about how other people will receive things.
| withinboredom wrote:
| Is it ironic I moved to the Netherlands and feel right at
| home here? (this story is from when I lived in the US)
| comprev wrote:
| I found the Dutch directness a breath of fresh air when
| working in NL.
| mythrwy wrote:
| Wow not excusing hurting people's feelings, but if you are
| literally crying you have become too identified with your code
| and possibly your job. Or else, you just cry too easily and
| that's a problem when working with others also.
| dominotw wrote:
| Is this person working on his own. Why can't people pair and
| give each other at the moment feedback vs doing code review.
|
| Person is already invested in his code by PR review time and
| doesn't really want to do PR ping pong for their task. What if
| you come back with more comments after they address your
| comments. Will they ever get to finish their task or forever be
| hostage to you subjective comments.
|
| Most of it is usually subjective too. you had to explicitly
| mention 'security issues' which imply that you have a feeling
| that most of your pr comments were your subjective
| interpretation .
|
| People are more open to suggestions during pair programming
| because people don't deliver feedback in cutting way ( with or
| without coffee) and its easy for get a common understanding and
| move forward.
| withinboredom wrote:
| > Why can't people pair and give each other at the moment
| feedback vs doing code review.
|
| That was something we discussed at various points. We never
| tried it when I worked there.
|
| > you had to explicitly mention 'security issues' which imply
| that you have a feeling that most of your pr comments were
| your subjective interpretation .
|
| Aren't almost all code review comments? I can't think of a
| single one except ones that point out literal bugs. Most are
| "do it this way because I think it will be more maintainable"
| or "do it this way because that is how we do it here." In
| this case, I mentioned "security issues" because they were
| literal bugs that introduced new attack surfaces that didn't
| previously exist. When I read comments on my code, I make
| sure to read it without any inflection. Some people have
| really bizarre code review styles, and some are totally
| straightforward. Some people are lenient on whether or not
| there actually needs to be a change, while some people will
| not accept code until changes are made, no matter how little
| the change request is.
|
| Knowing your reviewer is about as important as how you review
| code, which is why I suggested being consistent. If you're
| always straightforward, and then suddenly not, the reviewee
| is going to wonder if something is going on. Vice-versa, if
| you're suddenly straightforward, people are going to be
| offended or feel like you are taking something out on them.
| In my case, reviewing code within 3 minutes of waking up was
| a terrible idea and I have never done it since because I'm
| "grumpy-mc-grumpy-pants" for at least 10-15 minutes after
| waking up and having some coffee.
|
| I feel like a lot of the conversation in this thread is about
| how to review code, but the relationship between the reviewee
| and the reviewer is more important than any of that and how
| you review code _will_ affect that relationship whether you
| want it to or not.
| muzani wrote:
| Yeah, I think code reviews strike deep for whatever reason. I
| don't think being blunt works here even if you can be blunt
| elsewhere.
| dyingkneepad wrote:
| The different ways you're expressing the question give different
| meanings to your review. It's not clear to me what you want: are
| you blocking the review on extracting the code? Does this have to
| be done now or could this be on a separate patch? After or before
| the series? Is there an actual good reason to extract it (like
| you plan on using it tomorrow) or is this just about making the
| code easier to read?
|
| - Try to make this not personal (opposite of how I started the
| paragraph above!). Talk about the code, not about what the author
| did. The code does this, the code does that, instead of you wrote
| this, you wrote that. Also, it's our code, so "we should do this"
| instead of "you should do that" in case you can't use "the code
| should do that".
|
| - Be clear on what's just nice-to-have and what is really
| blocking the review process.
|
| - Explain to them why you think this should be extracted to a
| separate function. Maybe they have a good reason against it or a
| good counter argument.
| graton wrote:
| For code reviews we try to automate as much of the nit-picky
| things as possible. So for example in our Python code we run
| checks using: black, isort, flake8, mypy, and pylint. For
| reviewing. I try not to use the word "you" as I feel it can make
| people feel defensive. So for your options above I would like:
|
| Should this be extracted into a separate function?
|
| As a code reviewer I may think it is obvious that it should be in
| a separate function or something else that seems obvious. But
| often times the author has already tried that and had issues with
| doing that. Thus they did it the way it is because of that.
| pclmulqdq wrote:
| What do you mean to say when you want to review code?
|
| Personally, I have roughly three distinct levels of things that I
| comment on in code reviews:
|
| 1. Does not need response, but is my personal opinion about how
| the code probably would look nicer. In this case, I would do #2.
| If you just say "no," that's fine. I can revisit the issue later.
| Some people think these comments don't actually belong in a code
| review, and I think it depends a lot on your relationship with
| the code and the person asking for review.
|
| 2. Needs response, but is potentially open for discussion instead
| of a change to the code. I would do #4 here.
|
| 3. You must make this change or I will not accept. I would
| personally do #6. There is no need to use "non-confrontational"
| language with many people when you mean to be confrontational.
| There is a hard line about what you will and won't accept. If it
| is a major comment, you need to explain your rationale here out
| of courtesy to the person sending you code.
|
| Most things that fall into the third category are around code
| style conventions and minor bugs. More substantive changes, like
| changes to an architecture, often fall into the second category.
|
| Edit: Also, when I nitpick your code for one reason or another
| (which generally only happens when I am enforcing a company style
| guide), I will often say "Nit:" and then use the sixth style. If
| you are new to the team, I will quote the guide to you. Most
| people don't feel so bad about the nitpicks when you admit that
| you are nitpicking, and that it is for a reason.
| camgunz wrote:
| Yeah I agree w/ you. I think there are three major problems w/
| code review:
|
| - reviewer ego
|
| - author ego
|
| - reviewer ambiguity
|
| "Solving" ego is involved, but easy: you have to encourage
| trust amongst your team, and that can't happen in code review--
| it's too late.
|
| Reviewer ambiguity (being non-confrontational and wishy-washy
| when you really mean "this is an XSS vuln, you gotta change
| it") is an attempt to solve ego, but always fails.
|
| I'm not saying be a jerk, you have to act with compassion and
| empathy always, which is a marathon and not a sprint. But
| you're not gonna fix ego/trust/respect issues at code review
| time, no matter how deferential you are.
| mynameisvlad wrote:
| > Edit: Also, when I nitpick your code for one reason or
| another (which generally only happens when I am enforcing a
| company style guide), I will often say "Nit:" and then use the
| sixth style. If you are new to the team, I will quote the guide
| to you. Most people don't feel so bad about the nitpicks when
| you admit that you are nitpicking, and that it is for a reason.
|
| This. We do it on our team as well. Especially when it is
| smaller things like spelling, indentation, etc, I might have
| quite a few nits in one PR, so it makes it a little bit easier
| on the receiving end when you see a dozen comments but they're
| all marked as nit.
| 3minus1 wrote:
| +1 to the comment about using "nit:". I also don't think it's
| fair to block code changes for style/quality/whatever that is
| the same or better then the existing code base. If it's not
| making the existing codebase worse on average than don't block.
| exac wrote:
| I prefix my nitpicks with the same, but with the 3rd style.
| amalcon wrote:
| My usual approach is to point out the problem I'm trying to solve
| rather than suggest a specific solution. I also like to phrase as
| a question about practicality. Sometimes the answer to that
| question is "No", and I want to make sure that even someone very
| junior is comfortable giving me that answer.
|
| - This code looks pretty similar to this other code; is there a
| way it can be deduplicated?
|
| - I don't understand this code; I think it's because there are
| too many conceptual steps. Is there some way to structure it for
| better readability?
|
| - Is there a way to unit test this specific behavior?
|
| Sometimes this style doesn't work (e.g. correcting a comment that
| has become incorrect due to the change), and in those cases I
| usually just say "Please do this." I also try to point out things
| I _like_ when they are present.
| kevin_nisbet wrote:
| There's definitely a lot to good code reviews. One addition
| perspective I've started taking that I haven't seen so far in the
| comments is a "Have you considered this alternative approach type
| question". And to describe the alternative, show it, etc. And
| this ensures there is an option to basically reject the
| suggestion, a valid response is I've considered and rejected that
| idea or approach.
| weitzj wrote:
| Should we,
|
| Could we please
| MichaelMoser123 wrote:
| Smatbear has a great article on code reviews
| https://smartbear.com/blog/avoiding-the-politics-of-code-rev...
|
| He has the following suggestions: - "Ensure
| that reviews are two-way. Never have people who only review and
| people who only get reviewed." - "Always focus on the
| code and not the person who wrote the code." - "Make the
| reviews small, frequent, and informal. Marathon group sessions in
| rooms make people defensive." - "Frame things as
| questions and suggestions rather than orders and accusations. Ask
| that others do the same." - "Automate as many checks as
| possible so that reviews don't focus on simple details."
| - You can frame the review as optional "asking for advice"
| instead of a gatekeeper approach of "getting the code approved"
| - Says that the potential harm of the bad approach is worse than
| taking up the risks, that is taking the risks that come with the
| policy of not requiring a code review for each and every commit.
| hedora wrote:
| It depends on who the corworker is, how well I know them and how
| long the review is. My comments range from very polite to "???"
| "delete this" or "don't copy paste this".
|
| Once I have worked with someone for a while (and perhaps have
| gone out for beers / coffee a few times), I write shorter
| comments.
|
| At that point, they've usually figured out that I mean well, but
| am blunt.
|
| For new hires, I spend a lot more time spent on "why" and tone.
|
| If my team set guidelines for this, that'd be a strong signal of
| much, much deeper dysfunction and I'd consider switching jobs.
| no-dr-onboard wrote:
| I do code reviews as the meat of my job and have personally
| benefited from GitLab's handbook article on this topic [1]
| - No ego (Don't defend a point to win an argument or double-down
| on a mistake.) - Assume positive intent (If a
| message feels like a slight, assume positive intent while asking
| for clarification.) - Get to know each other
| (Building a rapport enables trust.) - Say thanks
| (Taking every opportunity to share praise creates a climate where
| feedback is viewed as a gift rather than an attack.)
| - Kindness (It costs nothing to be kind, even if you do not
| believe someone deserves it.) - It's impossible to
| know everything (You can't know how your words are interpreted
| without asking.) - Short toes (GitLab is a place
| where others can feel comfortable with others contributing to
| their domains of expertise.)
|
| Specific to your situation, I'd focus on what others have said as
| well. If they *need* to perform something as part of their job
| role, remove the suggestion and place an imperative. Ex: "This
| needs to be a separate function" vs "You should. . .".
|
| > Communicate assuming others have low context. We provide as
| much context as possible to avoid confusion.
|
| It also helps to provide context, especially if you have a
| particular fix in mind. "See this PR/MR for someone who
| encountered a similar situation recently."
|
| Hope this helps.
|
| 1. https://about.gitlab.com/company/culture/all-
| remote/effectiv...
| EVa5I7bHFq9mnYK wrote:
| Trying to never use the word "you", it seems a bit aggressive.
| Using "we" or "one" instead, or rephrasing to get rid of pronouns
| completely.
| presidentender wrote:
| I like "Consider extracting this to a separate function."
| b0sk wrote:
| second!
| hcarvalhoalves wrote:
| My rules for reviews (which influence the language used) are:
|
| * Criticise the code, not the author ("There's a bug here" vs.
| "You inserted a bug here");
|
| * Reach common agreement that people shouldn't cling to code
| written ("sunk cost fallacy"). Code is liability, and it's okay
| to throw things away and restart from scratch, the value is in
| the learning;
|
| * Don't use questions that invite discussion if you're not
| inviting a discussion, instead expose the reason you believe
| something should be different ("Should this be like X?" vs. "I
| think this should be like X for reasons A, B, C."). This avoids
| wasting time and reviews that turn into long discussions.
|
| * Don't give orders ("Do this", "Do that") to your colleagues,
| since they are not your employees, and it doesn't help promote
| learning & growth by not exposing the reasoning behind the
| demand;
|
| * Avoid bike-shedding about style/formatting/etc. If this is
| important where you work, automate with tools;
|
| * Agree previously on what constitutes a non-passable review, if
| there's such a process.
| thewebcount wrote:
| > Avoid bike-shedding about style/formatting/etc. If this is
| important where you work, automate with tools;
|
| I'd love some suggestions for how to get people to avoid this
| type of bike-shedding. While I'd love for us to use some tools
| to automate this, we have a huge codebase with existing code
| that doesn't follow the rules, and it's not in my ability to
| implement said tools. (Big company, lots of overhead, will
| affect a lot of people, don't have a lot of time, not my
| department, etc.)
|
| One of the things I hate is having a different comment for
| every single place where I misplaced a star or ampersand. My
| natural tendency is to do it one way, but our style guidelines
| prefer it the other way, so it's something I miss frequently.
| Having 30 comments in a review that are all "move the star to
| the other side" over and over again just makes me hate the
| reviewer. (Particularly if they don't provide any useful
| feedback.) A single comment on the first one that says
| something like, "This doesn't match the style guidelines, and I
| see several others that don't match. Please fix them all,"
| would be highly preferable. And if, god forbid, I miss one,
| just get over it. It can be fixed later. It's not nearly as
| important as having correct functionality in our app, and I'm
| pretty busy!
| woojoo666 wrote:
| Set up the linters to only lint code that was changed. This
| way legacy code doesn't need to be fixed all at once, but
| will slowly get fixed over time
| isbvhodnvemrwvn wrote:
| Are you sure it's not in your ability? Whenever I introduced
| something like that people were in general indifferent-to-
| happy, as long as you did the majority of the job and they
| could somewhat easily run those tools (e.g. anything written
| in Java is more or less a lost cause because people cling to
| their IDEs)
| 0xffff2 wrote:
| Sorry, but formatting is on you. If you don't like the
| comments, don't commit inconsistent code. Set up a local
| auto-formatter with the right rules. If enough of the
| existing code you're working on doesn't conform, set the
| formatter to only run manually and get in the habit of
| running it before committing.
| physicsguy wrote:
| Tools are easy. You add it to the CI/CD pipeline, it fails if
| it doesn't meet the guideline, people have to run the tool to
| get the merge in. On first run it's painful, so if you're not
| starting a project, use an autoformatter so it's not all
| manual work. If you really want, you can even make your CI
| pipeline push commits with autofixes
|
| Of course if you don't have a CI pipeline that's easy to
| integrate steps like this, you've got major issues to deal
| with elsewhere...
| de6u99er wrote:
| That's good advice!
| notyourwork wrote:
| I agree in general, though I find the second on clinging to
| code difficult when people counter with timelines. It can be
| difficult to express to juniors how to reason about taking a
| bit more time to do it "right".
| Morgawr wrote:
| > * Don't give orders ("Do this", "Do that") to your
| colleagues, since they are not your employees, and it doesn't
| help promote learning & growth by not exposing the reasoning
| behind the demand;
|
| I think this depends a lot. I do "readability" reviews at my
| company to make sure people apply good coding standards and
| adhere to the coding guidelines that we have. A lot of these
| are very "mechanical" in nature and when I am reviewing changes
| that have obvious coding standard mistakes I will write
| comments like a "linter" would. One of the most common
| examples:
|
| > Use descriptive ("Uses") rather than imperative ("Use") style
| for function docstrings. See: <url-explaining-docstring-
| guidelines>
|
| I think this type of "giving orders" in reviews is fine as long
| as it's clear this is an obvious misuse/misunderstanding/miss
| of the readability guidelines. For any other kind of comment
| I'll phrase it differently ("Is there a reason why you did X? I
| think doing Y might be more appropriate because <reasons>, what
| do you think?", etc) but for stuff that linter should've caught
| I don't think it's necessary to be verbose and "sweeten" the
| comments too much.
| theycallmemorty wrote:
| Is this work you're collaborating on together (as peers) or work
| you're supervising? Are you in any sort of relationship where you
| could be considered to be 'mentoring' the developer?
|
| When collaborating, I like to provide my feedback in a way that
| allows the person to provide their own perspective.
|
| "What do you think about moving this to a separate function so
| that we can keep each functions in the class small and focused on
| one specific purpose?"
|
| When supervising you of course want to allow two-way-
| communication but you can be more direct, but make sure to use
| the correction as a teaching moment.
|
| "Please move this block to a separate function so that we keep
| the functions in the class small and focused on their one
| specific purpose. See abc.js and xyz.js for good examples of
| classes that follow this pattern."
| ilrwbwrkhv wrote:
| Consider extracting this to a separate function.
| kelseyfrog wrote:
| After reviewing my last few code review comments, this is my
| most frequent phrasing too. I get that questions are a way to
| soften the bluntness of a code review comment, but I've been
| hugged to death by this sort of niceness in the past. It was
| not fun.
|
| "Consider ______" has the directness I, myself, enjoy, and
| whose response can take the form of a change, a reply, or a
| jumping off point for discussion. It indicates some action
| needs to take place without specifying exactly which one and
| leaves me open to having my mind changed if the response is
| "I've considered it and actually ..."
|
| Still, if there is ever a flaw so great that the code simply
| will not work without a fix, I will phrase the fix in the form
| of a command.
| m3Lith wrote:
| 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".
| JamesSwift wrote:
| If its not just asking for clarification on a comment then I
| really try to stick to the PR, so that the context is captured.
| If a back and forth is needed then I make sure to include a
| recap somewhere in the PR.
| ysavir wrote:
| Remember to be _problem oriented_ rather than _solution
| oriented_. If you just tell people "I think this should be the
| solution" or "you should change it to this solution", you aren't
| working with them, you're instructing them. And a key to
| teamwork, especially reviewing, is a cooperative spirit, not an
| instructional one.
|
| If instead of saying what you would do, or what you think they
| should do, you express a concern about the code, the tone is
| completely different. Now it's just saying something like "do you
| think repeating this across files will be problematic?", which
| allows the other person to be the determiner. If they agree they
| may well do exactly what you would have suggested, and if they
| don't, it can start a discussion that helps bridge together how
| various team members code. It's win/win.
|
| It also helps people understand what you aim for when writing
| code, which helps them keep in mind your needs in the future (and
| vice-versa for you to keep in mind their needs).
| quelltext wrote:
| > "do you think repeating this across files will be
| problematic?"
|
| If you write that you clearly think it's problematic but hide
| it behind an unnatural question. Then either I do what you hint
| at or you will push / discuss until I agree. I am not the
| determiner. It sounds like you are schooling me with an awkward
| back and forth.
|
| Honestly, to me this is beating around the bush and doesn't
| actually make me confident we can have a real discussion (in
| the context of that code review).
|
| "Do you perhaps think that doing this <tylically discouraged
| coding practice> is problematic?" sounds like either you _want_
| to passively insult me, or you don 't consider me a peer with
| whom to have an actual discussion on equal footing, or you
| don't have the guts to share your opinion without contortions.
| ysavir wrote:
| The wording above could be improved for sure. But the
| intention is to shift focus away from telling somehow how to
| solve a problem to forming consensus on whether there's a
| problem at all. And that starts with the reviewer expressing
| a concern rather than immediately providing a solution. "Is a
| change needed?" should come before "this is a needed change".
|
| A better example might be "I'm seeing this same code in
| various places--do we benefit from keeping them distinct?" or
| "you took an inheritance approach here, but I'm concerned
| about the long-term impacts as the codebase changes. How do
| you think this code compares with a composition approach?".
|
| If in doing so you would think that I'm being sly, that's
| unfortunate, but there's not much I can do in how you
| interpret stuff. I can't control for others assuming bad
| faith. And if there's bad faith, there are probably
| communication and/or trust problems amongst the team members
| that are broader than code reviews, and is something that
| should be tackled directly.
| EuanReid wrote:
| Obviously tone is hard in text, and it's worse in multilingual
| teams. What's been very effective for my current team is adding
| explicit context as a comment prefix with standardised terms: -
| Nit for "it doesn't actually matter" - Suggestion for "this
| approach might be better, what do you think?" - Question for
| "help me understand this" - Requested Change for "this is
| actually a blocker"
|
| We include this expectation in our working agreement, stick to it
| rigorously for reviews where reviewer and reviewee haven't
| established a strong rapport yet, and use it as appropriate
| beyond that. So far we've had no confusion about tone in reviews.
| agentultra wrote:
| I think it depends on your teams' expectations for code review.
| It seems like perhaps your team doesn't define what is
| acceptable/constructive/valued, etc from reviewers.
|
| Personally I prefer the first one on this list: it gives the
| author the opportunity to refuse the suggestion. Perhaps its a
| frivolous request and the PR is already been open for far too
| long. Maybe the author would prefer to leave the duplication
| there until there's more justification for extracting it to a
| function. Who can say?
|
| Not all feedback is necessarily good or useful.
|
| Personally I don't bother with suggestions like this where it's
| more of a style preference unless there is a style guide to
| adhere to where "extracting this to a function" would clearly
| connect to a guideline.
|
| I tend to focus on verifying the that the author did their
| homework: what evidence/proof did they submit that ensures me the
| change is correct wrt. specs/requirements/tasks? If that
| evidence/proof is sufficient that's all I care about: it makes it
| easier to manage a higher volume of review requests.
|
| I have a pet peeve for suggestions that are "trivial" and based
| on, "well I would have written it this way," as I don't find them
| terribly useful most of the time. They rarely affect the
| specification of the program and their claims to
| readability/maintainability are often dubious and superficial.
| Bike shedding is a huge waste of time in the review process. At
| least when worded with, "Should we..." there's a chance for the
| author to explain whether they had already considered that or
| accept the suggestion as helpful and make the change.
| radus wrote:
| "Please extract this to a separate function"
| DonHopkins wrote:
| "Don't repeat yourself (DRY)."
| Tade0 wrote:
| I've learned that commands not phrased as such usually aggravate
| people the most - especially #1 from your list or questions where
| the one questioning knows the answer and wants to suggest
| something.
|
| If I don't know how to phrase my comments I just schedule a call
| and we go over the code together. Much less time spent than on
| comment ping-pong.
|
| If the code generally works, but other aspects, like
| maintainability, would benefit from some rework I use the phrase
| "You can...", e.g. "The second argument to the `map` callback is
| the element index. You can use it to create this range instead of
| using an external variable incremented on each step."
|
| The recipient has the possibility, but is not actually forced to
| do anything, just like with code suggestions. In the vast
| majority of cases they follow my advice though.
|
| If there's an error or a kludge I use:
|
| -Code suggestions - no comment, so nothing to get upset over and
| if the recipient is feeling particularly irritable, they can just
| ignore it.
|
| -Direct language in imperative form, e.g. "make sure that X,
| otherwise this won't work as intended", "avoid X in Y", "use Z
| here (documentation link)".
| [deleted]
| Sjeiti wrote:
| I mostly only ever use the form: "This should be ... " with a
| possible explanation why. We're all mature enough to not take it
| personally, so I'm not going to beat around the bush by writing a
| lot of proza. The imperative of the last example ("Do this")
| leaves no room for argument (because I might be wrong).
|
| Either that or I ask for clarification of a piece of code: "Why
| did you do this instead of this?".
|
| Like already stated by someone else; your approach really should
| depend on the team you're in. I'd take a less direct approach
| with junior devs who tend to be a bit more insecure (and
| stubborn).
| mannykannot wrote:
| None of the suggestions contain any justification for taking the
| action. In many cases a reason may be obvious, but as it is not
| the case that every opportunity for creating a separate function
| should be taken (for one thing, doing so can make reading and
| understanding the code more difficult), this becomes an issue of
| judgement.
| vsareto wrote:
| 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 separate function?
|
| >* Could you extract this to a separate function?
|
| These are essentially the same: opening a dialog over an opinion.
| I'd suggest this when there's not an obvious flaw or rule
| violation or you have a gut-feeling about how code should be and
| want the author's input.
|
| >* I would extract this to a separate function.
|
| This is almost a command but isn't clearly one. It should be
| followed by some rationale, at least.
|
| >* This could be extracted to a separate function.
|
| This one's the least useful. You could do a lot of things with
| code. It doesn't resemble the command or inviting tones of the
| other examples here.
|
| >* This should be extracted to a separate function.
|
| >* Extract this to a separate function.
|
| These are commands and are practically the same tone and best
| when catching mistakes. Unless obvious, a rationale should be
| given like a demonstrable flaw in logic or inconsistent
| abstraction, etc. There should be a few sentences explaining
| this. If there isn't a clear violation, I'd prefer the dialog
| invitation tones.
| ajross wrote:
| > I'd switch the tone based on [whether you want a discussion
| or are insisting on a change]
|
| I think that's a great example of where "tone" is absolutely
| _not_ sufficient, especially in the modern world where many
| readers aren 't going to share your first language and won't
| absorb the nuance.
|
| If there's a situation where your disagreement is absolute, you
| need to say that factually, ideally with a recipe for how to
| resolve that included:
|
| "I can't approve this scheme, please do X instead."
|
| "This API isn't OK to use here, you have to..."
|
| "Under no circumstances will I ever approve of a feature that
| does this."
|
| You can phrase those as nicely as you want, but it's imperative
| that your disagreement be spelled out explicitly.
| colonwqbang wrote:
| It's sometimes refreshing to have people just write what they
| want me to do, clearly and without embellishment. Even if
| it's sometimes nitpicking, I know that if I just fix it I can
| then merge my patch. I don't have to wait for a second look
| from them because their comments were so direct "write this
| instead".
| jvanderbot wrote:
| Another thing that people don't often consider is that while
| some really appreciate being given a pointer to the rule as a
| justification (these are in a wiki / code style doc usually),
| many people will chafe at the "pedantry". In general, those
| second group need a bit thicker skin, but it is something to be
| considered.
| kevinpet wrote:
| There are different categories of things you might see:
|
| 1. This is wrong. I can tell from reading your code that it
| doesn't do what the description of this PR or the name of a
| test or some other indicator shows its supposed to do. This
| should be communicated in a tone of "you must not merge this".
|
| 2. This violates our agreed-upon style or best practices. The
| strictness of enforcement is part of that agreement and company
| culture. At my current company, this would be communicated
| along the lines of "this is not how we prefer to do this, so
| unless you have a good reason why our standard method is wrong,
| change it"
|
| 3. This is confusing to me, or I have a suggestion for
| improvement. This should be communicated as a suggestion or
| general feedback. If you're getting miffed about people not
| taking the suggestions, maybe it's actually in a category
| above, or maybe you need to adjust your own assumptions about
| how much stylistic consistency to insist on, since it sounds
| like you don't have a consensus.
|
| 4. You are new, either to software engineering or at least to
| this company, and your style is inconsistent with how things
| are done in this code. This is the same as #3 but should be
| communicated more strongly and depending on your company
| normals may be effectively a requirement.
| jorgeleo wrote:
| <sarcasm> I like your suggestions... I would add that they are
| more effective if you keep a background noise with whips
| cracking sounds and random screams </sarcasm>
| edmcnulty101 wrote:
| This is really useful. Thanks.
| m3Lith wrote:
| What about clarifications? In a sense when you're not sure if
| this is good code, and would need to get more details. I
| sometimes just ask directly ("Why is this there?"/"What does
| this do?"), but other times I just don't bother and let others
| review.
| matsemann wrote:
| I also think it depends a bit on the other person and the
| rapport one has with them.
|
| At least I try to remember back to when I was a junior, and how
| "personal" the feedback felt when I was just starting. So when
| reviewing for others in the same situation I make extra sure it
| doesn't come off the wrong way by adjusting my tone.
|
| But for someone I've worked with for years we're often a bit
| shorter and to the point, and everyone's happy.
| ginja wrote:
| Yeah I remember being a junior and feeling like blunt
| comments were kind of rude. So I make an effort to be nicer
| to new developers - sometimes simply adding "what do you
| think?"/"do you agree?" is enough to make those command style
| sentences sound much less blunt, and also encourages them to
| ask questions instead of blindly following my suggestion if
| they don't fully understand it.
| graftak wrote:
| To me those questions could come off as condescending. I
| say _could_ because I wouldn't think that's your intent,
| but the tone is there.
| kqr wrote:
| That depends entirely on whether you add the question as
| a formality or if you are genuinely interested in why the
| other person might disagree.
| eropple wrote:
| The best way I've found to introduce junior developers to
| code reviews is to pair on it. Just go side-by-side down the
| diff and hop into an editor when necessary. It also helps
| teach a developer _how_ to review code and what to look for--
| which has knock-on benefits of its own.
| spacemanmatt wrote:
| When you are reviewing, what is your authority level? Do you
| enforce a checklist? Are you merely asked to give your
| professional opinion? Are you a senior supervising a junior?
| These are critical inputs for me to calculate tone.
|
| When I've done reviews, I had some seniority and an
| organizational privilege to veto some code. I worked from a
| checklist and a goal (with which the checklist was meant to
| align, but we knew it was not possible to fully automate those
| aspects of review). These are my takeaways from that arrangement:
|
| Language like "declined" or "REJECTED" or "can't approve" is
| discouraging to the individual contributor. I replaced all that
| with discussion of why I can't allow that, under the obvious
| subtext of rejection. No need to just rub it in when there is
| learning to offer.
|
| When indicating required changes, especially where I was more-or-
| less handing them the replacement code, I always said please.
| Always.
|
| Most importantly, I gave accurate feedback. I took the time to be
| sure I was right before I wrote a review. Otherwise what's the
| point. Even simple patches got tested.
| livinglist wrote:
| "Won't it be better if we did that instead of this?" "Do you
| think it would be better if we did that instead of this" "I think
| it would be even better if we do this instead of that"
|
| I usually use "we" instead of "you" in my code reviews, "we"
| feels more friendly and comforting and way less judgy.
| ratww wrote:
| I agree. I tend to use "we" when for the negative stuff of
| anything that has to be decided by the team ("we changed the
| code here but it caused a regression", "should we refactor
| this?"), however I also like to use "you" for positive stuff
| and to give credit ("your change in b8e63dca also fixed this
| bug, livinglist").
| r_harriso wrote:
| "Obey!"
| LocalPCGuy wrote:
| Couple things I try to keep in mind when giving PR feedback
| specifically.
|
| 0) Give feedback with respect and just like as if I were sitting
| with someone face to face telling them these things. And keep it
| about the code, not the author.
|
| 1) Remember that I could always be wrong (whether from misreading
| something, not having the whole pictures, etc. - many reasons my
| suggestion may not be the right one).
|
| 2) Communicate how strongly I feel about a change - I use `nit:`
| to indicate opinions or annoyances that are basically "I wouldn't
| do it this way, but up to you whether to change it", slightly
| stronger language for things I think require changes, up to "I
| feel strongly about this, let's chat about it" for things I would
| not approve the PR before they (or my mind) are changed.
|
| 3) Phrase as requests and/or suggestions (could, would, prefer)
| over command (do X)
|
| 4) As others have mentioned, give the reasoning. Personally, I
| prefer request first, reasoning second, but I see many people
| expressing preference for it the other way. My reasoning is that,
| while the reviewee may read the reasoning once, the request is
| what they may come back for when fixing things, so ensure it is
| the first thing seen so someone doesn't have to parse through the
| reasoning again to find the specific request.
|
| 5) Provide examples if it's clearer to communicate that way as
| compared to writing it out.
| jjdin14 wrote:
| Depends on the situation. If it is a clear cut case or you're a
| mentor/senior to the reviewee, a more direct tone is fine. If
| it's a matter of opinion or it's a peer review then a more
| considerate tone would come across better.
| JamesSwift wrote:
| Default to short, polite statements, but make it clearly
| suggestive if its optional.
|
| e.g. "Extract this to a separate function please" if you aren't
| asking. "Should this be a separate function" otherwise.
|
| This also assumes that theres a certain protocol to PRs in terms
| of veto power. I've always followed the rule that the writer must
| make all requested updates by reviewer unless explicitly
| challenged. In other words, you can't silently not change
| something they mentioned, but you can say things like "thats out
| of scope for this update" or "I think this design is better
| because XYZ" and the writer is able to mark as resolved at that
| point.
|
| Sometimes this ends up with a conversation that needs to happen
| where the reviewer disagrees with the writer, but honestly it
| rarely does at least on the teams I've been on. It would really
| have to be a contentious point to escalate to that level rather
| than just being a difference of opinion.
| noname120 wrote:
| 1) State the specific reason that motivates you to ask for a
| change.
|
| ``This code is duplicated several times.''
|
| --
|
| 2) State the change you'd like.
|
| ``I'd suggest extracting it to a separate function.''
|
| --
|
| 3) (Optional) Expand on why you suggest solving the issue with
| this solution instead of another one.
|
| ``We could create a macro instead but it's not worth it as we can
| just let the compiler decide whether to inline the function or
| not.''
| softwarebeware wrote:
| My favorite guide on this is https://phauer.com/2018/code-review-
| guidelines/
|
| There are also some great things in the book, Debugging Teams,
| like the Humility-Respect-Trust (HRT) concept.
| Galxeagle wrote:
| In addition to other comments saying to add reasoning, I'm a big
| fan of the 'MoSCoW' method (must/should/could/would) to remove
| ambiguity around if things are a question vs a command vs a
| thought. 'You must do x' sets a clear condition before you
| approve a PR. 'I would have done y' shares perspective and
| experience without blocking when you think you might be just bike
| shedding.
|
| I'll usually link something like this page to the repo and align
| on the meanings of each phrase.
|
| https://en.m.wikipedia.org/wiki/MoSCoW_method
| k1ll3r wrote:
| only suggest improvements, and provide reasons if possible, never
| tell anyone they should do something in a certain way unless it's
| an obvious bug. I fucking hate know it alls that confuse their
| opinions and preferences with some absolute truth.
| spiderfarmer wrote:
| I'd say the last two are best, but I'm Dutch.
| new_newbie wrote:
| Don't forget a big part of this is the dependent on the image of
| you the recipient has in their head.
| _greim_ wrote:
| Personally I have two tones I take in a code review comment: the
| "Should ... ?" form for when I want to raise awareness but
| otherwise leave it up to the other person, and the "I recommend
| ..." form for when I think there's a real issue. Especially in
| the latter case, I'll back it up with a real-world scenario of
| how it could cause a problem; either a bug, difficulty reading
| the code by future maintainers, or making it more likely to
| introduce a bug when the code gets refactored later. Finally,
| back-linking to documented standards if necessary to avoid the
| appearance of a me-against-you type of thing.
| symby wrote:
| Perhaps, silence would be a good choice on this issue?
|
| To my view, coding is as much art as science, and it is
| limitlessly fascinating to me how different people find different
| ways of expressing ideas and solutions.
|
| Also, the code review process is fraught with opportunities for
| insult, misunderstanding and unfortunate power dynamics. It is
| inherently difficult regardless of the actual content being
| reviewed.
|
| On the other hand, if there is a significant issue here "I am
| having trouble following your thinking here. Perhaps dividing
| this up into smaller functions would help?" Might be a good
| review comment?
|
| The style wars are very tempting to engage in, but they virtually
| never drive greater productivity, real quality improvements, or
| positive team dynamics.
|
| If there are _real_ reasons that this wants to be broken out into
| a separate function (reusability for instance), then make that
| clear in your suggestion.
| OJFord wrote:
| I second guess myself and edit a lot, especially relative to how
| much we actually do code review, so I'm interested to see others'
| thoughts.
|
| I would say though that a lot depends on team dynamic &
| hierarchy. For example, I have no direct reports, so I'm never
| going to use the imperative as in 'extract this to a separate
| function' - taken literally, I don't have the authority to tell
| anyone to do that.
|
| Most often I think I favour 'I would', and give a reason. I.e. I
| found this a bit surprising, because I would have done it
| differently.
|
| Along the same lines I also ask (honest) 'is there a particular
| reason' or 'why not' type questions. Something else seems most
| obvious to me, but maybe they thought of (even tried) and
| dismissed that.
| alexashka wrote:
| It's about power dynamics.
|
| You ask someone above you. You tell someone below you. You
| discuss with someone on the same level as you.
|
| If you're smart, you'll quickly spot the assholes who think
| they're above others - you probably shouldn't comment on their
| code at all, other than 'great, looks good'.
|
| If you think someone is below you, you're an asshole.
|
| If you want to discuss, you need to include the word 'because'.
| 'Hey, can we do X, because I/we will need it for Y?' is the way I
| usually go about it. By the way, discussions are best had
| _before_ anyone writes code, not after. So for me, I don 't do
| code review - if someone's bad enough that they need their code
| reviewed on a regular basis, I will be leaving soon anyway.
| simonbarker87 wrote:
| I've introduced Must, Should, Could at every company I've been at
| (except the one where I picked it up myself) and it works
| wonders.
|
| Prefix every suggestion with M, S or C and then just write the
| suggested change as a plain statement. The prefix handles the
| severity and importance without you having to worry about tone.
|
| Coulds can be ignored by the coder author with no explanation as
| to why they are ignoring but if they have time or want to
| implement the change then they can. Shoulds can only be ignored
| with a reason and the reviewer has to agree or the change should
| be made. Musts have to be implemented and should be used rarely.
| The only person who can over rule a must is a department head,
| lead or CTO type person.
|
| This massively reduces friction in the CR process.
|
| Article here: https://allthecode.co/blog/post/how-to-get-better-
| at-code-re...
| darekkay wrote:
| We do something very similar:
|
| * NICE: Optional change. PR is approved.
|
| * SHOULD: Highly suggested change. PR is neither approved nor
| rejected.
|
| * MUST: Must be fixed. PR is rejected.
|
| This also helps the PR author to know which of my comments have
| to be addressed to resolve my rejection.
| evsasse wrote:
| Yeah, that is great.
|
| Learned something similar on a previous job. Prefix it with
| "#must", "#should", "#could", "#would".
|
| We've also extended it with some other things that you may want
| to comment, but are not necessarily actionable. "#wont",
| "#idea", "#tip", "#question", "#risk", are pretty much self-
| explanatory.
|
| "#fun": an ugly workaround that could've worked; some funny
| situation that the change could cause; ...
|
| "#domain-knowledge"/"#debrief": really nice if you are working
| with people that are more junior, or not that familiar with
| this codebase in particular. May help other reviewers
| understand better why the author made certain decisions.
| [deleted]
| glerk wrote:
| I tend to be overly "nice" in code reviews and phrase things more
| like suggestions and provide some justification for them:
|
| > have you considered x? It is standard practice in y to do z.
|
| > should we do x instead? I think it would be better in the long
| run because y.
|
| If there is a more serious issue, I prefer DMing the author and
| hashing things out synchronously.
|
| I think it is important to phrase things gently as much as
| possible, as tone is hard to read from text, and being overly
| harsh might discourage other teammates from taking risks and
| asking for feedback.
| kissgyorgy wrote:
| All of your examples are wrong, because you don't state WHY you
| think it would be better. Never give instructions without at
| least a short explanation.
|
| Better tone of the above sentence, without "blame game":
| None of your examples include WHY you think it would be better to
| do something. I never give instructions myself without explaining
| at least briefly why I think my suggestion is better, because
| they more likely understand the problem or accept my suggestion.
|
| Even if you think something is better only because your taste,
| it's still better to explain yourself, so you can agree to
| disagree. I usually let go small things if something is readable
| and functionally not different.
| spockz wrote:
| Typically I say something like:
|
| "What would be the advantage of using X instead?" If I'm not
| entirely sure or fine either way.
|
| If I think X is really the way to go I'll say something like:
|
| "Doing X would have these benefits. The benefit of Y is such and
| such. So X is preferred for ..."
| lhnz wrote:
| "How would you feel about extracting this into a separate
| function? It might be better because the logic seems quite self-
| contained, and we can then apply the extra business logic
| immediately after this with a few if-elses."
|
| I try to give critiques in this way, as it gives the person that
| wrote the code ample opportunity to defend their approach, but
| also gives explanation of why your suggestion might be better.
| melezhik wrote:
| Code reviews are tough when people have strong opinions. I try to
| ask questions rather then making statements, it gives a room for
| discussion and makes the whole process less challenging
| jdmoreira wrote:
| "I think we should..."
|
| "Maybe it would be better..."
|
| "IMO we should do ..., what do you think?"
|
| You really want to be on this person side and help them achieve.
| That's your job after all
| koolba wrote:
| Depending on the situation, one of the following three:
|
| > Should we extract this to a separate function?
|
| When you are not familiar with the codebase, potential reuse, and
| want the submitter to clarify rationale for the approach.
|
| > This should be extracted to a separate function.
|
| When you are the authority but you're open to a discussion. You
| should include the "why" aspect in this situation.
|
| > Extract this to a separate function.
|
| When you are the authority and there will be no discussion. If
| you want you can include the "why" but it's a waste of time. You
| are the law.
|
| The rest are sugar coated versions of the same thing.
|
| " _I think..._ ", " _I would..._ ", " _Maybe we..._ ", are
| unnecessary additions. Clearly it's what you would do as you're
| the one giving the review.
|
| If people are too thin-skinned to take direct feedback then they
| should work on improving that before submitting more code. Life
| is too short to waste time picking words that try not offend when
| you can spend that time actually producing something of value.
| magnusekdahl wrote:
| When it comes to feedback, ensure that you give the engineer
| recieving the feedback the maximum potential to grow to ensure
| the engineer does a better job next time.
|
| Its not the tone but rather the feedback style according that
| should be adapted to the knowledge/motivation of the engineer in
| the situation. For example by using the
| https://situational.com/blog/the-four-leadership-styles-of-s...
| herustic:
|
| 1: Engineer is junior in the context and insecure/unmotivated: Do
| X 2: Engineer is junior in the context and motivated/secure: Take
| the decision for the engineer and explain why 3: Engineer is
| senior in the context but insecure: Coach by open ended
| questions: how large of a function do you think is appropriate?
| 4: Engineer is senior and motivated/secure: From your perspective
| how should the function be and how should we do this in the
| future to reach our goals.
| izacus wrote:
| First decide what do you want:
|
| * Is the change mandatory for your approval? Then be polite and
| firm with reasoning - "This is poorly readable and doesn't
| conform to our code style chapter 5. Please extract this into a
| separate function.". Don't mess around with "Should, Could, I
| would" starters when it's not a question - be clear what you're
| asking.
|
| * Are you trying to start a debate? Then ask a question properly
| - "I think this is unreadable because of A, B, C. Do you think
| extracting this will help readability?" Again, be clear it's a
| question.
|
| * Are you saying an opinion and you're not very hung up on it?
| Mark it as such - I tend to put "Optional" or "Nit:" (team term)
| in those cases. "Nitpick/Optional: I think this isn't readable,
| if you have a moment maybe extract the method?"
|
| The worst you can do is veil your intent (order/question/opinion)
| into faux questions which will confuse people - especially if
| they're non-native english speakers. Adding background /
| explanation / context for your comment always helps too.
| nsonha wrote:
| isnt the first one good for everything, why even bother with the
| other options?
| corrral wrote:
| > * Should we extract this to a separate function?
|
| Reads very passive aggressive. No.
|
| > * Could you extract this to a separate function?
|
| Yes, I could.
|
| > * I would extract this to a separate function.
|
| Good for you.
|
| > * This could be extracted to a separate function.
|
| This one's OK as a style suggestion that you won't block the
| merge over.
|
| > * This should be extracted to a separate function.
|
| This is kinda, sorta OK, if you're going to block the merge if
| this isn't fixed.
|
| > * Extract this to a separate function.
|
| This is better because the tone matches the intent: it's a
| command. This gets addressed, or no merge. "This should..." is
| indirect--you're not stating what you actually mean. This
| phrasing is better.
|
| 4 is the best of these for optional suggestions, 6 is best if you
| consider the fix a requirement for a merge. In general, say what
| you actually mean--dancing around it leaves room for
| miscommunication and can give all kinds of bad impressions. Of
| course, that can also mean weakening language: don't _command_ a
| change that you don 't consider a big enough deal to block the
| merge.
|
| [EDIT] And, as others have noted in the thread, of course
| expanding with justifications is always a good idea. "Do this"
| without a "why" should be avoided. But lead with what you want
| done, not the why--that follows.
| spiderfarmer wrote:
| I agree with everything you said. But I'm Dutch.
| StellarScience wrote:
| Based on a great article we read almost 15 years ago ("Effective
| Code Reviews Without the Pain":
| https://www.developer.com/guides/effective-code-reviews-with...),
| we start most code review comments with "Did you consider...?"
| This makes it non-confrontational and non-judgmental, as the
| author can easily reply that, no, they hadn't considered that and
| thanks for the suggestion.
|
| Everyone in the company knows it's sort of a gimmick, and we even
| make fun of it a bit, but even so it still works!
|
| "Did you consider extracting this code into a separate well-named
| function with clear inputs and outputs, to make the original
| function smaller and easier to reason about and maintain?"
| greazy wrote:
| Never use should. It's confusing and inherently negative. It's
| also not very clear. Compare should with could in your examples.
| Could is very clear, a direct question to the read.
| xwdv wrote:
| How about
|
| * Why isn't this extracted to a separate function?
| XorNot wrote:
| Use number #6. If it's not necessary, then why say it at all in a
| code review? Everyones got other things they'd like to do.
|
| All the "should" and "considers" are tickets that can go on the
| backlog after a merge has delivered functionality.
|
| Then YAGNI will probably turn out to apply.
| BoorishBears wrote:
| To add to the other points, remember you can also leave positive
| feedback.
|
| Don't force a compliment for no reason because it _will_ come
| across as insincere... but if you see something well done
| /implemented, don't be afraid to call it out.
|
| It helps balance the tone sometimes, and is especially helpful
| with junior engineers who are well served not just learning
| what's wrong, but what's right.
| [deleted]
| 960design wrote:
| The clear declarative: "Extract this to a separate function."
| Everything thing else can be misinterpreted as a suggestion, dig,
| ect.
|
| Actually an interview question to work with us. We need code, not
| egos. Write and be wrong, correct and continue. Do not sweat
| errors. None of us are perfect, and neither is this review.
| pydry wrote:
| If it's negative feedback or something that feels controversial I
| dont put it on a PR at all.
|
| I try to do it in person or (remotely) send a private slack
| message.
|
| People can be sensitive to negative feedback and one way of
| ameliorating that is not to give it in a public forum where
| everybody can see it.
| mytailorisrich wrote:
| Code reviews are primarily a way to detect bugs and to verify
| compliance with a set of rules.
|
| Most comments are therefore expected to be 'negative' in some
| way.
| dyingkneepad wrote:
| If this is on an open source community, it's bad advice IMHO.
| On open source communities, please make all feedback public. It
| is really important to make the history of the project public.
| There are many ways to write the feedback in a not-offensive
| way, many of the comments in this thread have good suggestions.
| pydry wrote:
| I was assuming a normal working environment.
|
| OSS is a different kettle of fish. You dont have to worry
| about bruised egos nearly as much & the value of public
| feedback is higher.
| Uptrenda wrote:
| I'm sure this will piss people off: but almost all of the 'code
| reviews' I've been a part of were a waste of time. People trying
| to force stylistic changes to how a program works (OPs post is an
| example) aren't contributing anything constructive. What is
| worthwhile is trying to find critical security, logical, or
| performance bugs and reporting those. If you have taste / style
| concerns about how someone does something yet their solution is
| still fine then I'd suggest not wasting peoples time. There is
| nothing worse than nit picking.
| yoyopa wrote:
| last one... save people time. i don't need pleasantries.
| siquick wrote:
| You may not need them but what about others?
| perlgeek wrote:
| Code review is a form of communication, and thus depends very
| much on culture.
|
| This can both be company culture and origin culture of the
| developer you're communicating with.
|
| For example, sometimes I write comments like "I don't
| particularly like this because" plus an explanation, plus "but I
| don't see any significant better way, so I'm fine with it for
| now". Some of my colleagues are fine with that, maybe they add a
| code comment like "TODO: find a way to fix problem X", or they
| just acknowledge and don't change anything. And then there are
| developers I work with that come from a different country, and
| they'll spend another half day or even two days coming up with a
| better solution, or don't dare to click on "resolve thread".
|
| I think their culture is really geared towards avoiding verbal
| criticism, and if I even bother to write something negative (and
| maybe I'm also senior to them, in the org chart), it must be
| really meaningful and _must_ be addressed.
|
| So for some of these "foreign" developers I only leave comments
| where I expect something to change; for others that whose culture
| I better understand, I sometimes leave comments that are more
| conversational.
| Toreno96 wrote:
| Complementary to other answers, I would suggest introducing this
| technique: https://dev.to/allthecode/moscow-the-best-code-review-
| techni... However, share this w/ the reviewee first!
|
| I know some devs that are anxious if they don't fix _every_
| single comment in their PR, even if I'm just suggesting something
| as "nice to have". This helped to mark which comments are actual
| priority.
|
| Also, make sure you're using code formatter in your CI, to avoid
| reviewing the code style, as this can start some unnecessary wars
| and frustration.
|
| Also, I suggest sharing this article https://mtlynch.io/code-
| review-love/ with your colleagues, especially step 8:
|
| > As the author, you ultimately control your reaction to
| feedback. Treat your reviewer's notes as an objective discussion
| about the code, not your personal worth as a human.
|
| To some degree, as a reviewer you can keep your tone friendly,
| but ultimately _every_ sentence can be took as personal attack,
| and you can't really do anything about that. Keep that in mind,
| don't focus _too much_ on the tone, and don't let that prevent
| you from giving a comprehensive review.
|
| One other thing I personally do, because I'm famous for giving
| _really_ thorough reviews, is to make sure from time to time that
| the other person is fine w/ that. Simple DM "I hope you don't
| feel too overwhelmed by my CR?" helps, especially that often
| people respond that they actually approve I'm so thorough. If you
| worry people get offended, it is easy way to make sure if these
| worries are valid or not.
| 1-more wrote:
| "if we extract this to another function we can then add a test
| like this" with an example. Gotta say what the value add is. The
| value add could just be that it's more like the rest of the
| codebase! "In the past we usually extract functions like this
| link optional second link"
| justinram11 wrote:
| [Suggestion] Use conventional comments[1] to flag how important
| the comment is.
|
| Most of my comments end up being "suggestions", but then when I
| put a [blocking] on it, it clearly communicates that I think this
| should be fixed before merging in.
|
| 1: https://conventionalcomments.org/
| amrox wrote:
| praise: I came here to suggest conventional comments as well!
| imiric wrote:
| thought: reading this style of sentence would get annoying
| quickly.
|
| I don't get the point of these TBH. The usefulness of
| conventional commits is that they can be easily aggregated to
| get an idea of the types of changes that were made.
|
| Code review comments are purely meant for human
| communication, so requiring a specific structure is both
| annoying to write and to read.
|
| I tend to use one, at most two prefixes. "Nit", and rarely
| "blocker". Though I'd rather specify if something is a
| blocker once I make my case about what needs to be changed.
|
| Every other comment is assumed to be a suggestion, and
| depending or not I block the PR by "requesting changes", it's
| up to the author to either accept my suggestion or provide
| reasons not to.
|
| Any other strict guideline about how to write prose is silly
| to me. Especially the decorations section. C'mon now.
| Deradon wrote:
| Offtopic but related:
|
| Conventional commits:
| https://www.conventionalcommits.org/en/v1.0.0/
| ra7 wrote:
| Using conventional comments also forces me to rethink if my
| comment is really blocking or is just an optional suggestion
| and I can adjust the tone accordingly.
| [deleted]
| djadmin wrote:
| I use conventional comments everyday. It just helps me
| communicate clearly and concisely.
| cubes wrote:
| +1 for use of Conventional Comments. It's a simple lift for a
| noticeable improvement in clarity, and guidance on what to do
| next.
| difflens wrote:
| Across the different teams and orgs I have worked at, there's
| always been 1 constant during code reviews: Each comment provided
| an explanation of why some change was being debated. Every
| engineer knew the review is not a judgement of their coding
| prowess. Rather each discussion was purely on the merits of that
| line of code. Sure, there can be disagreements, but it was never
| judgemental. Usually folks are amenable when there is good
| justification to the feedback in my experience. It goes both ways
| too: I personally don't care if some of my nits are not fixed.
|
| Btw, since I think it is relevant to this discussion, we're
| working on making code reviews deeper than text based diffs.
| Check out (DiffLens)[https://github.com/marketplace/difflens] if
| you're code base is primarily TS, JS and/or CSS to get language
| aware diffs on your GitHub pull requests. We've found that it
| makes understanding code changes much easier
| harrisonjackson wrote:
| All of this should be hammered out in a set of guidelines around
| code review so that ego/feelings don't need to come into play.
| Have style guides, code review checklists, and best practices
| documented.
|
| The actual words used should be determined by the goal of code
| review and the relationship between reviewers.
|
| Anything that can possibly be automated by CI should be. Style
| guide and test coverage should automatically fail PRs before a
| human needs to add any "nit"
|
| That means code review should be a teaching or question moment
| (between a senior and junior, newer and veteran) or a discussion
| between peers.
|
| From a junior to a senior -> why did you extract this? why didn't
| you do it this way?
|
| From a senior to a junior -> typically, I would extract this
| because X. Check our best practices doc for a longer explanation.
|
| Between peers or senior/junior (maybe one is newer) -> it isn't
| in our coding best practices document, but normally we extract
| this because X
|
| Between peers -> what do you think about extracting this because
| X? Should this be in the coding best practices document?
|
| The last is important because ideally you're updating your coding
| guide so this same review doesn't need to be done 10 times. And,
| if you don't think it should be documented as a best practice
| then don't bring it up in a review.
| asimpletune wrote:
| Personally, I think that the focus on on the phrasing itself,
| which as you mentioned are all more or less synonymous anyway, is
| letting the underlying challenge get away from you. You can
| safely use any of them, but that there's other stuff that can be
| done that's more important.
|
| Any time you can convincingly explain exactly why you're
| suggesting something it makes the suggestion itself seem much
| more reasonable, and it's a valuable check on your own reasoning.
| From there, you can reword the explanation itself in creative and
| valuable ways to give feedback. If you get really good at this,
| then a lot of times you don't even need to ask for a specific
| change, you can just turn the problem you're seeing (I.e.
| 'explanation' from above) into a question. I'll give an example.
|
| Let's say you want to say "Extract this to a separate function".
| Ok, now, ask yourself why? So you tell yourself, "Because we may
| want to use part of this function again, but not the rest of it,
| and the exact same behavior can be achieved with little to no
| additional effort by function composition (or whatever you do in
| your PL)" Ok, that seems reasonable, but maybe now it may be
| perceived as a little too abstract to someone else, especially if
| they're just trying to get their work done. But you can take
| solve that by taking the above 'explanation' and make it
| concrete. One good way to do this is to think of a counter
| example, where it clearly doesn't work how we want.
|
| As an example, let's just pretend that they're doing something
| like parsing a thing from a database into some strongly typed
| thing, and then doing foo work on the strongly typed thing. You
| can ask yourself then, well if we were unit testing this, we
| won't care about the parsing part, we can just start with the
| strongly typed thing as the precondition of our test, and then
| isolate what we're testing to just be the foo functionality. Now
| you have a simple example that you can rephrase into your
| original feedback.
|
| This thought exercise all leads to the feedback going from
| "extract this to a separate function" to "Does this mean that
| unit testing the foo feature requires a parsing step for each
| test case?"
|
| Maybe that's a silly, contrived example, but I've found that if
| you even capable of going through this thought process then not
| only do people receive your feedback much better, but also you
| just tend to think of much better feedback altogether. After a
| lot of practice you end up being able to think this way very
| quickly and then you can focus on adjusting the wording based on
| your relationship with that person.
|
| At the end of the day, it's not what we say that really matters,
| it's how someone feels they're being regarded by one their peers.
| If they sense contempt, then it won't matter how politely or
| expertly you word things. Having extremely well thought out why's
| in your back pocket, and being able to provide concrete counter
| examples just makes it clear you're not wielding your review
| power just to talk but to illuminate the problem to them. It also
| solves the problem of when you're the idiot (as happens to all of
| us from time to time), by forcing you to actually justify your
| feedback before you share it.
|
| When I've done this, I've noticed that my reviews become faster,
| and I focus on stuff that people don't think about. It makes the
| code better and then people will actually seek out your review.
| ulisesrmzroche wrote:
| There's not a lot you can contribute unless you worked on the
| ticket. Notice how the only example you came up with was
| refactoring.
|
| This was more important back in the day but now linters and
| formatters are mainstream.
|
| The best time to code-review is at the beginning.
| mjfisher wrote:
| Actually reaching a level of team culture where you can be as
| brief, blunt and to the point as possible is a wonderful thing.
|
| There is no offence to be taken, because the deeply held respect
| for each other as team mates is already there and taken as a
| given. You trust each other to know that any comments are
| addressed directly to the code, rather than to the person behind
| them. All input is valued because everyone knows the goal is just
| to make the code better and help each other out, rather than
| criticise an individual. Discussion can be frank, open, unbiased
| and non-confrontational.
|
| However, because that takes a long time to develop and is
| somewhat of a rarity, I've had quite good luck with the phrasing:
|
| "Have you considered X here?"
|
| It has a number of advantages:
|
| * It doesn't imply that an author should have done it a different
| way
|
| * It doesn't imply that an author has missed something you think
| they should have covered
|
| * It's a good starter for a conversation, rather than a conflict
|
| * If the answer is "no", it provides a good prompt for an author
| to come up with a suggestion first
|
| * If the answer is "yes", they can explain why they ended up with
| the solution they did, which is visible to everyone who wants to
| look at the PR. That can also be a good prompt to add a comment
| omegalulw wrote:
| Another method: prepend "nit: " to comments that improve the
| code but are not calling for changing the main logic. As a
| reviewer, you are simply acknowledging that this is a nitpick
| :)
| angarg12 wrote:
| I keep a running list of articles I found useful over the years.
| There are many great articles about Code Reviews out there.
| Consider this a more general answer to your question:
|
| Feedback Ladders: How We Encode Code Reviews at Net-lify
|
| https://www.netlify.com/blog/2020/03/05/feedback-ladders-how...
|
| How to Make Good Code Reviews Better
|
| https://stackoverflow.blog/2019/09/30/how-to-make-good-code-...
|
| Best Practices for Code Review
|
| https://smartbear.com/learn/code-review/best-practices-for-p...
|
| Code Review Guidelines for Humans
|
| https://phauer.com/2018/code-review-guidelines/
|
| Unlearning toxic behaviors in a code review culture
|
| https://medium.com/@sandya.sankarram/unlearning-toxic-behavi...
|
| How to Do Code Reviews Like a Human
|
| https://mtlynch.io/human-code-reviews-2/
|
| Stop Nitpicking in Code Reviews
|
| https://blog.danlew.net/2021/02/23/stop-nitpicking-in-code-r...
| whoomp12342 wrote:
| Always friendly. Life is too short.
| barbarbar wrote:
| This one is nice.
| sixothree wrote:
| Static Analysis should be able to provide a "complexity" metric.
| If the method reaches a certain complexity, certain tools will
| consider it a bug. Refactoring is sometimes the only way to
| address these issues.
| some_chap wrote:
| I typically go with something like "Have you considered
| extracting this to a separate function"?
| davidthewatson wrote:
| My suggestion would be typical: concise, professional, and when
| straight-forward, specific. The key is to remember that the
| nature of a code review is first to improve product, and perhaps
| secondarily to improve process (ops or procedure) and perhaps
| thirdly, people improvement in the form of collective learning.
|
| Thus, if we're talking about refactoring a line-of-code then just
| writing the refactored line-of-code is probably the most clear
| and concise. If it's more involved than that, then it may be
| worth taking offline. In the example you've provided here, I'd
| suggest the best language is likely: Refactoring this block as a
| separate function provides the following benefits: 1. blah, 2.
| blah blah, and 3. blah blah blah and no side effects. The only
| additional clarity that's helpful is to say whether it's
| optional, required, or some other directive.
| ratherbefuddled wrote:
| The emphasis should be on clarity. Overall you want to leave the
| requester in no doubt about what it will take to get approval
| because the most frustrating thing for both of you will be ping
| pong.
|
| Tone is very hard to judge in written form particularly when
| there might be cultural and language divides, in my experience
| though it doesn't really matter what style you use provided that
| you also remember to comment on things that are good and do not
| need changing. This is routinely forgotten and more than balances
| any perception of negative tone the requester might inadvertently
| infer.
| mrfusion wrote:
| > "Extract this to a separate function please"
|
| 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.
| emichelsx wrote:
| I would extract this to a separate function.
| hluska wrote:
| There are three main categories of code reviews:
|
| 1.) Things are mainly bad.
|
| 2.) Things are mainly good.
|
| 3.) Somewhere in between.
|
| Within those, everything I see and flag as worthy of discussion
| is either:
|
| 1.) Something that violates our rules/style.
|
| 2.) Something I have a strong opinion on.
|
| 3.) Something I don't understand.
|
| Depending on which main category the review falls into, I'll deal
| with these differently. If things are mostly bad and I don't
| understand something, I'll ask a lot of questions about
| readability and naming conventions. If something is mostly good
| and I don't understand something, I'll ask "Hey, can you explain
| lines 71-104?"
|
| In general, I try to keep things friendly and positive because I
| don't want people to dread the experience. Life is too short to
| dread a big part of your job.
| eof wrote:
| I adopt, and appreciate when others adopt a neutral/brutal tone.
|
| "These lines have been repeated a number of places and probably
| ought to be pulled into a function called something like xyz in
| module zyq"
|
| Generally your tone doesn't matter as long as it's not mean.
| "This is the 10th time I've told you declare constants FOR EVERY
| MAGIC NUMBER," does not belong in a code review; it belongs in
| feedback.
|
| However there is something a bit dangling here which is linting
| and static analysis. If code has been repeated and genuinely
| ought to be pulled into a function, static analysis ought to be
| able to find this.
|
| If you have already dealt with the broad class of possible change
| requests that can be caught by static analysis, the nature of the
| reviews should get a lot more straight forward. It will stay at a
| higher level and naturally tend toward verbiage like "I wonder if
| we ..." or, "I was thinking if ..."
|
| In cases where static analysis would not find it for e.g. in the
| middle of a function doing something, we have several lines of
| code collecting data for some analytics event which is never
| repeated, but still confusing; I would say something like "For
| readability, could we pull the analytics bit into a separate
| function?"
|
| Generally; if you can use "we" instead of "you" or "I"; I would
| prefer "we" in any team setting.
| dschuessler wrote:
| Personally, I prefer "What do you think about extracting this
| into a separate function?"
|
| Unlike in any of your examples, the subtext is: "I anticipate you
| had reasons for doing it the way you did and I am open to
| listening to them." It engages the author on the same footing,
| leaves the door open for discussion but still communicates your
| intent in a straightforward manner.
| jeppester wrote:
| My personal favourite is: "Have you considered X"
|
| I also like to often prefix my suggestion with: "It might have
| missed something, but X", especially if it's a larger change.
| pclmulqdq wrote:
| I hate it when people say "have you considered X?" and mean "do
| X or I will block your change." It's all about communicating
| what you mean.
| XorNot wrote:
| I agree with this so much.
|
| IMO if it's not mandatory I don't think it should be said at
| all in a code review.
| jeppester wrote:
| I absolutely get that. I was a bit in a hurry when I wrote
| the above comment, and therefore didn't get around to better
| detail how I use those sentences.
|
| It's all about understanding the context and not talking down
| to the author.
|
| When I see obvious improvements I use the "order" style and
| just state what needs to change. Because surely the author
| can see that the change will be for the better.
|
| The other sentences are for bigger more complex changes where
| I'm curious about a certain decision. Most often I don't
| agree on the current design, but I would never write such a
| comment unless I'm ready to be convinced otherwise.
|
| When someone has spend hours on some functionality and I
| spend just 5-10 minutes skimming through the code, there is a
| very real chance that the change I'm going to propose was
| already considered. It would be arrogant and condescending to
| just conclude that my idea is better, so I open the door for
| the authors explanation.
| tboyd47 wrote:
| All of these are equivalent if you have showed your team that you
| trust them and value their work.
|
| If not, then only the first version is acceptable.
| jugg1es wrote:
| This is a great question because I find that when I use soft
| language like this, a lot of the suggestions get ignored.
| Sometimes that is fine but other times, I have to follow up after
| and explain that the 'suggestion' wasn't actually optional. I
| find it hard sometimes to protect egos and quality at the same
| time.
| gspencley wrote:
| My "formula":
|
| 1. Try to find something positive to say about the code. This
| reinforces what is being done well and supports the notion that
| everyone is on the same team chasing the same goal.
|
| 2. I like to go deep into the "why", being as objective and fact-
| based as I can. Code style and "best practices" are sometimes
| confused with personal opinions and preferences. I avoid words
| like "I prefer". If I can't back up my opinion with strong
| reason-based arguments for why my opinion is objectively better
| then I keep it to myself and continue to ask myself if it's a
| personal preference or if there's a good reason for favouring
| that.
|
| 3. Tone must always be friendly, polite and constructive. Of your
| options I often go "I would extract this to a separate function
| because ..." and then I go into the "why"
|
| 4. Sometimes I have a lot of supporting information to explain a
| position. I will often label these "Academic Digressions" and
| then drop a TL;DR. Sometimes people don't have time to read them
| right away but I'm often told that people appreciate them and
| save them for later.
| padjo wrote:
| Pointing out the good in a PR is such an important thing to do,
| particularly with juniors!
| ssd8991 wrote:
| Tainnor wrote:
| I find that I rarely use direct commands in code reviews, even
| ones expressed indirectly (e.g. "can yo do X" instead of "do X").
|
| If there are blockers in a PR, I'll add reasoning for why it is a
| problem (e.g. "this exposes us to vulnerability X", "in this edge
| case, what would happen is that ...", "I tested it and appears
| that there is a bug", etc.) If it's a team-agreed style issue
| (that isn't caught by the linter), I'd rather say "we use
| convention so and so in this code base" than "please change this
| to this style".
|
| The reasoning is that I feel that I work with professionals and
| while they may need _feedback_ , they don't really need commands.
| I can make suggestions as to how they can fix the issue I raised,
| but ultimately it's up to them to fix it and if they find an even
| better way that I didn't think of, why should I limit their
| problem solving capabilities artificially?
|
| I find that most people I've worked with use a similar style
| (more centered around the code than around power dynamics), and
| those people who used a lot of "please do X" style comments were
| usually also pretty defensive about their code or ideas, would
| get uneasy when you changed some of their code ("why did you
| refactor this? please change it back") etc.
|
| Now there may be some people (inexperienced, or just idiots) who
| really need some more forceful language, but you shouldn't
| necessarily start out on that default assumption.
|
| That covers the blocker-style comments. Everything else should
| probably be done in either a "I'm opening up a discussion here
| and would be interested in your opinion" or a "this is just a
| nitpick, feel free to ignore" kind of style.
|
| ---
|
| For the specific example, depending on the reasoning for why you
| might want the change:
|
| - "The linter is complaining that this function is too long and
| we typically stick to those rules. I think lines 20-30 could be
| extracted into a separate function, for example."
|
| - "I found it a bit hard to understand this function initially
| because it does a number of things at once (such as...). Maybe we
| could split it up?"
|
| - "This section of code is repeated between here and that other
| function and I think we should keep the implementations in sync,
| so I would recommend extracting that common logic."
|
| - "We've actually had to do something similar in other situations
| (see here and here) and your solution seems better than what we
| came up with before, so what do you think about extracting this
| functionality into a function and calling it from all those
| places?"
|
| and so on (and it doesn't need to be as verbose if you know that
| you have more shared context, for example).
| collinvandyck76 wrote:
| I always try to make suggestions as questions because it's less
| threatening, gives more agency to the author, and oftentimes ends
| up being a discussion on the merits. If the suggestion really is
| a blocker, I'll be a little more direct, but always always
| friendly.
| IncRnd wrote:
| 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 duplicates the same set of processes as over
| there. Refactor these into a single function, and we'll be good
| to go.
|
| * While I don't have feedback yet on this function, it's too long
| for me to follow. It would be easier to read and for future
| maintenance if you refactor this part into a function.
|
| * Since we're in closedown we can only take certain types of
| changes. If you refactor this into a separate function in the
| library, this change can be accepted.
|
| * Or, I hate to be the process person, but the internal
| guidelines for this team call for all code to be structured the
| same way. Refactor this part into a separate function and I'll
| approve the PR.
|
| There are lots of ways to provide feedback. I suggest stating the
| problem with the code and providing a solution. If that's the
| only possible solution to get past your review, state and don't
| ask. You can also give a carrot with "do this and I'll approve
| the merge."
|
| How would you speak when sitting next to the person face-to-face?
| What tone do you want your boss to use when providing a
| performance review during a 1on1?
| jszymborski wrote:
| Just wanted to chime in and say that I still remember the first
| code reviews I received and while I didn't take any of the
| comments personally, they certainly didn't feel great to
| receive. (I should add that I don't begrudge the reviewers,
| they were nice people).
|
| 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.
| IncRnd wrote:
| Thanks!
| jsnelgro wrote:
| These suggestions hit the nail on the head. Additionally, the
| reviewer also becomes a better dev because they're forced to
| really think about and articulate why they think the code
| smells.
|
| Even if my coworker's code doesn't feel right to me, I'll
| sometimes forgo commenting if I can't articulate why. My goal
| is to avoid bugs and deliver features, not to force my code
| style and opinions on others.
| pfortuny wrote:
| Yep: the operative word being "because", absent in all the OP's
| sentences.
| BeetleB wrote:
| I've long wanted to write a blog post on applying what I
| learned from effective communications books to code reviews.
|
| Your comment mirrored something I wrote in another thread about
| the problems with the Socratic method in general[1]:
|
| "If you have a concern, then express the concern openly before
| asking your question. This will make it clear to the recipient
| what your intent is, and they will not have to guess."
|
| The worst comment I see in code reviews (and sadly, all too
| often) is:
|
| "Why did you do it this way?"
|
| I have no idea why I'm being asked this. The answer is "Because
| it solved the problem."
|
| Even this is problematic:
|
| "Why did you do it this way instead of X?"
|
| Possible (unhelpful) answers:
|
| "Because I didn't think of X." (I still don't know if you want
| me to change the code and why).
|
| "Because it solved the problem."
|
| Your examples are good ones on how to ask this question "This
| could have been solved via X, which has the benefits Y and Z
| compared to your approach. I suggest changing this to use X,
| unless your approach has advantages that I'm unaware of."
|
| Probably about half the times I get something like this: Yes,
| my method did have advantages the reviewer is not aware of, and
| we then have a discussion.
|
| [1] https://news.ycombinator.com/item?id=31889518
| adhesive_wombat wrote:
| > Why did you do it this way instead of X?
|
| Argh, yes, that's one line that might make me just walk away
| from a PR as a new/junior/casual contributor.
|
| You, the reviewer, are an expert in the system. Likely you
| are the or one of the most expert people in the entire world
| on this exact thing. You know X exists and why to use it. As
| you should, because you put it there. You also should know
| that people who _aren 't_ experts (like me) don't know about
| it, simply because they didn't use it, in this PR, when they
| should have. Why don't they know it? Probably because _you_
| haven 't used it consistently in your own code, or it's not
| documented. This newbie has cobbled this PR together from
| what sense I can make of this project. Probably 90% is
| guessed from code I found in there already.
|
| What wouldn't wind me up?
|
| "I think a better way to do this is X. It's better because Y.
| Or have I missed a specific reason for X?"
|
| Note two things: 1) explanation to a noob of reason Y, which
| may well be valuable, not only to the noob, but also in the
| record of the project in general. 2) The indication that the
| noob might at least have had a logical approach, and they're
| not an idiot, just a noob.
|
| Afterwards, if this seems like something the noob should have
| known from the codebase, consider that you, the maintainer,
| have failed to make it clear.
|
| Of course, if I'm also supposed to be an expert, e.g. a co-
| maintainer, then it's different. I _should_ know X. Which
| means either it 's a brain fart, or I actually do have a
| reason. In which case _I_ should have commented on the code,
| because if another contributor can 't tell the intention in
| the PR, then they can't tell in a year when no one can
| remember why it went that way.
| BeetleB wrote:
| > You, the reviewer, are an expert in the system. ... You
| also should know that people who aren't experts (like me)
| don't know about it, simply because they didn't use it, in
| this PR, when they should have.
|
| While this happens, I find that it's very common that _even
| a junior developer_ has some insights the expert reviewer
| doesn 't by virtue of the fact that he/she has spent more
| time on this _specific_ problem than the reviewer has. As a
| reviewer, it 's always better to assume you are not the
| expert.
|
| > Of course, if I'm also supposed to be an expert, e.g. a
| co-maintainer, then it's different. I should know X. Which
| means either it's a brain fart, or I actually do have a
| reason. In which case I should have commented on the code,
| because if another contributor can't tell the intention in
| the PR, then they can't tell in a year when no one can
| remember why it went that way.
|
| I disagree, and your example is a good case of not
| considering all the possibilities.
|
| Yes, in certain cases, X may be the obviously better
| approach, and a comment would be a good idea. In many/most
| cases, there are N approaches, and you happened to pick
| one. If your approach has advantages over X, I don't see a
| need to justify it as a comment in the code because you
| then should put comments to justify it over the (N - 2)
| other solutions.
|
| At the end of the day, the burden is on the reviewer to
| specify why he prefers X. Only if he presents a case ("X is
| better because of reason Y") should the developer feel
| obliged to justify the decision.
| adhesive_wombat wrote:
| > In many/most cases, there are N approaches, and you
| happened to pick one.
|
| I'm not sure that's always true. A lot of the time, while
| there might be, in a vacuum, many ways, in the context of
| a specific project (or part of), there's usually one
| "most right" way to do it, considering local style,
| idioms, norms and conventions. Admittedly, this might be
| more true in some languages and applications.
|
| Sometimes there are multiple ways. Usually this means
| that one or more disparate legacy ways are extant, but
| there's a preferred way to move towards.
|
| Having PRs storm off in "exciting" new directions,
| technically correct or not, is usually a bad idea once
| that merges and is now everyone else's problem.
|
| If all alternatives are equally valid logically, it's
| likely they have practical implications that differ.
| Perhaps one might consider a std::set better than a std::
| vector in the abstract for your task, but maybe you know
| something important about the expected number of items or
| memory access patterns. This is when you should comment
| if that's not obvious to someone who isn't literally just
| done writing the change.
|
| > At the end of the day, the burden is on the reviewer to
| specify why he prefers X.
|
| If a reviewer _is_ going to pull me up on truly
| equivalent things that have no impact on correctness or
| maintainability, then I 'm probably not going to
| voluntarily be a co-maintainer with them. If I've
| consented to be a co-maintainer, then I have either
| mutual respect for the others, or I'm being paid enough
| to make it worth it. And indeed, I have refused
| maintainership offers in projects with ":eyeroll: why
| didn't you just magically intuit X" cultures.
| lloeki wrote:
| > "Why did you do it this way?"
|
| While the question is literally (and without any other
| thought) what I have sometimes in mind, usually I ask
| something like:
|
| _I feel I might be missing some insight or context, could
| you walk me through your rationale?_
|
| Because if I don't get some code change I may very well be
| missing something. And in any case by talking we're only
| going to improve our mutual understanding, both of the
| codebase and of each other's thought process.
| broast wrote:
| I agree with this. Don't give demands, give reasoning and
| examples.
| sebastien_b wrote:
| Agree with adding additional reasoning, regardless of the
| actual tone.
|
| I've received some feedback along the lines of "LOL Wut?" on a
| comment in code, which I really didn't know how to take or
| respond to (suffice to say its meaning was vague, and generally
| unhelpful).
| somethoughts wrote:
| Where possible it can also be useful to point out if there are
| existing guidelines/checklists the submitter should be
| referencing to understand these preferences of the project. The
| more the contributors can code review themselves prior to
| submitting a PR the better.
|
| And if such a resource doesn't exist or that topic is not in
| the current guidelines/checklists - offering to take up the
| task of creating or adding an topic to the list of things to
| check before submitting a PR can be helpful.
|
| Eliminating nitpicky PR comments by having pretty verbose
| coding guidelines as well as pretty strict settings for
| automatic code linters/syntax checkers/static type validators
| means the PR feedback cycle is a lot more focused.
|
| And for architectural discussion - draft PRs can be useful
| early on for feedback on design choices for more challenging
| features can be helpful.
| drewcoo wrote:
| This!
|
| Walking on eggshells because every comment chain devolves
| into religious battles about style? Find a linter or style
| checker that can automate comments/fixes. Talk about those
| checks in your team meeting, get consensus (who cares what it
| is, just consensus), and make the tools enforce it. Or
| suggest your manager do this.
|
| If discussing actual code problems in code reviews rankles
| people, just escalate to your manager.
|
| If you need a special code review human language lexicon
| because people can faint or throw punches, it's a management
| problem. Because that's the kind of thing that makes other
| people, not the troublemakers, leave the company.
| wayne-li2 wrote:
| 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.
| IncRnd wrote:
| Haha! That's an excellent way to state the issue.
| fossuser wrote:
| +1 the reasoning is the entire point and all of these examples
| are good.
|
| Just telling people to make changes without telling why doesn't
| teach them anything - especially when sometimes what's being
| requested isn't actually better (or is at least subjective).
| convolvatron wrote:
| my rule is to weigh whether or not its worth making a comment
| very carefully. nothing without a clear justification. never
| say 'I would have done it this other way' - unless its a good
| friend who you often discuss style issues with. comment on
| things that you feel would make a substantial difference on
| schedule, maintainability, or function - and provide a clear
| reasoning with which to argue against.
|
| wasting your peers time trying to enforce your style isn't
| just non-productive, it sours everyone on the whole exercise.
| samuell wrote:
| These work and sound great, but stating them without a "please"
| can come off as a little brusque. Please add a "please" before
| the orders, and I'll accept the advice.
| mmazing wrote:
| .. and sometimes after going through that exercise you might
| find you don't have a good reason for what you're asking for.
| :)
| [deleted]
| mmcnl wrote:
| I agree with the tone of your suggestions, but your suggestions
| still take quite some time to read. They can easily be
| shortened without losing any information. For example, no need
| to to apologize for being the process person (being overly
| apologetic decreases the value of apologies anyway).
|
| Also for me it feels like you're seeing code reviews as a
| senior/junior thing. It's more than that, no?
| [deleted]
| fleddr wrote:
| My tone normally is "I believe this should be extracted to a
| separate function, and here's WHY".
|
| The WHY is to typically refer to some best practice, coding
| standards, something as neutral as possible.
|
| I use the combination "believe" and "should" as it's friendly but
| still has some hint of authority. Another tip is to not just post
| negative review comments, I also comment on things that went
| well.
|
| When I see somebody repeatedly make the same mistake, or very
| severe mistakes, I schedule a session to discuss the topic, as
| there's likely a knowledge gap or misunderstanding. Invest in
| people like that and make it clear that this improves life for
| both themselves and the reviewer. Win-win.
|
| People can be very diverse, of course. Still, if you need to
| sugar coat feedback too much and are afraid of a possible
| backlash or drama, that's unhealthy. The exchange of good faith
| professional feedback is part of business.
| tessgadwa wrote:
| >> There are lots of ways to provide feedback. I suggest stating
| the problem with the code and providing a solution. If that's the
| only possible solution to get past your review, state and don't
| ask. You can also give a carrot with "do this and I'll approve
| the merge."
|
| Yes. This. Clarity is a prerequisite for tact.
|
| Also, be consistent in your tone and style across reviews in a
| given project or org -- no matter who you're talking to. For
| instance, on one development project I was informed that all new
| tickets should use the language "shall" instead of "should" or
| "will." Consistent language gives everyone a common reference
| point and helps keep people from feeling "singled out."
| Juliate wrote:
| The tone also deeply depends on the culture of the receiver.
|
| Depending on whether their cultural background is North American,
| Scandinavian, German, French, Italian or Chinese (to pick from
| the very situations I have faced), and especially if they are
| more junior, you cannot expect the same outcome from the same
| feedback, without additional context/care in the communication.
| MattGaiser wrote:
| As someone on the junior end, the tone is less relevant here than
| the content, and there is not a lot of content. Why do you want
| it extracted?
|
| Will we re-use it? Do you just not like blocks of code bigger
| than X (been given this reason many times)? Do you just feel the
| need to comment (a lot of devs do and I appreciate it when they
| are upfront about that rationale and I have done this myself to
| show that I am paying attention)? Are you concerned about
| readability?
|
| I need to know for future code reviews, so I can avoid the issue
| in the future.
| synu wrote:
| I think none of those, one that is focused on teaching and
| explaining your reasoning would be better. You can always do it
| humbly because the author may have some info you don't that they
| came across when implementing, which is why it is the way it is.
| throwaway4good wrote:
| Looks good to me.
| feliksik wrote:
| I have been looking for the tone too. Tone is important for
| techies, too. I just think it's not the biggest issue.
|
| I think the real problem is the we lack a framework that makes
| explicit:
|
| * What is the goal of a review, according to the team? Spotting
| bugs? Prevent security flaws? Guarding the architecture? Making
| suggestions to grow better as programmers? These goals partially
| overlap, but are different. Are you aligned, as a team?
|
| * Encode your priority in each comment. We could use some
| semantic prefix for a comment that categorizes it as, in
| increasing order: 1.suggestion to share views and alternative
| approaches, do as you please / 2. I think another approach would
| be better, suggest you use it / 3. I think it's very important to
| change something, let's discuss / 4. I'm not willing to approve
| or compromise, but feel free to get approval from someone else /
| 5. If anyone allows this to be merge, I will escalate.
|
| Then still we should use good tone, but it's more explicit what
| to expect, now. Open communication is key to a psychologically
| safe environment. But I'm Dutch :-)
| aruanavekar wrote:
| collaborative learning for all tone
| mrweasel wrote:
| While some people don't like hearing it: It is also partly up to
| the author of the code to ensure that reviews are handled
| professionally.
|
| 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.
| mberning wrote:
| The key is to understand the value and importance of what you are
| suggesting. "Remove this SQL injection." Vs "consider moving this
| functionality to the superclass". One is a must do and not doing
| it is unacceptable. The other is something that may improve the
| code, but isn't hurting anything if you don't.
| mhh__ wrote:
| Harsh but stick a joke in there so it looks like you aren't a
| dick.
| therealplato wrote:
| I explicitly prefix Error: when I am requiring a change, Warn: if
| I am suggesting a change, Info: if I have an unrelated comment
|
| Error: `< n` is an off by one error, the last element won't be
| processed
|
| Warn: Consider dropping the loop altogether. We know there are
| always eight elements.
|
| Info: Nice name!
| tialaramex wrote:
| Several other people made good points about the overall review
| structure that might well be _more important_. But to answer your
| actual question about tone:
|
| There are two axes I think about here when deciding on tone. Why
| am _I_ reviewing this code? And why did this person _write_ the
| code?
|
| For the me axis, at one extreme I'm paid to review this code, and
| I designed or co-designed the software that's being modified in
| the reviewed change. Architectural decisions were mine. At the
| other extreme, I'm just another volunteer, this system I'm
| presumably interested and somewhat knowledgeable about (otherwise
| why review code for it?) but I may not even be an expert, and
| certainly can't be said to "own" it in any way.
|
| For the other person axis, at one extreme they are assigned to
| work on my project, this is their job, at the other extreme it's
| some volunteer who has worked with this ten years longer than me
| and maybe designed it and it's good of them to ask me to review
| it, they could just push commit.
|
| #6 "Extract this to a separate function" is a command, so it's
| not appropriate unless either they're an employee and I'm
| supposed to be mentoring them, or they sent unsolicited
| contributions to some project where I'm an expert and frankly I'd
| rather not have their contribution than take it as it stands. If
| it's a project that _invites_ contributions (even very
| informally) then this is always the wrong tone.
|
| #1 "Should we extract this to a separate function?" is a
| question, and so it's not appropriate if I am expected to provide
| guidance (e.g. in mentoring or when reviewing code from a brand
| new team member) but absolutely appropriate if I'm reviewing code
| by somebody who knows the project much better.
|
| #3 "I would extract this as a separate function" is always
| appropriate _if_ you are sure you would do that work. However, if
| you are sure you would do that work, consider just doing it
| instead. In the scenario where it 's a junior employee being
| mentored maybe there's pedagogic value in _them_ extracting it so
| write either #5 or #6 depending on whether they seem to need the
| explicit instruction or whether it 's implied in the usual
| workings of your environment. In the scenario where you're a new
| volunteer maybe the author has a good reason it should _not_ be
| extracted and so it 's worth asking. But in the middle case why
| aren't you doing it? If it's just not worth your effort the same
| is true for the author.
|
| #4 "This could be extracted to a separate function" is redundant
| and so is #2 "Could you extract this to a separate function" the
| answers are "duh" and "hopefully you can do so or else you should
| learn this language". So overall I think I see three tones I
| would use (#1 #5 and #6), but in quite different circumstances,
| two tones I think are too vague/ redundant (#2 and #4), and one
| tone which is probably never useful (#3) because it suggests I
| should do work instead, in which case why not just do the work
| and say you did it "also I extracted this as a separate function,
| [link]..."?
| Traubenfuchs wrote:
| Easy:
|
| Start with "Consider..." in case it's not a must.
|
| If it's a must, start with explaining the consequences, e.g.
| "this value must be kept in a request scoped context or there
| will be race conditions and other concurrent misbehavior.
| Consider putting it in a @RequestScope bean."
| McNutty wrote:
| Neither of the first two nor anything else ending in a question
| mark unless you're really asking a question.
|
| Not #3 because you're still being coy.
|
| I'd suggest #4 or #5 when dealing with peers but they aren't
| interchangeable, they mean different things.
|
| Save #6 for when you're giving tons of feedback to a junior.
|
| If you're worried about being perceived as harsh or unfriendly,
| remember you can still be super friendly in the chatter on either
| side of the review session.
| tetha wrote:
| A good way to package possibly negative feedback is to formulate
| it as an I-message, or an observation and go from there:
|
| "I find this method to be overwhelmingly long and it's hard to
| keep track through all of it. However, it looks like this, this
| and this are rather isolated steps, they could be extracted to
| make the overall structure easier to see".
|
| "To me, this, this and this look like duplicated code. Do you
| expect them to evolve differently, or could they be extracted
| into a common function / module?"
|
| And if the code goes against architectural or style guidelines,
| it also softens the feedback to start with that reason and go to
| the suggesting from there. "This class was designed to be
| independent of that module to be testable. Please extract your
| code into the foobar-interface and inject it"
| lxe wrote:
| Here's a trick: always approve the code review, unless it's an
| actual bug, a security risk, or obvious quality issue. Then you
| can use whatever tone you want without much damage.
|
| "approved, but I think this should be in a separate function"
| poulsbohemian wrote:
| You've done a good job here, but I might suggest stepping back
| even a bit further. If you want reviews to work, they must be in
| the spirit of "we all improve and learn together" rather than
| "bad dog! bad!" The tone you set in _why_ you are doing them, who
| is involved, how they run makes all the difference in the world.
| What you want is to create a culture where it is _safe_ to point
| out something that could have ramifications, and to promote a
| culture where there can be dialog about what 's best for the
| health of the team and products you support. More often than not
| though, I've seen these meetings turn into "beatings shall
| continue until morale improves" sessions.
|
| Just my $0.02.
| sfink wrote:
| Lots of good suggestions.
|
| I don't think there are simple answers, and it's something you'll
| have to figure out separately for every reviewee, and sometimes
| every review.
|
| You have to consider power dynamics, preexisting relationships,
| your own time, the importance of the change, and cultural
| differences.
|
| If the power dynamics are too far apart, the right answer is
| often "don't". A high-level engineer or architect reviewing a
| relatively junior coder's patch should not be commenting on
| anything involving taste or stylistic suggestions. They should be
| reviewing for functionality, security, compatibility, etc. And
| only bring up maintainability or performance when it really makes
| a difference.
|
| Some suboptimal code will get landed as a result, but it's better
| than creating a culture where the senior's personal preferences
| are always going to win out. When there's a power imbalance,
| nuance gets lost--the junior either has to bend to the senior's
| preferences even when they have a good reason to do things
| differently, or they have to go to a lot of effort to justify
| their position. Which is fine in some cases, but it's a waste of
| time and effort for things that don't matter as much. (This isn't
| about avoiding hurting people's feelings, by the way, though it
| does serve that purpose as well.)
|
| I also agree that pointing out positive qualities of a patch can
| make up for quite a few critical comments. Explicitly
| constructive criticism (eg sketching out an alternative, or doing
| some work to provide data or justification for a comment) can
| also "buy" a smaller amount of more negative-sounding comments.
|
| And of course, if you're swapping patches with someone regularly,
| there's no need to overthink it: state the problems you see, the
| things you're not sure about that concern you, the suggestions
| you have, etc., using as clear and concise language as possible.
|
| In all cases, do not make the reader guess your actual opinion or
| intention. If you're couching a criticism in gentler language to
| avoid making them feel bad, that's fine, but don't play games or
| leave out important pieces. There's always a way to coach or
| criticize honestly, and you'll do yourself a lot of good by
| finding it.
| unnouinceput wrote:
| A - for juniors
|
| 1 - This / should / could etc etc make sense only if the code is
| duplicated. If it's long, better to make a sub-function and use
| those in main body of the function for better readability /
| maintenance.
|
| 2 - As for tones, you are the senior. Don't sugarcoat juniors,
| tell them the truth green in their faces. You'll be appreciated
| more in the future by them because you're helping them grow, not
| running for mayor office and you need to be PC. Also, don't be
| rude / asshole, they still need your help though.
|
| 3 - I found out, the best is to talk to them like you talk to
| your friends.
|
| B - for seniors
|
| No sub-points here, seniors who make blatant mistakes gets
| treated like juniors (see section A). Those who make honest
| mistakes, well I found out they only need you for rubberducking
| because mid-review they'll realize and correct / change course on
| the spot.
| nickjj wrote:
| I think setting team goals are important before anything. For
| example if the goal is to create the most maintainable and best
| version of the code in the long term I think this opens up doors
| for feedback not being taken personally as long as the person
| giving the feedback isn't dropping lines like "why didn't you
| just do xyz instead?".
|
| I tend to provide more open ended questions like "what do you
| think about ..." where "..." could be a few options to take. In
| my mind I've most likely picked one based on personal preference
| but I like proposing a few choices to spark some discussion
| amongst team mates and give everyone a chance to contribute.
| Often times throwing out a few options will result in someone
| coming up with another option that only became apparent after
| they've seen a few alternatives (which is a good thing).
|
| It also lets more folks make decisions and that's a very
| important thing. If you keep making every decision then folks
| won't feel like they can contribute anything because "why
| bother". I remember an old TNG episode (S3E21) around this with
| "Broccoli" when he was trying to integrate on the engineering
| team but Jordi kept alienating him. Picard dropped some really
| good life advice in that episode!
___________________________________________________________________
(page generated 2022-06-27 23:01 UTC)