Overcoming the stress surrounding Code Review, for the betterment of your project and career

Code Review is a practice where before a change is made to a code base, the code is first posted somewhere for peer review and critique. Code Review is an extremely productive way to catch problems before they are delivered to users as well as help individuals mature as programmers. In this talk we’ll explore Code Review by documenting the responsibilities of those involved, the person posting the code, the person (or people) reviewing the code, and then again back to the poster, as they react to the feedback given. In addition to the raw process of these stages we’ll also review the very human side of Code Review using real world stories, the good, the bad and the ugly. We’ll close with more general tips and tools that can help, as well as cover some of the how and why you might want to utilize these practices even in your own solo work. The best audience for this talk are people who are looking to improve their personal or team code processes. Those who attend will leave with very actionable strategies to execute productive code review on their own projects.

Transcript

1. Overcoming the Stress Surrounding
   Code Review, for the Betterment of
   Your Project and Career
   

   View Slide

2. Michael Zornek
   

   View Slide

3. What is code review?
   

   View Slide

4. View Slide

5. View Slide

6. How does it work?
   

   View Slide

7. master
   zorn/feature-123
   

   View Slide

8. Why is it beneficial?
   

   View Slide

9. Why is it stressful?
   

   View Slide

10. View Slide

11. Responsibilities of
    the Poster
    

    View Slide

12. Work is complete
    

    View Slide

13. The Reviewer is not your QA
    

    View Slide

14. PR is complete in detail
    

    View Slide

15. [Delivers #xxxxx]
    ## Finishes
    https://www.pivotaltracker.com/story/show/xxxxx
    ## Additions/Changes
    * One
    * Two
    * Three
    ## Testing
    Verify this. Use real hardware because of x.
    

    View Slide

16. 
    As a user,
    I want FEATURE,
    So that I ACCOMPLISH_GOAL.
    Acceptance Criteria:
    * One
    * Two
    * Three
    

    View Slide

17. 
    As a user,
    I want FEATURE,
    So that I ACCOMPLISH_GOAL.
    Acceptance Criteria:
    * One
    * Two Moved: #1234
    * Three
    

    View Slide

18. Merge into master and
    kept up to date
    

    View Slide

19. Consider PRE commenting lines of
    code that are interesting
    

    View Slide

20. Responsibilities of
    the REVIEWER
    

    View Slide

21. Be timely in your review
    

    View Slide

22. Verify the behavior works
    as documented
    

    View Slide

23. Verify edge cases
    around the behavior
    

    View Slide

24. Review code quality
    

    View Slide

25. • Hard to comprehend
    • Single
    Responsibility
    Principal
    • Code Duplication
    • Boy Scout Rule
    • Off by One Errors
    • Large Input Errors
    • Error Handling
    • Performance
    • Old Devices
    • Low Memory
    • Method Names
    • Variable Names
    • Method Length
    • File Length
    • Docstrings
    • NSLocalizedStrings
    • Unit Tests
    • Testing Coverage
    

    View Slide

26. View Slide

27. Crafting productive criticism
    using plain text is hard
    

    View Slide

28. Style Nitpicks

    (whitespace, indentation, etc.)
    

    View Slide

29. Praise! 

    Today I LearneD!
    

    View Slide

30. Too Many Cooks
    

    View Slide

31. Responsibilities of
    the POSTER
    

    View Slide

32. Be timely in your responses
    

    View Slide

33. Do not take
    the criticism personally
    

    View Slide

34. Don't be afraid to push back
    

    View Slide

35. Talk in person when needed
    

    View Slide

36. Crunch time reviewing
    

    View Slide

37. View Slide

38. Don't skip reviewing
    

    View Slide

39. Don't skimp reviewing
    

    View Slide

40. View Slide

41. You see, their morals, their code, it’s a
    bad joke. Dropped at the first sign of
    trouble. They're only as good as the world
    allows them to be. I'll show you. When the
    chips are down, these... these civilized
    people, they'll eat each other.
    

    View Slide

42. Accepts merges that…
    • add errors / stops the build
    • add warnings
    • break user behavior
    • are not associated with a
    ticket/story/sprint
    • have not been code reviewed
    • are not backed up by new
    tests
    • does not contain inline
    method documentation
    

    View Slide

43. Tips and Tricks
    

    View Slide

44. View Slide

45. View Slide

46. No surprises
    

    View Slide

47. Don't make BIG PRs
    

    View Slide

48. Document your
    FIXME with URLs
    

    View Slide

49. Find mostly regular reviewers.
    

    View Slide

50. Solo Reviewing
    

    View Slide

51. Thank You!
    Mike zornek
    

    View Slide