Pull Request Review Guidelines

Reviewing other people's code is one of the most valuable contributions you can make to Bahmni (or to any open source code). You do not have to be a senior developer, or a core team member, to make a meaningful code review contribution, and doing a review does not necessarily mean you need to be the one to merge the code. Doing a partial review can still allow the contributor to iterate and improve the PR before a final review and merge happens.

Please see the Submitting Pull Request page for PR guidelines, which will help you to guide the requester

Before you (reviewer) start

1. Is there a JIRA card created with description of the feature/issue and acceptance criteria articulated in the card? 
2. Is the PR marked with the right JIRA card number? When you merge, you must ensure that the JIRA card number is reflected in the commit msg.
3. Are you the right person to review this PR? If not, try to direct to the right person. If you have partial context, try to find help from one who knows better.
4. Are any checks (travis-ci, codeclimate) failing? If so, immediately request that the contributor look into this.
5. Has the submitter signed the CLA? (See the status of the cla-assistant check.) If not, kindly ask them to do so.
6. Ask questions, instead of making statements. Avoid “why” - which often seem accusatory, rather try to understand the context and reasoning … “what was the reason for ….”
7. For bigger PRs, try to “meet” with the person. Often pairing on the PR review for big PRs is the best way. Bigger PRs are also best to merge onto a branch, run/check locally and then commit.
1. You can follow github ‘Command line instructions’ suggestions to merge a PR locally before pushing onto master. Often for large PRs and in cases you can’t assert the correctness of the code without running it locally, this is a preferred approach. Just go the PR link in github; towards the end click “command line instructions” next to the “merge pull request” button and follow the suggestions.  
8. Lots of handy tools exists for code analysis - e.g. a Static Code analyzer with IntelliJ or Sublime

Guidelines on code

1. Coding styles are important but should not be overbearing.
2. The code must not break any existing feature during upgrades
1. Data migrations if needed must be done so as to persist and keep working with existing transactional data.
2. Configuration if introduced must have the previous behavior as the default, with options of extending/changing the behavior. Meaning, using existing configuration with new code does not require updates by any implementation. Configurations are documented and reflected with the release version indicated.
3. Feature behaves, as much as possible, like before and is easy to pick up and preferably does not require retraining of end users.
3. Unused code
   1. Don’t carry dead code, including commented off code is not useful and just baggage and often confusing. We use git anyways and all editors show a pretty good diff between versions!
   2. Duplicate code - maybe create another card?
4. Encourage using Java 8 paradigms
1. Use of closeable in try-with-resources
2. Using streams
3. Loops like “names.forEach”
4. Lambda expressions
5. DRY - don’t repeat yourself please.
6. Loops have definite set length and correct termination conditions.
7. Is the code performant? Has it been tested with enough representative data? (Ask the contributor to share what they've done, or test it yourself if you have concerns.)
8. ROT code? - surely someone has come up against it? Use a known library if you can’t find already plugged in.  
9. SOLID principle

Essential Reviewer Guidelines

1. Code reviews are a discussion not ‘dictation’/“orders”.
2. The reviewer is equally responsible to the contributor - for correctness, design, security, readability and maintainability. Do not merge, and feel free to push back if you (reviewer) are not satisfied.
3. It's fine if the contributor disagrees with your opinion, as long as he/she provides explanation that is logical.
4. Direct the contributor to relevant code sections as examples to follow.
5. Are there multiple commits that make it difficult to review or test locally? You may ask the contributor to squash into a single one, but remember that GitHub lets you "squash and merge", so you don't normally need to request this.
6. Ensure there are corresponding tests for the PR relevant code
1. Unit tests and Integration tests (if applicable) should be present.
7. Does the code change something in the product that needs documentation (new or updates) in WIKI? - If so:
   1. mark the JIRA card accordingly by adding a "needs-doc-update" label
   2. ask the contributor to add the documentation (and verify that they include appropriate information like ‘applicable from version X’)
8. Dependencies:
1. Either to a library - ensure that the library is a well known and supported/latest one, is appropriate for the job, and there is no duplication (e.g. No Joda datetime library if it can be solved by Java 8 date APIs).
2. To another module: Is the other module released? Discourage snapshot release dependency. In exceptional cases such snapshot dependency maybe allowed but ensure there is a JIRA card existence to resolve such dependency.
3. Obvious: no circular dependency of components/services/codes
9. If you (reviewer) discover a missing “acceptance criteria”, please update the JIRA card.

What to Review?

See JIRA for cards that need code review in the next release: