Skip to content

Instantly share code, notes, and snippets.

@namuan

namuan/pull_request_checklist.md

Last active January 17, 2019 22:06
Show Gist options
  • Save namuan/92c9c09977c1f7aff0f0f106466dda9c to your computer and use it in GitHub Desktop.
Save namuan/92c9c09977c1f7aff0f0f106466dda9c to your computer and use it in GitHub Desktop.
Download ZIP
[Pull Request] #checklist
Raw
pull_request_checklist.md

Ref: https://github.com/felipefarias/pull-requests-checklist/blob/master/checklist-en.md

[ ] Pull request workflow

  • Read thoroughly the feature description to check if everything is implemented.
  • Run the code and use it as the end user would. Double check requested feature’s description.

[ ] Creating the pull request

  • Create Pull Request (but don't assign it yet).
  • Describe how to test the PR: urls, environment variables and other needs.
  • Refer to issue(s)/Trello card(s) the PR solves.
  • Refer back to the PR on Trello card(s).
  • Merge the target branch into the PR branch. Fix any conflicts that might appear.
  • Add screenshots of the new behavior, if applicable.
  • Add a description including the context and the chosen implementation strategy.
  • Explain code lines which the reviewer might not understand correctly:
  • Don't do it in the description, do it in the code itself as comments.
  • Consider refactoring and changing variable/function/method names to make it clearer.

[ ] PR Self Review:

Look for the following problems:
  • Code is not following code style guidelines (add links to your guidelines here when copying this checklist).
  • Bad naming: make sure you would understand your code if you read it a few months from now.
  • KISS: Keep it simple, Sweetie (not stupid!).
  • DRY: Don't Repeat Yourself.
  • YAGNI: You aren't gonna need it: check that you are not overcomplicating something for the sake of 'making it future-proof'.
  • PS: Fowler said "Yagni only applies to capabilities built into the software to support a presumptive feature, it does not apply to effort to make the software easier to modify".
  • Architecture errors: could there be a better separation of concerns or are there any leaky abstractions?
  • Code that is not readable: too many nested 'if's are a bad sign.
  • Performance issues (if that's a real concern for your application).
  • Complicated constructions that need refactoring or comments: code should almost always be self-explanatory.
  • Forgotten TODOs and noop lines: make sure they're mapped in your Trello/Issues board.
  • Grammar errors.
  • Continuous Integration errors, including tests and coverage (configure CI to send you an e-mail when tests are done).
  • If the feature needs regression tests, write them.
  • Other possible improvements (add to this Checklist if it's a general concept - it's open source!).
  • Once all problems are addressed, allocate the reviewer.

[ ] Responding to feedback

  • If any problem hasn’t been addressed and the PR needs to be accepted ASAP, create new issues for remaining problems.
  • Respond all of the reviewer's comments ASAP:
  • Be grateful for the reviewer's suggestions. ("Good call. I'll make that change.").
  • Don't take it personally. The review is of the code, not yourself.
  • Try to understand the reviewer's perspective.
  • Once you receive feedback and address all issues, merge and close the PR/branch.

Code Review - Ref: https://medium.com/@mrjoelkemp/giving-better-code-reviews-16109e0fdd36

Phase 1: The Light Pass

General

  • Code formatting has been used
  • Line indentation char and line breaks
  • Only required files are changed
  • Disagreement between code and specification
  • Comments in source code are included
  • Documentation of the implementation is created or updated
  • Does not contain any unimplemented areas like //TODO or //FIXME
  • Optimistic or undefensive programming

Unclear Or Messy

  • Verify correct and meaningful naming
  • Magic numbers and values
  • Variables used for more than one purpose
  • Minimized variable, method and class scopes
  • Method signature
  • Packing too much into one line
  • Complex IF conditions, IF instead of Switch
  • Cyclomatic complexity

Error Handling

  • Use exceptions only for unexpected errors
  • Avoid empty catch blogs
  • Error handler over-catches exceptions and aborts current flow or application
  • Error handler is not implemented e.g. contains TODO, FIXME

Java

  • Review class imports
  • Wrong use of == and equals()
  • Wrong use of collection data types, like List instead of Set
  • Object contract errors (e.g. equals and hashCode)
  • Exposure to immutable data types

Git Commit Message

  • Describes what and why it has changed
  • Verify correct format

Phase 2: The Contextual Pass

Functional programming

  • Prefer immutability
  • Minimize side-effects and perform them in a central place
  • Do not rely on global state
  • Plan for composing functions
  • Keep method signatures as simple as possible
  • Write generic functions
  • Avoid type specific functions

Code Structure

  • Understandability of written changes
  • No logical errors
  • Max usage of static type compiler checking
  • Does not violate architecture guidelines, design principles or implementation patterns
  • Are there any alternative implementations that increase simplicity, readability or maintainability
  • Check edge cases in functions
  • Any better approach to use a framework, library or class exists?
  • Look for omissions: Shouldn't this component also do X?
  • Enough log statements to find bugs quickly

External Systems

  • Reduce amount of calls / Optimize calls to external systems

Tests

  • Unit-tests and End 2 End Tests are added and test all functionality
  • Test coverage of changed lines and critical path

Related: https://github.com/mestachs/experiment/blob/master/codereview/checklist.md

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