Git and Code Review

Git

Every new feature, bug fix or enhancement for SimMobility must be implemented in its own git branch. It is important for all developers to understand how the Git works.

Branch Naming: all branches (without exception) must follow the following naming convention.

<long/mid/short/shared>-<fix/ftr/enh>-<branch description>

• fix: Bug fix
• ftr: New feature
• enh: Enhancement
• Examples:
  • shared-ftr-messagebus
  • long-fix-marketmodel
  • mid-enh-ptroutechoice

If the purpose of the branch is not any of fix/ftr/enh, kindly discuss with the team and update this page.

Commits:

• Commit short. Commit frequently. Avoid large commits with many files.
• Each commit should have one purpose. All changes in a commit should be semantically related. Avoid unrelated changes in the same commit.
• Give meaningful commit messages. A commit message must roughly try to answer the question "what is in this commit?" Example: "fix for issue with some specific component" or "new file filename added for whatever reason" or "code refactoring for some crazy time consuming function".
• If the commit fixes an issue reported on Github, the issue number must be mentioned in the commit message.
• As a guideline, limit the length of the commit message to a maximum of 70 chars if possible.
• If the message is long, break it into multiple lines and capture the gist of the commit in the first line.

Code Review

How does it work? All developers in the team will have to review other team members' pull requests. Sometimes, code review can be an opportunity for the reviewer to learn cool programming tricks from the code written by a fellow developer. During review, the reviewer must keep in mind that he is partly accountable if something goes wrong in the code in a pull request accepted by him.

But my code is perfect! Nobody's code is perfect. The code review must guarantee that the code follows our code style and is well documented. A reviewer shall not give permission to merge a pull request if it does not even follow the minimal requirements of code style and documentation. Any suggestions on code refactoring and any visible bugs that escaped unit-testing must also be reported during code review.

The spotlight is on quality. The developer of a branch needs to understand that the objective of the reviewer is not to criticize the developer, but to improve quality of our code base. Any criticism needs to be taken constructively. The reviewers on the other hand must be polite in suggesting changes and pointing out mistakes.

Code review process

When a feature has been developed and tested in a development branch, it has to be merged with the master branch on git. The code review process includes the following steps.

1. the developer of the feature creates pull request on github. Pull request is a request to the team to pull the new feature branch into the master branch.
2. the developer assigns a reviewer for the pull request. The reviewer is peer software engineer, preferably one who is more senior or experienced than the developer who programmed the new feature. It is even better if the chosen reviewer is fairly familiar with the new feature.
3. the assigned reviewer then goes through the changes/commits in the feature branch and reports his comments inline on github. The code reviewer must look for compliance to code style, presence of documentation comments, ways to improve the implemented code and report all findings to the developer. The reviewer must also make sure that the new features in the branch are documented adequately in the wiki.
4. the developer then addresses each review comment, making pertinent changes if he accepts the comment or provide proper reasoning to the reviewer if he wants to defend his implementation against the comment.
5. after a few iterations of such reviewer-developer interaction, the reviewer gives a 👍 to the developer to merge his feature branch into master when he is satisfied with the code changes.
6. the developer merges his feature into master, deletes his feature branch right away and notifies the team that such a feature is in master now.

Merging Process

Before creating the pull request, the developer is responsible to

1. Test the development branch well
2. Checkout master
3. Pull master
4. Checkout the development branch
5. Merge master into this branch
6. Resolve conflicts (if any)
7. Compile the code
8. Test the code again
9. Commit & push conflict resolutions

After the reviewer's approval, the developer is required to

1. Merge pull request into master
2. Delete the development branch right away from github.
3. Notify the team about the feature merged into master

Code Style

Every new developer who joins the SimMobility team must get familiar with our code style.
Whenever possible, keep the code simple and readable. Code without documentation comments shall not be merged. You do not need to comment all fields or constructors or simple getters and setters, but when the code is not self explanatory you need to add extra comments to help other programmers understand your code.

Example:

void Conflux::updateAgent(Person_MT* person)
{
	if (person->getLastUpdatedFrame() < currFrame.frame())
	{	
        //if the person is being moved for the first time in this tick, 
        //reset person's remaining time to full tick size
		person->remainingTimeThisTick = tickTimeInS;
	}

	//let the person know which worker is (indirectly) managing him
	person->currWorkerProvider = currWorkerProvider;

	//capture person info before update
	PersonProps beforeUpdate(person, this);

	//let the person move
	UpdateStatus res = movePerson(currFrame, person);

	//kill person if he's DONE
	if (res.status == UpdateStatus::RS_DONE)
	{
		killAgent(person, beforeUpdate);
		return;
	}

	//capture person info after update
	PersonProps afterUpdate(person, this);

	//perform house keeping
	housekeep(beforeUpdate, afterUpdate, person);

	//update person's handler registration with MessageBus, if required
	updateAgentContext(beforeUpdate, afterUpdate, person);
}