Tips for an Effective SAP Commerce Cloud Code Review
In this article, we cover how to design an effective code review process. You will learn what the code review process looks like, as well as the tools and resources available to help ensure that you are merging higher quality code.
Table of Contents
Effective Code Reviews
For an effective code review process, apply the following principles and ensure they are understood by the whole team:
- Take the time to actually review each pull request. Yes, big pull requests are painful to review, but they are also the ones that need it the most.
- Consider the code review as a service. Do not be shy when reviewing code from developers who are more senior than you. Senior developers know the importance of feedback and want you to review their code.
- Ask questions instead of making statements. The author has likely put more thought into the code than you are aware of, and has already solved the problem once. Asking questions is powerful. They benefit both the reviewer and the author, and are easier to accept over criticism.
- Ensure that there is only one feature in the pull request.
- Consider more than just the code. In addition, look for design problems:
- Does this change solve a real problem? Is it worth solving?
- Given the constraints, is the overall approach correct and reasonable?
Mentally separate back-end and front-end code, and review both. Even though it is easier to only review the part you are more familiar with, use this exercise as a learning opportunity.
Reject pull requests that do not have any test(s) that will validate the feature or defect.
Discuss with the author (nicely) if the test coverage is sparse.
The tests should be readable and make sense.
- Look for code smells, and suggest refactoring if needed (include tests also).
- Ask for help with reviewing code that is outside of your area of expertise.
- Be encouraging. If you see something that was done well, say so!
- Ensure they follow the recommended practices for SAP Commerce Cloud and code standards.
Reduce the List of Things to Check Manually
You can use continuous integration and code quality tools to enforce parts of your Definition of Done without requesting more attention from the code reviewers:
- Require a successful build prior to merging a pull request. This ensures that the build on your target branch will not break due to code.
- Ensure that the platform starts.
- Confirm there are no changes that break the initialization in the target branch. If an initialization fails, it should halt your continuous integration pipeline.
- Automatically enforce a strict zero test-failure policy in the target branch. Execute your tests, and upon any test failure, fail the build.
- Enforce code coverage and code quality by using SonarQube. For example, the absence of common issues such as null pointers.
Code reviewers will have more time to perform value-added checks by automating the above. However, there is a trade-off. It may take more time to deliver the user stories. The solution is to adjust the merge prerequisites (in your Definition of Done) and the level of quality that was agreed upon with the customer at the beginning of the project.
Compliance and security standards, like PCI-DSS, require that all code is reviewed prior to being deployed to production, and that the results are tracked. Code reviews are also a great way to improve the code quality, catch defects and make sure that the whole team is in sync with the latest changes.
The recommended practice when using a distributed version control system (DVCS), such as Git, is to:
- Add the code review step. This should be done before the code is even merged into the shared development branch.
- Only accept code into the development branch that passed the code review step. This allows you to review one feature at a time, and avoids blocking a release because of issues detected during a code review.
The general workflow is centered around the idea of a Pull Request. This is the preferred method for submitting contributions with a DVCS. Pull requests are offered by all major DVCS services in the cloud (Github, BitBucket) and hosted servers (GitBucket, Stash). A similar flow is achievable with a more traditional centralized version control system.
The code review process should not be used for spike solutions or Proof of Concepts (POC). Spikes and POC's contain temporary code that must be rewritten before it will be part of the code review process.
Create a Feature Branch
For SAP Commerce Cloud projects, it is recommended to use the feature branch development workflow (see Gitflow Workflow). This provides the flexibility for developers to work on multiple isolated features in parallel, while still being able to support and test production hot-fixes.
To create a feature branch run:
This will check-out your develop branch, where all features will eventually be merged. It will also pull the latest changes.
Next you will need to create a feature branch:
Ideally, the name of the branch will reflect the identifier of the user story/ticket number. This way, when you have multiple feature branches, you can easily associate it to something recognizable.
If you already have an existing feature branch, you can just run:
Once on your feature branch, you can make the code changes you need. In order to prevent future rework, you should ensure you are coding to the same standards that will be applied as part of the code review.
Create Pull Request
Once your code is ready for review, you will need to create a pull request that can be merged into your develop branch. While on your feature branch, pull in the latest changes from your develop branch.
If you are working on a bug-fix or hot-fix, you may need to pull from the master or support lines instead of develop.
If you run into conflicts, resolve them on a case-by-case basis. Once all conflicts are resolved, execute a commit using the command below. Ensure your comments on the commit are detailed enough so that someone else can understand what you did.
You can now push your changes upstream:
The --set-upstream flag is optional, and only useful the first time you push. This avoids having to explicitly tell Git which branch you want your local feature branch to be synchronized to.
Provide comments in the JIRA ticket about what you have done. Additionally, add any instructions for the deployment to production, as well as any notes for the testers. Depending on team policies, you should document:
What you have already tested.
The impacts of the change.
How to deploy the change if there are manual steps to perform beyond the automated deployment procedure.
Create the pull request as described in Creating A Pull Request in Bitbucket (or any other tool), and add the necessary approvers.
In the case of rework, or if the previous pull request had not been merged, do not create a new pull request. It is enough to push the branch to the origin for the pull request to be updated.
Check Continuous Integration Results
To ensure a repeatable and consistent process, a continuous integration/continuous delivery (CI/CD) pipeline needs to be in place. You should have your CI/CD process set up such that creating a pull request will trigger a build immediately (if you use a webhook) or after a few minutes (if you use polling).
Your CI/CD pipeline should do all of the following against the feature branch:
- Build the code.
- Run sonar quality check.
- Execute automated tests (unit, integration and, if available, front-end test suites)
If a test fails, or the code quality analysis results in one or more "blocker" issues, the system should mark the pull request as failed for all to see (approvers, developer). Some solutions require even more stringent standards by marking any "critical" code quality issues as a fail. Once the code has passed the CI/CD pipeline, the developer (or CI/CD hook) can set the status of the ticket to "In Review".
Perform Code Review
After checking the continuous integration results, and before changing the status of the ticket to "To be Reviewed", some teams find it beneficial to perform an informal demo to both a code reviewer and a tester. This extra step (on the developer's local environment) is a more efficient way of initiating the feedback loop. The tester learns how the story works, and the code reviewer can verify that the basic acceptance criteria are met.
- The pull request lists all commits that will be merged, and their authors.
- Check the pull request build status:
- Test results (including automation if available).
- Quality violations in Sonar.
- Verify each commit or each modified file in the pull request:
- Refer to the article on
- If the ticket has been reworked, check if all comments have been answered in the pull request and the JIRA ticket.
- If a rework is necessary:
- Add your comments in the pull request.
- Update the ticket status.
For tickets not requiring code changes, simply change ticket status to “To be tested”.
Approve Pull Request
Approving a pull request does not automatically merge it. A pull request can be approved by multiple approvers. Only developers with write-access to the repository can approve pull requests (they do not need write-access to the target branch of the pull request).
A per-repository policy can be set up to require a specific number of approvals for a pull request to be merged.
Merge Pull Request
Once the pull request has been approved by all approvers, it will need to be merged into either the develop, support, or master branch by the owner of that line. It is recommended to squash all commits as part of the merge, and to select the “delete source branch after merging” option. Doing this reduces the number of branches to only those that are being worked on.
Once the pull request has been merged, the person performing the merge should change the status of the ticket to “To be Deployed”. It should also be included in the next build to your development environment.
Code reviews are an important step in the development process. They help:
- Drive consistency.
- Approvers (one or many) identify code issues a developer may have overlooked.
- Ensure the code achieves the "Definition of Done" in the ticket.
You can catch potential issues in the code early by implementing a code review process, and by creating barriers (for your develop, support and master lines) that ensure only code that has gone through rigorous checks are eventually deployed to production.