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

Code Reviews FTW!

David Majda
September 21, 2013

Code Reviews FTW!

About a year and half ago, we started to use code reviews in several teams in SUSE. What experience did we gain and what benefits do we see? How does a good review look like? What should a reviewer focus at and what should he/she avoid? And do code reviews make sense for you? We will try to answer all these questions.

Presented at WebExpo Prague 2013.

Details & video: http://webexpo.net/prague2013/talk/code-reviews-ftw/

David Majda

September 21, 2013
Tweet

More Decks by David Majda

Other Decks in Programming

Transcript

  1. Prague, 21st September 2013 David Majda @dmajda Code Reviews FTW!

  2. #1 Ugly Code?

  3. #2 Hard to Start?

  4. #3 Nobody Understands?

  5. What? 1

  6. Code review = code checked by other person

  7. Why? 2

  8. Code Quality

  9. Encourage Learning

  10. Bus Factor

  11. SUSE Way 3

  12. None
  13. Code Change Pull Request Review Merge Accept Reject

  14. My Way 4

  15. Do I Understand It?

  16. David's Personal Code Review Checklist

  17. High-level Checklist

  18. High-level Checklist ☑ Best approach & design?

  19. High-level Checklist ☑ Best approach & design? ☑ Proper level

    of abstraction?
  20. High-level Checklist ☑ Best approach & design? ☑ Proper level

    of abstraction? ☑ Change isolated?
  21. Mid-level Checklist

  22. Mid-level Checklist ☑ Already seen this?

  23. Mid-level Checklist ☑ Already seen this? ☑ Easy to maintain?

  24. Mid-level Checklist ☑ Already seen this? ☑ Easy to maintain?

    ☑ Easy to extend?
  25. Mid-level Checklist ☑ Already seen this? ☑ Easy to maintain?

    ☑ Easy to extend? ☑ Not overengineered?
  26. Mid-level Checklist ☑ Already seen this? ☑ Easy to maintain?

    ☑ Easy to extend? ☑ Not overengineered? ☑ Boy scout rule?
  27. Low-level Checklist

  28. Low-level Checklist ☑ Are erros handled?

  29. Low-level Checklist ☑ Are erros handled? ☑ Covered by tests?

  30. Low-level Checklist ☑ Are erros handled? ☑ Covered by tests?

    ☑ Follows coding style?
  31. Low-level Checklist ☑ Are erros handled? ☑ Covered by tests?

    ☑ Follows coding style? ☑ Bugs: other occurrences?
  32. Social Aspects 5

  33. None
  34. None
  35. None
  36. None
  37. David Majda @dmajda