How do I review the code

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.

9986d4cfe597f7ce12331ffdd75f471f?s=128

Denis Yagofarov

May 26, 2019
Tweet

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.