The reviewer checklist
Download as pptx, pdf1 like517 views
This document discusses how to be a better code reviewer. It begins by defining code review and describing different types of code review like formal inspections, pull requests, and pair programming. It then provides general rules for code reviewing like managing time, providing quick feedback, and making it about improving code not blaming authors. The document outlines areas a reviewer should focus on like best practices, architecture, security, and performance. It concludes by presenting tools that can help with code review and automation.
The reviewer checklist
- 1. Nicola Pietroluongo @niklongstone THE REVIEWER CHECKLIST How to be a better reviewer
- 2. Nicola Pietroluongo @niklongstone Im really good at stuff until people watch me do that stuff. -Nerd Anatomy
- 3. Nicola Pietroluongo @niklongstone Nicola Pietroluongo @niklongstone Lead developer/Solution architect Tech article writer Open Source contributor Author of Learning PHP7 by Packt Publishing +10y of PHP programming Amazon Alexa skill challenge award winner
- 4. Nicola Pietroluongo @niklongstone Main areas What we are going to cover Introduction to code review The checklist Tools and conclusions
- 5. Nicola Pietroluongo @niklongstone 1. What code review is Introduction to code review, things to do before
- 6. Nicola Pietroluongo @niklongstone Code review is systematic examination of computer source code. It is intended to find mistakes overlooked in the initial development phase, improving the overall quality of team and software. -Wikipedia
- 7. Nicola Pietroluongo @niklongstone TYPES OF CODE REVIEW
- 8. Nicola Pietroluongo @niklongstone Formal Inspection / Fagan Inspection Meeting http://www.mfagan.com/pdfs/software_pioneers.pdf https://www.cs.umd.edu/class/spring2005/cmsc838p/VandV/fagan.pdf OverviewPlanning Preparation Rework
- 12. Nicola Pietroluongo @niklongstone FeedbackSpot the error Fix TIME Key points
- 13. Nicola Pietroluongo @niklongstone Pair Programming TIME Feedback Spot the error Fix
- 16. Nicola Pietroluongo @niklongstone Time is money -Your boss when youre late
- 17. Nicola Pietroluongo @niklongstone Manage your time Provide quick feedback Inspection rate 250 lines/hours Review 200-400 lines per review session Quick Feedback
- 18. Nicola Pietroluongo @niklongstone Capture metrics Define goals Lines Of Code Function Point Defect Density Risk Density Code Coverage Defect Detection Rate Defect Correction Rate Cyclomatic Complexity
- 19. Nicola Pietroluongo @niklongstone Cyclomatic Complexity Thomas J. McCabe, Sr. 1976 E: Edges N: Nodes P: Number of exit points (CC) = E - N + 2P http://www.literateprogramming.com/mccabe.pdf http://www.mccabe.com/pdf/More Complex Equals Less Secure-McCabe.pdf A B C D E
- 20. Nicola Pietroluongo @niklongstone Cyclomatic Complexity A>B B==1 END IF END IF A=BB=0 7 Edges 6 Nodes CC = 7- 6 + 2 = 3 T TF F
- 21. Nicola Pietroluongo @niklongstone Its a Bug Hunt NOT a Blame Game
- 22. Nicola Pietroluongo @niklongstone WHEN QUICKLY DECLINE
- 23. Nicola Pietroluongo @niklongstone When the pull request is out of scope you should _______ When quickly decline?
- 24. Nicola Pietroluongo @niklongstone When the pull request is out of scope you should DECLINE When quickly decline?
- 25. Nicola Pietroluongo @niklongstone When the code has no test coverage you should _______ When quickly decline?
- 26. Nicola Pietroluongo @niklongstone When the code has no test coverage you should DECLINE When quickly decline?
- 27. Nicola Pietroluongo @niklongstone When you dont like the solution provided you should _______ When quickly decline?
- 28. Nicola Pietroluongo @niklongstone When you dont like the solution provided you should _______ When quickly decline?
- 30. Nicola Pietroluongo @niklongstone 2. The checklist Reviewers responsibilities
- 31. Nicola Pietroluongo @niklongstone The checklist What we will cover Best Practices Foundation Architecture Key Areas (Security, Logging, Performances)
- 32. Nicola Pietroluongo @niklongstone BEST PRACTICES
- 33. Nicola Pietroluongo @niklongstone Fat model, skinny controller -Weight Watchers
- 34. Nicola Pietroluongo @niklongstone Services config # Sf 2.8 app/config/services.yml services: # keep your service names short app.slugger: class: AppBundleUtilsSlugger # Sf 3.3 app/config/services.yml services: # use the fully-qualified class name as the service id AppBundleUtilsSlugger: public: false
- 35. Nicola Pietroluongo @niklongstone Adopt whats make your teams life easier. Best practices
- 36. Nicola Pietroluongo @niklongstone Controller as a Service class EventController { //... public function __construct( TemplatingEngine $templating Router $router, LoggerInterface $logger, EventRepository $eventRepository ) //... public function indexAction($id) { //...
- 38. Nicola Pietroluongo @niklongstone Parameters/ Return Types /** * Get options. * * @param string $name * @param mixed $value * * @return mixed $option */ public function getOption($name, $value) { //...
- 43. Nicola Pietroluongo @niklongstone Never assume anything -Law enforcement
- 44. Nicola Pietroluongo @niklongstone To bundle or not to bundle, that is the question -Sf Hamlet
- 46. Nicola Pietroluongo @niklongstone KEY AREAS
- 47. Nicola Pietroluongo @niklongstone Security CSRF token # app/config/security.yml security: # ... firewalls: secured_area: # ... form_login: # ... csrf_token_generator: security.csrf.token_manager # twig template {# ... #} <form action="{{ path('login') }}" method="post"> {# ... the login fields #} <input type="hidden" name="_csrf_token" value="{{ csrf_token('authenticate') }}" > <button type="submit">login</button> </form>
- 48. Nicola Pietroluongo @niklongstone OWASP - Code Review Guide https://www.owasp.org/index.php/Category:OWASP_Code_Review_Project
- 49. Nicola Pietroluongo @niklongstone Syslog Message Severities RFC 5424 Numerical Code Severity 0 Emergency: system is unusable 1 Alert: action must be taken immediately 2 Critical: critical conditions 3 Error: error conditions 4 Warning: warning conditions 5 Notice: normal but significant condition 6 Informational: informational messages 7 Debug: debug-level messages
- 50. Nicola Pietroluongo @niklongstone Syslog Message Severities RFC 5424 Numerical Code Severity 0 Emergency: system is unusable 1 Alert: action must be taken immediately 2 Critical: critical conditions 3 Error: error conditions 4 Warning: warning conditions 5 Notice: normal but significant condition 6 Informational: informational messages 7 Debug: debug-level messages
- 51. Nicola Pietroluongo @niklongstone I want it all, I want it now -Queen
- 52. Nicola Pietroluongo @niklongstone Website Performance source : Akamai.com
- 53. Nicola Pietroluongo @niklongstone Loops A) for ($i = 0; $i < count($array); $i++) { B) $total = count($array); for ($i = 0; $i < $total; $i++) {
- 54. Nicola Pietroluongo @niklongstone 3. Tools Tools and automation
- 61. Nicola Pietroluongo @niklongstone PHPMD $ ./phpmd.phar filesOrDir reportFormat rules $ ./phpmd.phar fileA.php,fileB.php text cleancode,codesize
- 62. Nicola Pietroluongo @niklongstone Git diff $ git diff --name-only --diff-filter=d master src/CinemaBundle/Controller/CinemaController.php src/CinemaBundle/Entity/Screening.php src/CinemaBundle/Entity/ScreeningVenue.php src/CinemaBundle/Entity/Showing.php
- 63. Nicola Pietroluongo @niklongstone Sed $ sed ':a; $!N; s/n/,/; ta' :a creates a pattern $!N appends lines to the pattern if not last line s/n/,/ replaces new line with comma ta repeat the a
- 64. Nicola Pietroluongo @niklongstone Pull it together *All in one line ./phpmd.phar $(git diff --name-only --diff-filter=d master | sed ':a; $!N; s/n/,/; ta') text cleancode,codesize
- 65. Nicola Pietroluongo @niklongstone Automate, automate, automate. Allright, allright, all right. -Matthew McSymfony
- 66. Nicola Pietroluongo @niklongstone LIGHT TOUCH REVIEW
- 67. Nicola Pietroluongo @niklongstone Light touch review Inspect whats critical Automation High code coverage Small and contained release Good knowledge of the project, business and practices
Editor's Notes
- #3: I'd like to share a secret with you: I think code review is really boring thats why Ive created this talk. Before starting I have a little question for you Who likes code review? (show of hands)Who thinks that code review improves only software quality? We will see during the course of this session why I asked this question.
- #33: most effective procedure to build clean and less complex web app
- #35: appname.location.servicename to avoid collisions. We will see more examples of how the target has been adjusted based on the community considerations, based on the real usage of the framework.
- #36: Its a little bit like the bubble wrap story. Bubble wrap invented in the 1957 was at the beginning advertised as wallpaper. Can you imagine you put your hand on the wall and pick pock. Then when IBM launched the 1401 computer bubble wrap was used for the shipping.
- #38: about to review the core functionalities of classes and methods
- #41: JsonDecode class
- #42: about to review the core functionalities of classes and methods
- #43: about to review the core functionalities of classes and methods
- #47: about to review the core functionalities of classes and methods
- #49: about to review the core functionalities of classes and methods
- #50: Symfony comes with Monolog under the PSR-3 the Logger Interface that follows the log levels described by the RFC 5424 for the syslog protocol.
- #63: In the review process you dont need to run a complete analysis of all your code base but only what is included in a pull request.