Skip to content

week 11

Jump to bottom
Duke Manh edited this page Nov 24, 2021 · 2 revisions

Week 11

TODO

  • Do the Case Study Reading on Code Review at Google
  • Complete Lab 10
  • Begin working on Release 0.4. By the end of the week you should have a plan about what you'll do, how you'll do it, and your goals. You should have your first planning blog post added to the 0.4 wiki page this week.

Readings: Code Review at Google

Case Study: Code Review at Google

Google’s “senior” Rule for code review is: consider approving a change once it is definitely improving the overall code health, even if it isn’t perfect--you can iterate on it later. “There is no such thing as perfect code, only better code.”

As an individual, junior developer, you can learn a lot by examining the software development practices of larger projects and companies. At Google code review is a critical part of maintaining software quality. Google's best practices for giving and receiving a code review are public, and we'll look at a number of different aspects.

You may not be doing a lot of code review yet, but you are submitting your code for review on a regular basis, and need to understand how to improve the process.

NOTE: please read the linked sections as well as the highlighted notes that I've supplied.

1. Creating Reviewable Code

  • You are making a change that will become a permanent part of the history of the project
  • In the future developers will try to find this change, searching through the old commits, Pull Requests, and Issues. Make sure they can find it! You need to supply enough detail to search for things later.
  • Your Pull Request Title should:
    • use a short summary of what is being done.
    • be a complete sentence
    • Examples:
      • Bad: “Fix build” or “fix 123”
      • Good: “remove size limit on RPC server message freelist” or “Construct a Task with a TimeKeeper to use its TimeStr and Now methods"

2. Preferring Small Changes

  • Reviewing a small change, or multiple small changes separated into different PRs, is much easier than 1 larger PR.
  • The more code in a change, the less likely the reviewer is to spend time on each line
  • Smaller changes introduce fewer bugs. More code == more bugs added. (Microsoft study in 1992 found that there were 10 - 20 bugs per every 1,000 lines of code)
  • Smaller changes are easier to throw away and not feel like you wasted your time. A reviewer might say ‘no’ to your change.
  • Easier to merge: fewer files/lines touched means fewer conflicts with other changes
  • Easier to "back out" later if a problem is found once merged

Question: how small is small? Answer : one self-contained change

  • decompose a change into smaller changes, which can be done over time
  • address just one thing. Err on the side of being too small, not too big
  • the change should be self-contained, not have all sorts of external context. The code, title, description, should be enough to understand the change.
  • the change won't break master - don’t break the code/tests for other developers code you add should be used somehow.
  • Don’t check-in dead code.
  • 100 lines of code is usually reasonable, 1,000 is usually too large.
  • A 200 line change that touches 50 files might be too big, though
  • The "right size" for the developer might be too big for the reviewer, who has to suddenly understand everything you’ve done.
  • Split up a large change into separate changes, possibly using different reviewers for different parts (e.g., front end change, database change, configuration change, etc)
  • Fixing a bug shouldn’t happen in the same change where you refactor an API.
  • Keep tests for code in the same change as the code itself, so the reviewer can validate a change

3. The Standard Code Review

  • Code Review makes sure that the overall code health of a code base is improving over time.
  • Code Review must not be used as a weapon to delay development
  • Code Review should not allow changes to get merged that would decrease the overall health of the project
  • A code reviewer is responsible, with the developer, for the changes they add to the project
  • Google’s “senior” Rule for code review is: consider approving a change once it is definitely improving the overall code health, even if it isn’t perfect--you can iterate on it later. “There is no such thing as perfect code, only better code.”
  • Use “Nit:” to prefix comments in code reviews that something could be polished/improved, even if it won’t block the review.
  • Code review is a great place to mentor more junior developers and/or teach aspects of a language, framework, tool, or software development idea.
  • Code review should fall back on the project’s style, current approaches vs. personal preference.

4. How to Review Code and How to Handle Review Comments

  • Do the interactions in the various parts of the change make sense?

  • Does this change belong in this code at all?

  • Does this change integrate well with the rest of our code?

  • Is this the right time to make this change?

  • Does this change do what the developer claims and intends?

  • Is what the developer intends good for the project?

  • Does the change work? If tests aren’t passing, or the code doesn’t function, this isn’t ready for review

  • Are there edge cases that this change hasn’t considered?

  • Ask the developer to give you a demo of a change, make sure you can understand and witness what this change will do. This could be in person, via video, screenshots and animations, etc.

  • Is this change more complex than it needs to be? Could it be simplified?

    • Complex code is code that is hard to read and understand quickly.
    • Complex code is code that other developers could easily break, or introduce bugs without knowing

  • Is this change over-engineered? Is it too generic? Does it add functionality not currently needed? A change should solve the problem at hand, not the problem the developer wants.

  • Does this change have tests?

  • Are the tests useful in testing the change?

  • Are there test cases not considered that should be added?

  • Would the tests actually fail if the code was broken?

  • Tests are code too, and we don’t write tests for our tests, so they must be correct and useful.

  • Will the tests produce false positives?

  • Are the tests doing too many things? Can they be split up into separate assertions?

  • Does the change use good names for everything?

  • Does the change have code comments?

  • Are the comments necessary? Code comments should explain things that the code cannot

  • Has the change followed our style guide?

  • Does this change affect how developers will work? Does it change the workflow, introduce new tools, steps, techniques? If so, it needs to also update Documentation for the project

  • Read every line of code in the change, can you understand all of it? If you can’t understand a line, or it isn’t clear, leave a comment asking for clarification.

  • If you read the code, and understand what it does, but don’t feel qualified to judge it, ask another project member to review it with you (e.g., this one aspect of the change vs. whole thing).

  • Read the lines being changed in their appropriate context. Only a few lines in a function might get changed, but you should go back and read the whole function again. Does this change fit in the context?

  • If you see something nice in the change, tell the developer. Don’t only focus on mistakes. Offer encouragement, appreciation, motivation.

Clone this wiki locally