Upgrade to Pro — share decks privately, control downloads, hide ads and more …

Code Reviews

Code Reviews

When working in the cold and calculating space of computers, it can be easy to forget the emotions of our fellow colleagues. This is especially true when reviewing someone else's work. We need to be sensitive to one's time, effort, and thought while also providing constructive feedback. Emoticons can help express the connotation of feedback you are giving which is sometimes lost in written form. This talk discusses how to use emoticons to set the tone and provide clear visual indication for the context of your code review feedback.

Brooke Kuhlmann

August 10, 2021
Tweet

More Decks by Brooke Kuhlmann

Other Decks in Programming

Transcript

  1. "When a group of people fuse into a meaningful whole,

    the entire character of the work changes." https://www.alchemists.io/articles/peopleware Peopleware
  2. "When a group of people fuse into a meaningful whole,

    the entire character of the work changes." https://www.alchemists.io/articles/peopleware Peopleware "A jelled team is a group of people so strongly knit that the whole is greater than the sum of the parts."
  3. Insensitive Feedback No. This can't possibly work. I don't agree

    with these changes. This is unacceptable. You might as well do this over.
  4. Considerate Feedback Nice work and thanks for folding in the

    feedback. Have you thought about the security implications? What about using the following solution? Would you want to use the following instead? Maybe leverage this aspect of the architecture instead?
  5. Feedback Categories Blocking Non-blocking • Requires immediate attention. • Must

    be fi xed. https://www.alchemists.io/articles/code_reviews
  6. Feedback Categories Blocking Non-blocking • Requires immediate attention. • Must

    be fi xed. • Informative or suggestive. https://www.alchemists.io/articles/code_reviews
  7. Feedback Categories Blocking Non-blocking • Requires immediate attention. • Must

    be fi xed. • Informative or suggestive. • Author's discretion. https://www.alchemists.io/articles/code_reviews
  8. Feedback Categories Blocking Non-blocking • Requires immediate attention. • Must

    be fi xed. • Informative or suggestive. • Author's discretion. • Changes are encouraged. https://www.alchemists.io/articles/code_reviews
  9. Feedback Categories Blocking Non-blocking • Requires immediate attention. • Must

    be fi xed. • Informative or suggestive. • Author's discretion. • Changes are encouraged. • Discussion can continue. https://www.alchemists.io/articles/code_reviews
  10. Feedback Format <emoticon> <message>. 🎨 Would you want to use

    `Object#then` to avoid the temporary local variable? https://www.alchemists.io/articles/code_reviews
  11. Emoticons ℹ 💭 💡 ⚠ 🎨 🛑 🙇 ❇ 📛

    🪓 🍄 https://www.alchemists.io/articles/code_reviews
  12. Emoticons ℹ 💭 💡 ⚠ 🎨 🛑 🙇 ❇ 📛

    🪓 🍄 https://www.alchemists.io/articles/code_reviews
  13. Emoticons ℹ 💭 💡 ⚠ 🎨 🛑 🙇 ❇ 📛

    🪓 🍄 https://www.alchemists.io/articles/code_reviews
  14. Emoticons ℹ 💭 💡 ⚠ 🎨 🛑 🙇 ❇ 📛

    🪓 🍄 https://www.alchemists.io/articles/code_reviews Encouraging/Informational
  15. Emoticons ℹ 💭 💡 ⚠ 🎨 🛑 🙇 ❇ 📛

    🪓 🍄 https://www.alchemists.io/articles/code_reviews Encouraging/Informational Concerning/Actionable
  16. Emoticons ℹ 💭 💡 ⚠ 🎨 🛑 🙇 ❇ 📛

    🪓 🍄 https://www.alchemists.io/articles/code_reviews
  17. Emoticons ℹ 💭 💡 ⚠ 🎨 🛑 🙇 ❇ 📛

    🪓 🍄 https://www.alchemists.io/articles/code_reviews Blues / Whites
  18. Emoticons ℹ 💭 💡 ⚠ 🎨 🛑 🙇 ❇ 📛

    🪓 🍄 https://www.alchemists.io/articles/code_reviews Yellows
  19. Emoticons ℹ 💭 💡 ⚠ 🎨 🛑 🙇 ❇ 📛

    🪓 🍄 https://www.alchemists.io/articles/code_reviews Reds
  20. Emoticons ℹ 💭 💡 ⚠ 🎨 🛑 🙇 ❇ 📛

    🪓 🍄 https://www.alchemists.io/articles/code_reviews Greens (mixed)
  21. Information ℹ • Non-blocking. • Status update. • Generally doesn't

    need a response. https://www.alchemists.io/articles/code_reviews
  22. Information ℹ • Non-blocking. • Status update. • Generally doesn't

    need a response. :information_source: https://www.alchemists.io/articles/code_reviews
  23. Information (examples) ℹ Will rebase onto `main` once build is

    green. ℹ Rebasing to resolve upstream merge con fl icts. ℹ All feedback has been addressed and ready for re-review. ℹ
  24. Inquire 💭 • Non-blocking. • A chance to learn more.

    https://www.alchemists.io/articles/code_reviews
  25. Inquire 💭 • Non-blocking. • A chance to learn more.

    :thought_balloon: https://www.alchemists.io/articles/code_reviews
  26. 💭 We should consider leveraging the Command Pattern for these

    objects soon. 💭 Since this pattern exhibits The Rule of Three, should we plan to DRY this up soon? Inquire (examples) 💭
  27. Tip 💡 • Non-blocking or blocking. • A chance to

    educate. https://www.alchemists.io/articles/code_reviews
  28. Tip 💡 • Non-blocking or blocking. • A chance to

    educate. :bulb: https://www.alchemists.io/articles/code_reviews
  29. 💡You might want to study the use of whole value

    objects when it comes to immutability. 💡You might enjoy looking at existing examples of pattern matching. Tip (examples) 💡
  30. Style 🎨 • Non-blocking or blocking. • Improvements to style.

    https://www.alchemists.io/articles/code_reviews
  31. Style 🎨 • Non-blocking or blocking. • Improvements to style.

    :art: https://www.alchemists.io/articles/code_reviews
  32. 🎨 Would it be easier to inject your dependencies to

    avoid a tight coupling of these constants? 🎨 Could you use `%()` to avoid escaping quotes in this string? Style (examples) 🎨
  33. Warning ⚠ • Non-blocking or blocking. • Alert or caution.

    https://www.alchemists.io/articles/code_reviews
  34. Warning ⚠ • Non-blocking or blocking. • Alert or caution.

    :warning: https://www.alchemists.io/articles/code_reviews
  35. ⚠ You'll need to rebase in order to resolve upstream

    merge con fl icts based on the work Jake recently added. ⚠ Be aware that the work Jill is doing will soon require these objects to be updated. Warning (examples) ⚠
  36. 📛 In the fi rst sentence of your commit message,

    I think you mean to say "capital" instead of "capitol". 📛 I think you want to say "implemented" instead of "implemed". Typo (examples) 📛
  37. Cut 🪓 • Blocking. • Signi fi es text, code,

    or commit that can be deleted. https://www.alchemists.io/articles/code_reviews
  38. Cut 🪓 • Blocking. • Signi fi es text, code,

    or commit that can be deleted. :axe: https://www.alchemists.io/articles/code_reviews
  39. 🪓 No need to keep this object around anymore since

    it's not called anywhere else in the code base. 🪓 You can delete this spec since it's a duplicate of Line 12. Cut (examples) 🪓
  40. Architecture 🛑 • Blocking. • Signi fi es overarching architectural

    concerns. https://www.alchemists.io/articles/code_reviews
  41. Architecture 🛑 • Blocking. • Signi fi es overarching architectural

    concerns. :stop_sign: https://www.alchemists.io/articles/code_reviews
  42. 🛑 Could you break this object down into smaller objects

    since the specs are so complex and trying to tell you as much? 🛑 Could you inject all of these constants instead of embedding them? This would allow you to have an object that is more fl exible to change. Architecture (examples) 🛑
  43. Bonus 🍄 • Non-blocking. • Next level power up. :mushroom:

    https://www.alchemists.io/articles/code_reviews
  44. 🍄 There are several other places in the codebase that

    would bene fi t from this object. Reducing duplication would be a nice win as part of this work. 🍄 It would be awesome if you could take this work a step further and refactor all of the other objects in this namespace to use the same pattern. Bonus (examples) 🍄
  45. Favorite ❇ • Non-blocking. • Positive only. • Must be

    authentic. https://www.alchemists.io/articles/code_reviews
  46. Favorite ❇ • Non-blocking. • Positive only. • Must be

    authentic. :sparkle: https://www.alchemists.io/articles/code_reviews
  47. ❇ Looking forward to rebasing upon what you've done here

    since it'll make my job a lot easier! ❇ Excellent work folding in all of the feedback. Favorite (examples) ❇
  48. Thanks 🙇 • Non-blocking. • A chance to honor feedback.

    https://www.alchemists.io/articles/code_reviews
  49. Thanks 🙇 • Non-blocking. • A chance to honor feedback.

    :bow: https://www.alchemists.io/articles/code_reviews
  50. 🙇 Fixed. I meant to remove that commit but forgot.

    🙇 Fixed. Good eye. 🙇 Fixed. Nice catch. Thanks (examples) 🙇
  51. Resolving Feedback • Address quickly. • Split into smaller stories

    as necessary. https://www.alchemists.io/articles/code_reviews
  52. Guidelines • Lines of Code: ~300 lines (max) • Length

    of Time: ~1 day (max) https://www.alchemists.io/articles/code_reviews
  53. Recap • Use emoticons. 🎉 • Be considerate of other's

    work, ideas, and time. https://www.alchemists.io/articles/code_reviews
  54. Recap • Use emoticons. 🎉 • Be considerate of other's

    work, ideas, and time. • Always provide constructive feedback. https://www.alchemists.io/articles/code_reviews
  55. Recap • Use emoticons. 🎉 • Be considerate of other's

    work, ideas, and time. • Always provide constructive feedback. • Learn from one another and level up. https://www.alchemists.io/articles/code_reviews