cjohnson496 · January 17, 2019 22:06 · Aug 30, 2018 · Aug 30, 2018 · Aug 30, 2018 · Aug 30, 2018
diff --git a/pull_request_checklist.md b/pull_request_checklist.md
@@ -118,4 +118,7 @@ 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>
diff --git a/pull_request_checklist.md b/pull_request_checklist.md
@@ -43,4 +43,79 @@ Ref: <https://github.com/felipefarias/pull-requests-checklist/blob/master/checkl
   * 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.
+  * [ ] 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
+
+
diff --git a/pull_request_checklist.md b/pull_request_checklist.md
@@ -1,7 +1,7 @@
 Ref: <https://github.com/felipefarias/pull-requests-checklist/blob/master/checklist-en.md>
 
 [ ] Pull request workflow
-  * [x] Read thoroughly the feature description to check if everything is implemented.
+  * [ ] 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

diff --git a/pull_request_checklist.md b/pull_request_checklist.md
@@ -1,7 +1,7 @@
 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.
+  * [x] 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

diff --git a/pull_request_checklist.md b/pull_request_checklist.md
@@ -0,0 +1,46 @@
+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.
No results found