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

Code Reviews: A critical part of the development process

Code Reviews: A critical part of the development process

Presented at a Mediacurrent knowledgeshare

Mark Shropshire

October 11, 2016
Tweet

More Decks by Mark Shropshire

Other Decks in Technology

Transcript

  1. Intros 2 Mark Shropshire (shrop) Mark brings 20 years of

    experience leading technical teams to his role as Mediacurrent’s Open Source Security Lead. He is a leader in tech community organizing, blogging, podcasting, and public speaking within the Drupal community. Mark is passionate about architecting systems to solve workflow problems and improve efficiencies using open source software. Mark is the maintainer of the Gaurdr Drupal security module suite. Over his 20 year career leading technical teams, Mark gained experience in IT roles at a large urban research university and nationally recognized, award winning graphic communications company. Open Source Security Lead @shrop /in/markshropshire shrop
  2. About 3 Mediacurrent helps organizations build highly impactful, elegantly designed

    Drupal websites that achieve the strategic results they need. • Single-source provider • Specializing in Drupal since 2007 • Headquartered in Atlanta, GA • Team of 70+ Drupal Experts including development, design and strategy • Clients include: Large Enterprise and high-profile global brands
  3. Style Guide Contents What’s a code review? Team roles What

    to review 4 3 2 1 4 Why does it matter?
  4. 6 A peer review of software source code to find

    mistakes, issues, and refinement possibilities that provide the opportunities to improve the quality, readability, and security of a codebase. Credit: Photo by nyuhuhuu on Flickr
  5. 8 Readability • Improves ongoing maintenance • “Pay” upfront to

    “save” later • Future developers (could be you) will thank you • Makes the code’s purpose more clear Why does it matter?
  6. 9 Knowledge Sharing • Learn from reviews (developer and the

    reviewer) • Mentor developers • Expand team knowledge of the codebase • Take time off! Why does it matter?
  7. 10 Other Benefits • Consistency • Identify bugs before testing

    and deployment • Accessibility • Performance • Find security issues before they find you Why does it matter?
  8. 14 Developer • Complete the feature work or hotfix •

    Perform local testing • Check coding standards • Properly annotate commits Team roles
  9. 15 Developer • Create a pull request ◦ Keep PRs

    small ◦ Assign to team members and others who have domain knowledge ◦ Add additional notes to the PR for context • Correct code review findings and continue review • Document details in the appropriate issue queue • Reinforce the process with the team Team roles
  10. 16 Project Manager • Reinforce importance of reviews with stakeholders

    • Reinforce importance of reviews with team • Assist developers with obtaining reviewers if code review is urgent • Don’t allow steps of the process to be skipped or short-changed • Reinforce the process with the team Team roles
  11. 17 QA Tester • Were the features being tested code

    reviewed? • Did the developer test local? • Can tests be automated? • Reinforce the process with the team Team roles
  12. 18 Reviewer • Take the review process seriously! ◦ Don’t

    approve quickly because there seems to be a sense of urgency ◦ If you can’t put in the time necessary, help find another reviewer • Check out the code branch • Consider in-person reviews with the developer • Consider a co-review with another developer Team roles
  13. 19 Reviewer • Take breaks as necessary • Add comments

    to the PR for follow up by the developer ◦ Constructive ◦ Appropriate level of detail ◦ Clarify if a fix is required to pass review or informational • Click approve when ready to keep the process moving • Reinforce the process with the team Team roles
  14. 20 No pointy fingers! Learn from mistakes as a team.

    Take ownership of mistakes. Credit: Photo by J E Theriot on Flickr
  15. 22 Code Efficiency and Effectiveness • Can the code be

    streamlined? • Any obvious logic errors? • Best practices • Reuse of code • Functions too long/abstraction What to review
  16. 23 Comments • Is code properly commented? ◦ Do the

    comments explain what is happening? ◦ Would you be able to pick up this code in 6 months and understand it? What to review
  17. 24 Technical Debt • Identify technical debt ◦ Should it

    be removed? ◦ Does the team agree that it is an acceptable risk? ◦ Create tickets to deal with technical debt so it isn’t forgotten What to review
  18. 25 Security • Code that isn’t secure against common exploits

    ◦ XSS ◦ SQL Injection • Permission checks • Backdoors ◦ Intentional code to allow non-authorized access • Writing secure code for Drupal 7 and Drupal 8 What to review
  19. 27 Coding Standards • https://www.drupal.org/docs/develop/standards • Developers check with static

    analysis tools like phpcs prior to review • Jenkins and other CI tools can automate analysis What to review