Skip to content

Instantly share code, notes, and snippets.

@shanezhiu

shanezhiu/kind-code-review.md

Forked from ross-spencer/kind-code-review.md
Created August 23, 2022 09:06
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save shanezhiu/358e3aa8d7a2c78c66b90ef48d5c04ee to your computer and use it in GitHub Desktop.
Save shanezhiu/358e3aa8d7a2c78c66b90ef48d5c04ee to your computer and use it in GitHub Desktop.
Download ZIP
Code Review (CR) Guidelines and Principles
Raw
kind-code-review.md 
Status: Work in progress
Use: Unofficial
Language: Python

Kind code review

Summary

Guiding principles for code-review.

Purpose

The purpose of a code review is to capture any potential errors in logic, or issues for future maintenance of the code and ensuring the legibility of the work of the developer.

Code review helps to bring an individual developer's style in-line with an organization's style-guidelines but can also help to improve style-guidelines.

Code reviews do not always take place with the author as the subject - the reviewer also has the opportunity to develop their knowledge of internal practices. Code-reviews can be great for new-starters.

Guiding Principles

  • Code reviews are about a conversation between developers.
  • Code reviews are not a substitute for testing.
  • Code reviews are about good style principles.
  • Code reviews do not require stepping through the entire technology stack.
  • Code reviews are not about perfection, but bringing code up 'a letter grade, or two' e.g. Grade C to Grade B.
  • Code reviews should be requested by the author to create a safe-space for R&D.
  • Code reviews are not about making code look or act like yours.
  • Code reviews are about finding an agreed baseline for a functional and readable shared code-base.

What to do before CR

  • Check code passes chosen linter tools, e.g. for Python: PEP8 principles using flake8 and pylint
  • Check lines are 79 characters or less
  • Check that comments are 72 characters or less.
  • Ensure that imports look sensible.
  • Ensure tests pass locally where possible

Ways to Code Review

The following thoughts do rely heavily on GitHub like CR features.

  • Comment inline

Inline comments help to identify and preserve context. They can be responded to by the developer which forms part of the conversation.

  • Give examples

An example use-case might be renaming a variable. If the name could become more descriptive, or useful for future maintenance, then don't leave the developer guessing. Provide an example and useful feedback.

 var 'k' might be more descriptive for the future reader as 'api_authentication_key'

  • Use inline code-blocks in review comments

    #.Use inline code blocks in review comments someObject = someClass()

This makes the context of comment clearer and can be more informative for the developer who might be learning new syntax/behavior in your suggestion.

  • Keep your style-book handy

What to look for should be nicely described in the coding principles, e.g. CONTRIBUTING.md

  • An idea alone shouldn't necessarily block code being merged

"I have started making sure there is always Unicode in my tests, but it isn't part of the contributing guidelines"

This could be seen as a curve ball. If it's a good idea consider encoding it in our principles at a higher-level. If it isn't part of a contributing guideline, then it should only be a suggestion that the developer can make their own decision on.

  • Don't ask developers to fix another problem "while they're there"

A developer might tackle this of their own volition, and follow appropriate internal process, but the reviewer should perhaps be even more laser-focused on the code being changed and the impact. See below: "working agile".

  • Create a safe-space for the author

A developer should request code-review, either for submission, or for discussion. Work-in-progress is work-in-progress. Use other frameworks for code-adjacent conversations, e.g. impact on release schedules etc.

  • There are no nitpicks

"Nitpick, verb INFORMAL: engage in fussy or pedantic fault-finding."

Using linters and style-checkers will pick up a lot of the style differences that might divide other developers. Suggested changes can then focus on bringing work up a letter-grade, catching pit-falls, or actual errors. A nitpick diminishes your suggestion as a reviewer. Consider a different phrasing or if it really is a nitpick, whether you can just live with someone else's approach.

  • Recognise effort

The larger the changes, the more effort a developer has taken to get to where they are. Maintaining the context of a large-technology stack in your head can be challenging, especially with split responsibilities. Mistakes will be made, and perfection might not always be reached. Try to recognize the effort taken so-far, and look at issues as innocent oversights, and improvements as ways for both of you, and the code-base to get to an even better place.

  • Be kind

Does your suggestion add something useful to what the programmer has already contributed? Is it constructive to them/the project? If not, leave the nitpick. Be comfortable with other's style.

Outputs of Code Review

  • Style changes
  • Logging considerations, e.g. what the developer requires vs. what the user requires
  • Code fixes where it is clear they may fail, and suggested tests to catch in future
  • Opportunities to refactor, i.e. don't perpetuate complexity/technical debt

Working Agile

  • Sometimes new bugs or issues will be found yet the PR satisfies the ticket, the code-reviewer may consider passing the code review and logging the new bugs or issues. These become part of the backlog and can be addressed specifically.

References

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment