Polite
pull requests

A guide to emotionally intelligent code reviews

[Image: A rooster in a meadow.]

Follow along for some presentation fun

https://slides.com/yvesgurcan/pr/live

(results may vary; speaker is not liable if presentation is not actually fun)

Polite
pull requests

First edition

[Image: The cover of the book Eloquent JavaScript.]

Senior Software Engineer

Yves GURCAN

WHO DIS?

"Yves" sounds like "Eve" or "Steve."

JavaScript is my love language.

Devetry since October 2021.

G4m3rz with poor gaming skills.

[Image: Portrait of the speaker.]

👀

Always looking for parenting advice.

Pull request?

A request submitted with the intent to merge changes into the codebase once they have been approved.

👍

Let's open a pull request together!

[Image: Example of a pull request with code changes and a conversation.]

Why POLITE?

The D-word

But not the one you're thinking about.

Divorce

  • Emotional intelligence (understand your emotions).
  • Empathy (put yourself in their shoes).
  • Active listening (acknowledge/retain what they tell you).
  • Desire to be heard and feel connected (you matter to me).

Solves a lot of problems preventatively.

🤵

SERIOUSLY, Why?

  • Avoid hurting the feelings
    of your coworkers and clients.
  • Make a good impression.
  • Keep things professional.
  • Create a relationship of trust.
  • Think twice about what you write.
  • Quality.

 

👔

Because I want you to talk nicely to me.

Pardon my French

  • Subjective.
  • Not definitive.
  • Spark a conversation.
  • Can be applied to various contexts.
  • Own it.

How's my driving?

@Yves

🇫🇷

Tried a different format for this presentation:
fictional examples first instead of theory.

We might not go through the entire presentation but that's not a problem. I would prefer a good discussion. Slides are sorted by importance.

This page intentionally left blank.

Sorry, I've always wanted to do that at least once in a presentation. 😅

It's not personal

That function is wrong.

function getBookTitle(language, edition) {
  const bookTtle = `Eloquent ${language}: ${edition} edition`;
  return bookTitle;
}

It seems like there is a typo in the constant name `bookTtle`.

Avoid using "I think," "right" vs "wrong," and other phrases that put people on the spot. Instead, point to what you noticed precisely and make it impersonal ("it seems") to avoid confrontation or hurting someone's feelings.

I think this won't work.

🤐

BLAMELESS Cheatsheet

  • It seems that (this does not work as intended)...
  • It looks like (this is the wrong type for this variable)...
  • I noticed that (this line causes an error)...
  • I suspect that (this could be implemented differently)...
  • There might be (a more elegant solution to solve this problem)...
  • It could be beneficial (to write a function to avoid repeating the same code)...

❤️

It's not You, IT's me

You should stick to a case. Why no semi-colons?

function get_book_title(language, edition) {
  const Title = 'Eloquent'
  const editionLabel = 'edition'
  return Title + ' ' + language + ': ' + edition + ' ' + editionLabel;
}

I've noticed that using the same case as well as semi-colons throughout a codebase helps with the readability of my code. How about using camelCase for function and variable names?

Avoid blaming the author ("you should") when you see bad code. Instead, provide the reason why changing the code would be beneficial, preferrably drawn from your personal experience.

✋

Actionable comments

The name of the function should have get.

function bookTitle() {
  return 'Eloquent JavaScript';
}

It looks like the name of this function does not respect the naming convention used in the repository. I would suggest naming it `getBookTitle`.
```suggestion

function getBookTitle() {
```

Provide an alternative. GitHub and GitLab let reviewers add code suggestions that can be committed straight from the pull request.

🗣️

Google It™

That's not how you do template literals.

function getBookTitle(language, edition) {
  return `Eloquent {language}: {edition} edition`;
}

I noticed that you use a template literal (on line 2). However, I suspect after re-reading the documentation at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals that you might be missing the `$` before the curly braces.

Providing links to the Mozilla Developer Network documentation or reliable Stack Overflow questions, for example, can help provide more context about the issue and can be a great learning experience.

🔍

Take it outside

const ELOQUENT = 'Eloquent Python: Twenty-third edition';

function book() {
  return ELOQUENT;
}

Hey! I have some thoughts about how we could refactor this function. I sent you a message on Slack. Let's put a video call on our calendars.

Instead of diving in a long conversation, offer to chat on Slack or to hop on a video call. It can get a little frustrating when an exchange keeps going on and on without a resolution. It can also create misunderstandings or give the impression that something is more important than it seems. Talking it out outside of the code review can help with getting on the same page.

✍️

Give the benefit of the doubt

The function needs to return a string.

function getBookTitle(language, edition) {
    const bookTitle = {
        title: `Eloquent ${language}`,
        edition: `${edition} edition`
    }
    return bookTitle
}

It seems like returning a string might be more handy. Is there a reason why you chose to return an object instead?

You might have strong opinions about how to write code. The author of the pull request might as well. In doubt, ask the author why they wrote the code this way. They might not have thought about the shortcoming of their approach or they might have some interesting suggestions on how to resolve some problems.

🤔

Establish guidelines

  • You might want to use an existing style guide to save yourself some work (Airbnb JavaScript Style Guide).
  • Set the guidelines together as a team instead of dictating them.
  • Make sure that there is a good reason behind each rule you set.
  • Refer to that guide as a supporting document during code reviews.
  • Also specify the review process (who? how many? when?).

⛰️

Be supportive

Use const, not let.

function getBookTitle(language, edition) {
    let languageText = `Eloquent ${language}`;
    let editionText = `${edition} edition`;
    return `${languageText} ${editionText}`;
}

I noticed that you used `let` in the function. Would it work for you to replace it with `const`? I know, it's so easy to forget! I suspect that you'll get used to it in no time.

It might be a little discouraging when there is a lot of issues with a pull request or when the same mistakes are repeated. Show a little empathy and tell the author it's not a big deal.

🆘

No acronyms

AFAIK LGTM but needs TLC IMO

function getBookTitle(language, edition) {
  return `Eloquent ${language}: ${edition} edition`;
}

Nice work! Let me know if you need a hand with some of my comments.

Refrain from using acronyms. You never know if the author of the pull request is familiar with the ones you use. Context might not always be enough to guess what you really mean. At best, they look the acronym up. At worst, they assume you mean something else and misunderstand your feedback.

🙉

Use emojis

+1

function getBookTitle(language, edition) {
  return `Eloquent ${language}: ${edition} edition`;
}

Looks good! 👍

Emojis can make your feedback a little more pleasant. Don't hesitate to add a smiley face or a thumb up to give it a warmer touch or illustrate your intention.

🚀

Shortcomings

  • Lengthy comments.
  • Takes a little extra time.
  • Might be a little too formulaic.
  • Might not fit the style of your client.
  • Sometimes, you don't feel like being nice.

💔

What else?

Resources

📚

Thank you!

Made with Slides.com