Lecture 20: Code reviews

Collaboration and review

• Software development is collaborative
• You are collaborating across space, with others, and across time, with your future self who has forgotten most of the code’s context
• Engaging other people helps maintain this collaboration

Purposes of code reviews

• Enforce standards
• Consider alternate designs
• Expose bugs
• Suggest optimizations
• Keep people synchronized
• Maintain culture

Code review process

• Code review can be beautiful
• Code review can be painful
• Painful examples first

Sharing time 1

• The Linux kernel mailing list on my PhD thesis

• Dick Johnson

  Now, if you want to trash your copy of the Operating System with the output spewed from a C++ compiler, then I suggest you keep it real quiet. It is similar to "touching up" a famous painting with spray-paint, of defecating on a wedding cake.

  Again, writing a Linux kernel module in C++ demonstrates arrogance, absolutely, positively arrogance, and is an affront to the programmers who have dedicated major amounts of their time optimizing code execution in the kernel.

• Linus Torvalds

  In general, I'd say that anybody who designs his kernel modules for C++ is either (a) looking for problems (b) a C++ bigot that can't see what he is writing is really just C anyway (c) was given an assignment in CS class to do so.

• Linus and Linux are known for toxicity

  This week people in our community confronted me about my lifetime of not understanding emotions. My flippant attacks in emails have been both unprofessional and uncalled for. Especially at times when I made it personal. In my quest for a better patch, this made sense to me. I know now this was not OK and I am truly sorry.

  The above is basically a long-winded way to get to the somewhat painful personal admission that hey, I need to change some of my behavior, and I want to apologize to the people that my personal behavior hurt and possibly drove away from kernel development entirely.

  I am going to take time off and get some assistance on how to understand people’s emotions and respond appropriately.

Sharing time 2

• I try to get a patch into PHP
• It’s open source, so incentives get weird
• https://github.com/php/php-src/pull/1701

Sharing time 3

• People ask you to do stuff
• “Bikeshedding”
• https://github.com/KaTeX/KaTeX/pull/689
• Who was right?

Code review emotional guidelines

• Be mindful of the consequences of your comments
  • On the code
  • On people’s time, energy, motivation
• Focus on what matters, only secondarily on matters of taste
  • Too many code review guidelines fail to distinguish
  • Arbitrary barriers to membership in some clique
  • Counterpoint 1: Enforce style rules with tools
    • Shuts down pointless arguments
  • Counterpoint 2

9 Tips

• From Fog Creek Software
• For everyone
  1. Review the right things, let tools do the rest
  2. Everyone should code review
  3. Review all code
  4. Adopt a positive attitude
• For reviewers
  5. Code review often and for short sessions
  6. It’s OK to say “It’s all good”
  7. Use a checklist
• For submitters
  8. Keep the code short
  9. Provide context

A checklist

• From Fog Creek Software
• General
  • Does the code work? Does it perform its intended function, is the logic correct?
  • Is all the code easily understood?
  • Does it conform to your agreed coding conventions?
  • Is there any redundant or duplicate code?
  • Is the code as modular as possible?
  • Can any logging or debugging code be removed?
• Security
  • Are all data inputs checked (for the correct type, length, format, and range) and encoded?
  • Are returning errors being caught?
  • Are invalid parameter values handled?
• Documentation
  • Do comments exist and describe intent?
  • Are all functions commented?
  • Is any unusual behavior or edge-case handling described?
  • Are data structures and units of measurement explained?
  • Is there any incomplete code? Should it be removed or marked?
• Testing
  • Is the code testable?
  • Do tests exist and are they comprehensive?
  • Do unit tests actually test that the code is performing the intended function?

Today’s reviews

• Purpose
  • Pset 4 buffer cache
  • Especially (but not only) buffer cache synchronization and invariants
  • Style will be different—think positive!