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.

Transcript

1. Code Reviews
   Presented by Brooke Kuhlmann
   

   View Slide

2. https://www.alchemists.io/articles/peopleware
   Peopleware
   

   View Slide

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

   View Slide

4. "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."
   

   View Slide

5. View Slide

6. TRUST
   

   View Slide

7. TRUST
   🔨
   🔧
   🪛
   

   View Slide

8. Building Trust
   

   View Slide

9. Building Trust
   👩💻
   💡
   Ideas Work
   ❤
   Compassion
   

   View Slide

10. Insensitive Feedback
    

    View Slide

11. 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.
    

    View Slide

12. Considerate Feedback
    

    View Slide

13. 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?
    

    View Slide

14. Feedback Categories
    Blocking Non-blocking
    https://www.alchemists.io/articles/code_reviews
    

    View Slide

15. Feedback Categories
    Blocking Non-blocking
    • Requires immediate attention.
    https://www.alchemists.io/articles/code_reviews
    

    View Slide

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

    View Slide

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

    View Slide

18. 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
    

    View Slide

19. 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
    

    View Slide

20. 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
    

    View Slide

21. Feedback Format
    https://www.alchemists.io/articles/code_reviews
    

    View Slide

22. Feedback Format
    .
    https://www.alchemists.io/articles/code_reviews
    

    View Slide

23. Feedback Format
    .
    🎨 Would you want to use `Object#then` to avoid the
    temporary local variable?
    https://www.alchemists.io/articles/code_reviews
    

    View Slide

24. Emoticons
    https://www.alchemists.io/articles/code_reviews
    

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

34. Emoticons
    ℹ
    💭
    💡
    ⚠
    🎨
    🛑
    🙇
    ❇
    📛
    🪓
    🍄
    https://www.alchemists.io/articles/code_reviews
    Greens
    
    (mixed)
    

    View Slide

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

    View Slide

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

    View Slide

37. Information
    (examples)
    ℹ
    

    View Slide

38. 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.
    ℹ
    

    View Slide

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

    View Slide

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

    View Slide

41. Inquire
    (examples)
    💭
    

    View Slide

42. 💭 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)
    💭
    

    View Slide

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

    View Slide

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

    View Slide

45. Tip
    (examples)
    💡
    

    View Slide

46. 💡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)
    💡
    

    View Slide

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

    View Slide

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

    View Slide

49. Style
    (examples)
    🎨
    

    View Slide

50. 🎨 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)
    🎨
    

    View Slide

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

    View Slide

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

    View Slide

53. Warning
    (examples)
    ⚠
    

    View Slide

54. ⚠ 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)
    ⚠
    

    View Slide

55. Typo
    📛
    • Blocking.
    • Corrects a mispelling.
    https://www.alchemists.io/articles/code_reviews
    

    View Slide

56. Typo
    📛
    • Blocking.
    • Corrects a mispelling.
    :name_badge:
    https://www.alchemists.io/articles/code_reviews
    

    View Slide

57. Typo
    (examples)
    📛
    

    View Slide

58. 📛 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)
    📛
    

    View Slide

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

    View Slide

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

    View Slide

61. Cut
    (examples)
    🪓
    

    View Slide

62. 🪓 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)
    🪓
    

    View Slide

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

    View Slide

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

    View Slide

65. Architecture
    (examples)
    🛑
    

    View Slide

66. 🛑 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)
    🛑
    

    View Slide

67. Bonus
    🍄
    • Non-blocking.
    • Next level power up.
    https://www.alchemists.io/articles/code_reviews
    

    View Slide

68. Bonus
    🍄
    • Non-blocking.
    • Next level power up.
    :mushroom:
    https://www.alchemists.io/articles/code_reviews
    

    View Slide

69. Bonus
    (examples)
    🍄
    

    View Slide

70. 🍄 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)
    🍄
    

    View Slide

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

    View Slide

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

    View Slide

73. Favorite
    (examples)
    ❇
    

    View Slide

74. ❇ 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)
    ❇
    

    View Slide

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

    View Slide

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

    View Slide

77. Thanks
    (examples)
    🙇
    

    View Slide

78. 🙇 Fixed. I meant to remove that commit but forgot.
    🙇 Fixed. Good eye.
    🙇 Fixed. Nice catch.
    Thanks
    (examples)
    🙇
    

    View Slide

79. Resolving Feedback
    • Address quickly.
    • Split into smaller stories as necessary.
    https://www.alchemists.io/articles/code_reviews
    

    View Slide

80. Cadence
    https://www.alchemists.io/articles/code_reviews
    

    View Slide

81. Cadence
    🌄 ☀ 🌃
    Dawn Noon Dusk
    https://www.alchemists.io/articles/code_reviews
    

    View Slide

82. Guidelines
    https://www.alchemists.io/articles/code_reviews
    

    View Slide

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

    View Slide

84. Guidelines
    • Lines of Code: ~300 lines (max)
    • Length of Time: ~1 day (max)
    https://www.alchemists.io/articles/code_reviews
    

    View Slide

85. Recap
    https://www.alchemists.io/articles/code_reviews
    

    View Slide

86. Recap
    • Use emoticons. 🎉
    https://www.alchemists.io/articles/code_reviews
    

    View Slide

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

    View Slide

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

    View Slide

89. 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
    

    View Slide

90. 🙇
    Thanks
    https://www.alchemists.io
    

    View Slide