Code Reviews: A critical part of the development process

Presented at a Mediacurrent knowledgeshare

Transcript

1. A critical part of the development process Code Reviews
2. 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
3. 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
4. Style Guide Contents What’s a code review? Team roles What to review 4 3 2 1 4 Why does it matter?
5. What’s a code review? 1
6. 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
7. 2 Why does it matter?
8. 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?
9. 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?
10. 10 Other Benefits • Consistency • Identify bugs before testing and deployment • Accessibility • Performance • Find security issues before they find you Why does it matter?
11. 3 Team roles
12. “I don’t have time.” Teams should make code reviews a requirement
13. 13 Credit: Photo by eu on Flickr
14. 14 Developer • Complete the feature work or hotfix • Perform local testing • Check coding standards • Properly annotate commits Team roles
15. 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
16. 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
17. 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
18. 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
19. 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
20. 20 No pointy fingers! Learn from mistakes as a team. Take ownership of mistakes. Credit: Photo by J E Theriot on Flickr
21. 4 What to review
22. 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
23. 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
24. 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
25. 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
26. 26 Additional Considerations and Concerns • Accessibility concerns? • Performance • Tests (unit/BDD) What to review
27. 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
28. @Mediacurrent Mediacurrent.com Thank you! slideshare.net/mediacurrent