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

Code Review is an Architectural Necessity - SATURN 2016

Code Review is an Architectural Necessity - SATURN 2016

There's a wealth of material on code review from a code quality standpoint, tracking a host of metrics and generating enough Big Data to employ a small army of analysts at some companies, I'm sure. But introducing code review at the architecture stage seems to be rarely done, maybe even sufficiently rare to qualify as novel. In this presentation, I intend to focus on some quality attributes valued by a team that conscientiously conducts code reviews, and that code review enables, but not guarantees, three main attributes in the team's systems' architecture: maintainability, compliance, and security.

I posit based on my own meandering experience across several projects, open and proprietary, that these quality attributes are enabled through code review and saved from being a poorly timed afterthought or patch onto an architecture.

Colin Dean

May 03, 2016
Tweet

More Decks by Colin Dean

Other Decks in Technology

Transcript

  1. Code review is an architectural necessity Colin Dean @colindean 1

  2. @ColinDean Software Engineer Organizer, Abstractions.io Wearer of many hats 2

  3. My words are my own and not my employer(s), past

    or present. Please save questions until the end of the presentation. 3
  4. Agenda • Quick anecdote • What is code review? •

    What problems does code review solve? • Quality attributes code review ensures • Tips for code reviews • Limitations 4
  5. 5

  6. Agenda • Quick anecdote • What is code review? •

    What problems do code review solve? • Quality attributes code review ensures • Tips for code reviews • Limitations 6
  7. What is code review? 7

  8. Code review is the process by which those who maintain

    a software codebase evaluate a proposed change to that codebase, regardless of the source of the proposed change. 8
  9. Code review is systematic examination of computer source code. 9

    Code Review, Wikipedia
  10. Peer Review 10

  11. Code Review 11

  12. Code Review Vocabulary • Change - an individual unit of

    work altering what exists • Submission - a collection of changes • Submitter - the person proposing the submission • Reviewer - the people evaluating the submission • Annotation - remarks or ratings bestowed upon the submission 12
  13. The submitter proposes changes in a submission, which is evaluated

    by a reviewer, who annotates or accepts it. 13
  14. Inspection Team review Walkthrough Pair programming Peer deskcheck, passaround Ad-hoc

    review Wiegers’ peer review formality spectrum 14 Least formal Most formal
  15. Most formal Least formal Inspection Team review Walkthrough Pair programming

    Peer deskcheck, passaround Ad-hoc review Wiegers’ peer review formality spectrum 15
  16. 16

  17. Agenda • Quick anecdote • What is code review? •

    What problems does code review solve? • Quality attributes code review ensures • Tips for code reviews • Limitations 17
  18. Code review solves two major problems. 18 Aside from the

    primary goal of reducing defects,
  19. Mental model synchronization Code review solves 19

  20. 20

  21. 21

  22. Close enough Need guidance On target 22

  23. Tribal knowledge development Code review solves 23

  24. Michael Keeling
 Creating an Architecture Oral History, SATURN 2012 “Architecture

    oral history requires that the team is both willing and able to retell the stories and keep the oral history alive.” 24
  25. Write it down. Make it searchable. Code review forces us

    to 25
  26. Agenda • Quick anecdote • What is code review? •

    What problems does code review solve? • Quality attributes code review ensures • Tips for code reviews • Limitations 26
  27. Maintainability Code review ensures 27

  28. Maintainability • Learnability • Understandability • Serviceability Code review drives

    28
  29. Learnability • Developing Code • Patterns & Conventions • Risks

    & Goals • Developing People • Common Vocabulary • Teaching Moments Maintainability Learnability Understandability Serviceability Code review drives 29
  30. Learner Expert Coding Reviewing Coding Reviewing Synchronous Pairing & Teaching

    Exemplary Reading Constructively Critical Evaluation Serendipitous Evaluation of Example Maintainability Learnability Understandability Serviceability 30
  31. Understandability • Establishes common yet evolving mental model • Builds

    confidence in direction and design decisions • Builds tribal knowledge • Bonus: Enables elevator pitch Maintainability Learnability Understandability Serviceability Code review drives 31
  32. Serviceability • Exposes addressable “gotchas” • Exposes end-user interaction points

    • Establishes consensus on supported workflows Maintainability Learnability Understandability Serviceability Code review drives 32
  33. Serviceability • Exposes addressable “gotchas” • Exposes end-user interaction points

    • Establishes consensus on supported workflows Maintainability Learnability Understandability Code review drives 32
  34. Linus’s Law “Given enough eyes, all bugs are shallow.” 33

  35. Maintainability ✓Learnability ✓Understandability ✓Serviceability Code review drives 34

  36. First programming job out of school - B2B imprinting company

    if($customer == “spacely_sprockets”) { do_something(); }
 else { cry(); } • Version control! • No code review tooling or process • Minimal pairing • Continous integration easily circumvented 35
  37. Lack of code review 36 Lost Opportunities

  38. Lack of code review 37 Lost Opportunities Lost Revenue

  39. Lack of code review Lost Opportunities Lost Revenue Lost Job

    38
  40. Compliance Code review ensures 39

  41. Compliance • Accessibility • Auditability • Idiomaticity Code review drives

    40
  42. Second job out of school - Consulting • Lone wolf

    working alongside other lone wolves • No version control in proprietary software with custom “IDE” a.k.a. textarea. • Last modified and modifier only • No process of our own 41
  43. First professional code review experience was group review • Subcontractor

    on government project, 2010-2012 • Lone SME on platform • Borland StarTeam + in house review system • My tools for version control integration • Weekly merge window • Round robin inspection 42
  44. 43

  45. Not a pleasant experience • Three to four hour weekly

    round robin inspection • Cutthroat mixture of competing contractors, subcontractors, and employees • Embarrassment galore ‛ Not a learning environment • Immediate defensive posture • “Merge next week” = you failed, possibly delayed project 44
  46. $1,450 per hour 45

  47. $1,450 per hour $5,800 per weekly meeting 46

  48. $1,450 per hour $5,800 per weekly meeting $290,000 per year

    47
  49. Effects? • Waste • “Get this over with.” • Obstructionism

    • Plenty of bugs • “I’ll fix that mistake later.” 48
  50. Missed opportunities • Accessibility expert was most vocal • Project

    manager was vocal on contractual and HF matters ➡ Both could have reviewed asynchronously • Project was behind ➡ Too many people could say No 49
  51. Security Code review ensures 50

  52. Security • Spot vulnerabilities • Teach best practices • Filter

    unnecessary code • YAGNI Code review drives 51
  53. Reviewers are like your lawyer Screening and recommending actions to

    minimize risk, avoid preventable mistakes 52
  54. Agenda • Quick anecdote • What is code review? •

    What problems does code review solve? • Quality attributes code review ensures • Tips for code reviews • Limitations 53
  55. When should you integrate code review? 54

  56. Context • Project • Technical 55

  57. Keep reviews informal and short. 56

  58. Tips for thorough code review 57

  59. Tips for thorough code review • Devote time 57

  60. Tips for thorough code review • Devote time • Accept

    debt 57
  61. Tips for thorough code review • Devote time • Accept

    debt • Identify churn 57
  62. Tips for thorough code review • Devote time • Accept

    debt • Identify churn • Minimize pedantry 57
  63. Tips for thorough code review • Devote time • Accept

    debt • Identify churn • Minimize pedantry • Make progress 57
  64. Major things we look for • Algorithmic complexity • Exception

    & error handling • Exception, class, & variable naming • Logging sufficiency & level • Style conformation (automate!) • Long lines & methods • Readability • Single purpose per commit 58
  65. Most importantly 59

  66. Most importantly Does it work? 59

  67. Most importantly Does it work? Is it tested? 59

  68. Agenda • Quick anecdote • What is code review? •

    What problems does code review solve? • Quality attributes code review ensures • Tips for code reviews • Limitations 60
  69. Analyze dynamic structures Code review cannot 61

  70. Go on endlessly Code review cannot 62

  71. Solve political problems Code review cannot 63

  72. Agenda • Quick anecdote • What is code review? •

    What problems do code review solve? • Quality attributes code review ensures • Tips for code reviews • Limitations 64
  73. Code Review 65

  74. Code Review is systemic examination of proposed changes to a

    codebase. 65
  75. Code Review is systemic examination of proposed changes to a

    codebase. solves mental model synchronization and tribal knowledge development. 65
  76. Code Review is systemic examination of proposed changes to a

    codebase. solves mental model synchronization and tribal knowledge development. ensures maintainability, compliance, & security. 65
  77. Code Review is systemic examination of proposed changes to a

    codebase. solves mental model synchronization and tribal knowledge development. ensures maintainability, compliance, & security. must be short, thorough, and automated where possible. 65
  78. Code Review is systemic examination of proposed changes to a

    codebase. solves mental model synchronization and tribal knowledge development. ensures maintainability, compliance, & security. must be short, thorough, and automated where possible. will not solve all human problems, but some is better than none. 65
  79. 66

  80. 1,500+ software professionals in Pittsburgh in August abstractions.io @abstractionscon 66

  81. @ColinDean github.com/
 colindean/talks speakerdeck.com/ colindean 67

  82. FIN 68

  83. Attributions • Westminster College picture: https://www.flickr.com/photos/westminstercollege/15759678054/in/album-72157649340620016/ • RMU picture: http://cfbarchitects.com/higher-education/selected-projects/academic-buildings-libraries-learning-commons/robert-

    morris-university/ • Pittsburgh picture: probably Dave DiCello • On switch https://openclipart.org/detail/180085/switch-on • Off switch https://openclipart.org/detail/180084/switch-off • “Their first code review” http://classicprogrammerpaintings.tumblr.com/post/142702963264/their-first-code-review-william- frederick • Bass, Len; Paul Clements, and Rick Kazman. Software Architecture in Practice. Addison Wesley, 2013. • Wiegers, Karl E. Peer Reviews in Software. Addison Wesley, 2012. • Cohen, Jason, Steven Teleki, and Eric Brown. Best Kept Secrets of Peer Code Review. Smart Bear Software, 2006. • Wilhelm, Alex and Alexia Tsotsis. Julie Ann Horvath Describes Sexism and Intimidation behind Her Github Exit. TechCruch, 2014 March 15. Retrieved 2016 April 26. http://techcrunch.com/2014/03/15/julie-ann-horvath-describes-sexism-and-intimidation- behind-her-github-exit/ • and others mentioned in the slides 69
  84. No, really. Fin. Srsly. 70

  85. Third out of school and current job - Engineering •

    Highly disciplined team using Java, Scala, and Groovy • Git + Gerrit • Constructively critical feedback • No criticism without alternative solution and reasoning • Wide experience range: 1-2 yrs to 25+ yrs • Team split in late 2014, I was asked to be tech lead 71
  86. Github Enterprise in 2016 • All new projects • Same

    workflow as public Github 72
  87. Code Review Tools Used Haven’t Used Like Dislike ̣Github ̣Gerrit

    ̣Gitlab •Gitbucket •BitBucket ̣StarTeam •Phabricator •git-assess 73