How do I review the code

All of a sudden I review more and more code while working for solarisBank as a Staff Engineer. This presentation is about getting the best out of code reviews and being a helpful teammate. ... and still, have fun from your job.

Transcript

1. How do I review the code Denys Yahofarov May 2019 ❤
2. 05.06 /me • I’ve Wrote a lot of code in Ruby, Scala, and JavaScript • Seen some awesome and crazy things (in so@ware engineering) • Now working for solarisBank as an engineer • My team builds [payment] cards transacHon processing
3. 05.06 self.talk • My own experience • Non-technical and completely subjec9ve • Use at your own risk • All stunts were done by me, but hey… I’m s9ll alive
4. What kind of code? • Different teams • Different products !" #$ %& '( )*
5. What kind of code? • Different teams • Different products • Different urgency • … and everything is connected !" #$ %& '( )*
6. Why reviewing at all? • For sharing the knowledge • Learning and coaching • Discussing solu5ons async, close to the code • Keeping the codebase healthy • A regulatory requirement in
7. My principles
8. The code brings value in master, deployed to production* * Unless it’s an PoC
9. No software is perfect
10. Contributors deserve respect. No ma3er who they are
11. Think twice before posting a comment
12. Time is precious. Going back and forth with comments is painful
13. No matter how “bad” the code is, there should be something good
14. Code in master brings value • Acceptance criteria should be covered. In tests • No questions regarding the syntax and style – it should be automated • Merge PR straight away if it’s good to go • … and I can always open a PR into the original one. Asking for feedback • Every approval is a tradeoff between "no value" and "added technical debt
15. No software is perfect • Comment only about the most important things • Mark if the feedback s8ll lets merging the PR • … and approve merge, but yet list things to improve • I don’t always refer to “Boy-scout” rule • I leave comments even if PR is merged already
16. Respecting people • My assump)ons: • The engineer did the best thing he or she could • And don’t make the same mistakes mul)ple )mes on purpose • And I don’t have a full context. Nor looked into all possibili)es ways of solving the problem • This means “I can be wrong in my judgments”: • Use benchmarks • Provide metrics (code quality, test coverage) • I ask ques)ons to get a full understanding of the context
17. BTW, who had one of?.. !,",⁉ or $,% and & As an only comment to your PR
18. Commenting and being clear • ! is ❌ a #$ (yet) • Use simple English • And, yes, it’s OK to add Emoji too • Provide sample code. Not rewriAng a big part, however • Leave a final comment in PR, summarize your comments
19. Save &me on async and sync communica&ons • When I comment, it’s now my responsibility too to clear it out • It may take just 10 minutes for two people to resolve a long-going conversation • … and add the recap into the PR
20. Be positive • No ma&er, how “bad” it is, there should be something good. Find it and appreciate it.
21. ”I’d better do it myself” - does not scale
22. Act the same way you expect other people to act
23. What do you think?
24. Thank You.