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

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.

68d48587fc806c2b35eb9ff0b7ad8115?s=128

Mike Zornek

August 16, 2017
Tweet

Transcript

  1. Overcoming the Stress Surrounding Code Review, for the Betterment of

    Your Project and Career
  2. Michael Zornek

  3. What is code review?

  4. None
  5. None
  6. How does it work?

  7. master zorn/feature-123

  8. Why is it beneficial?

  9. Why is it stressful?

  10. None
  11. Responsibilities of the Poster

  12. Work is complete

  13. The Reviewer is not your QA

  14. PR is complete in detail

  15. <Story Title> [Delivers #xxxxx] ## Finishes https://www.pivotaltracker.com/story/show/xxxxx ## Additions/Changes *

    One * Two * Three ## Testing Verify this. Use real hardware because of x.
  16. <Story Title> As a user, I want FEATURE, So that

    I ACCOMPLISH_GOAL. Acceptance Criteria: * One * Two * Three
  17. <Story Title> As a user, I want FEATURE, So that

    I ACCOMPLISH_GOAL. Acceptance Criteria: * One * Two Moved: #1234 * Three
  18. Merge into master and kept up to date

  19. Consider PRE commenting lines of code that are interesting

  20. Responsibilities of the REVIEWER

  21. Be timely in your review

  22. Verify the behavior works as documented

  23. Verify edge cases around the behavior

  24. Review code quality

  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
  26. None
  27. Crafting productive criticism using plain text is hard

  28. Style Nitpicks
 (whitespace, indentation, etc.)

  29. Praise! 
 Today I LearneD!

  30. Too Many Cooks

  31. Responsibilities of the POSTER

  32. Be timely in your responses

  33. Do not take the criticism personally

  34. Don't be afraid to push back

  35. Talk in person when needed

  36. Crunch time reviewing

  37. None
  38. Don't skip reviewing

  39. Don't skimp reviewing

  40. None
  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.
  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
  43. Tips and Tricks

  44. None
  45. None
  46. No surprises

  47. Don't make BIG PRs

  48. Document your FIXME with URLs

  49. Find mostly regular reviewers.

  50. Solo Reviewing

  51. Thank You! Mike zornek