Ref: [ ] 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: 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: