Why do we need code review?

Transcript

1. Why do we need to review code?
2. Agenda • Discuss why we need to review our code • Do some code review together, learn some good practices and assess their importance • Speak about how to review code sanely
3. What you guys think about code review
8. Pros and cons • Preventing potential issues • Knowledge sharing • Improve code • New ideas for solving tasks • Time-consuming • Subjective reviewers • Requires diving into a task • Non included in estimations • Big PRs • Human being factor • Code review stage has the highest time to action across other stages
9. Let’s review some code
10. Rules • It’s only about writing Ruby, nevermind performance/DB/HTTP and etc. • It’s pseudo code, if something is not clear, ask questions. The more questions, the better
11. Assessment criteria Can be noticed by linter? Shows whether an issue can be noticed by an automated tool locally or during CI runs Ease testing Shows whether it will be easier to test code if we notice and fix an issue Ease extensibility Shows whether it will be easier to extend code by adding new features if we noticed and fix an issue Ease readability Shows whether it will be easier to read if we fix an issue
14. Use #map instead of #each get a collection of new values Can be noticed by linter? Ease testing Ease extensibility Ease readability ? No No Yes
17. Use interpolation instead of concatenation Can be noticed by linter? Ease testing Ease extensibility Ease readability Yes No No ?
20. Use the new style of hash literals Can be noticed by linter? Ease testing Ease extensibility Ease readability Yes No No ?
23. Avoid unnecessary curly brackets when passing hashes to functions Can be noticed by linter? Ease testing Ease extensibility Ease readability Yes No No No
26. User symbol shorthand for blocks Can be noticed by linter? Ease testing Ease extensibility Ease readability Yes No No No
29. Guard clauses instead of nested conditions Can be noticed by linter? Ease testing Ease extensibility Ease readability Yes Yes Yes Yes
32. Predicates should return booleans Can be noticed by linter? Ease testing Ease extensibility Ease readability No Yes Yes ?
35. Null-object pattern. Returning nil from functions makes consumers check if a value is nil Can be noticed by linter? Ease testing Ease extensibility Ease readability No Yes Yes Yes
38. Dependency injection Can be noticed by linter? Ease testing Ease extensibility Ease readability No Yes Yes ?
41. Use keyword args or hashes for public methods with several args Can be noticed by linter? Ease testing Ease extensibility Ease readability No ? Yes Yes
44. Law of Demeter Can be noticed by linter? Ease testing Ease extensibility Ease readability No Yes Yes ?
47. Isolate code which is NOT under your control Can be noticed by linter? Ease testing Ease extensibility Ease readability No Yes Yes ?
50. Factory pattern Can be noticed by linter? Ease testing Ease extensibility Ease readability No Yes Yes ?
53. Interfaces Can be noticed by linter? Ease testing Ease extensibility Ease readability No Yes Yes Yes
54. How to improve a code review process?
56. 1. Automate everything you can • Configure rubocop as much as you can to avoid spending people’s time to review what could be reviewed by machines • Rubocop is not a single Linter • Write your own COPs and checks
57. 2. Introduce a design style guide A design style guide is a kinda code style guide but for architectural things which displays potential tasks developers might face and patterns which could solve a typical case easily
58. 3. Discuss architectural decisions beforehand • You can create implementation plans (IMPLANs) where you describe what and how you are going to solve • Create draft pull requests and ask colleagues to take a look at your intermediate and high-level solution before you implement everything wrong • Pair programming sessions
59. 4. Introduce a code style guide Inherit from the most popular style guides and adjust it according to your team’s preferences. If you have two different opinions, just vote.
60. 5. Everything that can’t be automated should be listed 1. Check business logic correspondence 2. Check design/architecture of an implemented solution 3. Check potential performance/security issues 4. Check tests 5. Check implementation details 6. Check code style/taste things 7. Leave your proposal or alternatives
61. 6. Treat code review as mandatory tasks • People tend to postpone code review till a manager asks them to review • It causes poor reviews because it’s being done in hurry How to fix: • Codeowners • Specify code reviewers explicitly(a codeowner + middle/senior dev) • Set a highest priority to code review
62. 7. Discuss everything in comments • Leave as much text comments as you can • Provide examples • Writing could take some time, write explanatory videos • As code authors highlight controversial or most probably comment things beforehand. Explain your decisions
63. Get rid of code review ;-)
64. Thanks!