-
Notifications
You must be signed in to change notification settings - Fork 0
General Engineering Guide
The title should be 80 characters or less, and be an extremely simple one-line summary of your change. Include a the task ID, if applicable.
Examples of good titles:
ISSUE-123: Fix failing tests
[ISSUE-123][ISSUE-456] : Fixes failing tests and implements linting
Examples of bad titles:
Any title that doesn't give a sense of what is being addressed.
The description should provide a detailed account of the issue the commit is trying to resolve, details on how it solves the issue, and any related details.
Choose a primary reviewer for your diff, and also set them as the Assignee so they can easily see their avatar in the pull request list. Additional reviewers may be added at your discretion, but try to emphasize whose job it is to give the final word.
Ensure that your code’s style adheres to the appropriate style guides. Since we are primarily working with JavaScript, ensure that you follow the recommended engineering guides for JavaScript
Why?
We need to ensure the codebase has consistent code that is easy to read, understand and maintain.
Some aspects to note while working
-
Make sure your statements end with semicolons.
let name = 'James'; -
Make sure there is an order of precedence to your imports.
-
Avoid long lines in your code, having code on one line does not ensure a performance boost of your algorithms.
-
Ensure proper indentation of your work and this can be done by adding recommended formatting plugins to your IDEs.
-
Maintain es6 standards as we work, instead of using traditional functions, make use of pointers
const sum = (num1, num2 ) => { ... } -
Avoid using var or plain unscoped variables and have a high preference for using let and const.
-
When handling comparisons, high preference for using === to ==, !== to !=.
-
Ensure that your variables, function names, classes are meaningful! Avoid ambiguous naming such as
const myX = (x, y) => x+y;instead useconst sum = (x, y) => x+y;etc.
When possible, posted PRs should be concise and single purpose. Save for refactors that affect the entire codebase, aim for your changes to be 200 lines or less. If you find your change growing larger and larger, search for ways to incrementally build and commit the feature. This can be accomplished by creating small, independent changes that are hidden behind a feature flag.
The benefits of small commits include:
- More opportunities for design evaluation
- The ability for reviewers to provide finely tuned feedback
- Easier ability to refactor early, instead of forcing a major rewrite after weeks of work
- Granular rollbacks, should an issue be found.
- Faster feedback loops: Reviewers are more likely to provide several smaller reviews throughout the day, whereas a large review may only get a single look.
If your commit shows clear lines of separation that could have been broken into smaller commits, it is not unreasonable for a reviewer to request changes and expect you to break the commit into smaller chunks. This is a huge pain in the ass, so try not to let your change get to that point.
Always run unit tests on the app module before you merge your pull request. Ideally, you would also create new unit tests to test the changes related to your PR. This step is easy to forget, so do your due diligence and at the very least make sure your code changes don't break unit tests.
This assumes that you have the branch cloned locally.
- Every branch will be based on the develop branch. Hence checkout to the develop, then create your branch. If you are doing this locally:
git checkout develop,
git checkout -b ben/123-fix-test-failing-builds
- Create a feature branch using your name as a prefix. This makes it easier to clean up later when our remote repositories are cluttered with leftovers:
git checkout -b ben/issue-123-fix-failing-builds
- Add your code and commit like normal, don’t worry about the number of commits, they’ll get squashed during merge
git add .
git commit -m "commit message"
- Push your branch to origin
git push origin ben/issue-123-fix-failing-builds
- If you need to rebase from develop:
git checkout develop
git pull
git checkout ben/issue-123-fix-failing-builds
git rebase master
Resolve conflicts
git push -f
- Follow the steps outlined in “Submitting a Pull Request”, taking note that you are working on a branch of the main repository instead of a branch of a fork.
- Commits on branches should be squashed to a single commit before merging.
- Make sure that the final commit message includes the Issue ticket and a meaningful description, do not use the default Squash & Merge message, which is generally worthless.
- The requester of the PR should do the merging once approved. 10.Usually, this is a good time to delete your branch from the origin. If not, be sure to delete your branch later.
Your test plan should explain the exact steps followed to prove your commit does what it says it does, and does not adversely affect any existing features. Steps should be accompanied with a link and a screenshot whenever applicable.
Always provide cases to prove no regressions. For example, if the feature you’re committing is behind a feature flag, your test plan should include steps with the feature flag off to confirm that the app functions normally when the feature is disabled.
Test plans should not assume prior knowledge of the issue being addressed. They should include explicitly or link to details needed to test the fix or features. If particular campaigns or accounts are would facilitate the test, these items should be included in the plan.
Before writing your code, consider talking out the feature structure and design decisions up front before work even begins. Ensure that you have buy-in for your approach before spending too much time on your feature.
Before adding reviewers, look over the diff or pull request you’ve submitted to find any issues before they are put in front of other engineers. Fix any latent style issues, remove any commented code, and fix any bugs. Evaluate the approach you’ve taken and consider if there are better approaches to take. Look at the review as if you were reviewing it. Are there aspects of the commit that you would ask someone else to change, if it were not your own code?
As reviewers comment on your code, try to respond to all inquiries. If you add a revision that resolves a previous code reviewer’s request, ask them to re-review your changes and get their approval. If you decide that a comment is not worth addressing, is not correct, or can be done in a subsequent commit, make sure you respond with those details. Do not make it appear as if you’ve ignored any comments.
Read the entire diff before you do anything. Soak in the summary and test plan, making sure that you understand its purpose before beginning to leave comments.
At a high level, do you agree that the author used the best architectural approach?
Is the author building a bridge to support 500-ton trains when it only needs to support bikes? Overly complex features are bug-prone and should be avoided. Sometimes complexity is necessary, but be sure to evaluate that the level of complexity found in the commit coincides with the requirements of the feature.
On a component and function level, is the code written as clearly and concisely as possible? Look for opportunities to reduce nesting, clarify functions, and reduce complexity wherever possible. Also be on the lookout for consistency, as inconsistent naming, style, or formatting often translates to unnecessarily increased complexity.
This should be looked at from 2 angles: Known reusability and potential reusability.
Known
Reuse often comes after a pattern of repeating a function or component. However, it’s important not to “reuse” too early. A good rule of thumb to follow is this:
The first use of the component: Write once The second use of the component: Copy The third and all subsequent uses: Refactor to a shared component
All future potential uses of a component are often not known, and attempting to create a generic component or utility too early often results in the need to refactor later anyway. In this case though, you’ve already created a component that’s potentially in use by other consumers or developers, and refactoring at this point may be more work than if you had simply avoided early reuse.
Potential
There are instances where simple code will obviously have future potential for reuse, such as format utility functions.
A non-exhaustive list of reasons that you can use to mark a PR as "Request Changes":
- The code doesn't fix the issue at hand.
- The code contains bugs and/or regressions.
- The code fails to account for all possible inputs.
- The diff is too large and combines multiple independent fixes that should not be paired into the same PR.
- Code changes do not follow our guidelines.
- The CI build fails to complete.
- Unit tests are now broken.
- Code should include new unit tests but none were added.
There are situations where the code can be merged without issue, but it’s suggested that the author perform either a revision or a follow up commit.
Some examples of suggested revisions:
- Isolated areas of code or logic could be written more clearly and/or concisely.
- Code is breaking minor stylistic rules, and the author has not exhibited a pattern of being a repeat offender.
- Minor layout or style issues
It’s never a bad idea to check a commit out and test it locally. However, this is not always necessary. For smaller or very straightforward changes, a solid test plan provided by the author should imbue enough confidence to approve a commit without additional pre-merge manual testing.
When testing, minimally test the “happy path”, but do your best to try and break the commit. Test edge cases, zero/null entries, potential out-of-range issues, etc.
When issues are found, use the severity at your discretion to either request a revision to the commit or expect a subsequent diff.
Some major points to keep in mind when performing a code review:
These are people you need to continue working with. So don’t say things like “this sucks,” “you suck”, or any other related negative or derogatory comments. Be helpful, humble, and respectful!
This is not your code or their code, it’s the product's code. Don’t take things personally. If someone is refactoring your code, don’t take it as an affront to your abilities as a software engineer. Instead, work together to attain the result you both strive for: High quality and reliable software that’s a delight to work with for both the engineer and the user.
Once your code has been reviewed and all automated checks have passed, Github pull-requests have several options for merging. To keep the history of the master branch linear, we have chosen to always Squash & Merge branches to a single commit when merging.
A good commit message should include the following details: Title:
A short one-line summary including the Issue ticket number and the least-technical meaningful description of what was changed. For example, "ISSUE-123: Fix failing builds".
Body:
- A full Link to the Issue ticket describing this work
- An overview description of the motivation for the work — the why not the how.
- Acceptance criteria for the fix
- (Optional) Test Plan
- (Optional) Technical details of the implementation
- (Optional) Follow-ups, if any
When commit messages are formatted in this manner, several advantages are realized:
Release notes can be created from something as simple as git log release-x.a.b..release-x.c.d --pretty=oneline.
Engineers working on the app in the future (remember, it could be you!) have a reasonable chance at understanding not only the what of your code but the why.
We all feel good about ourselves because we have been relieved of the need to differentiate our commit message style from our peers, and instead choose to merge our tiny individual selves into something much greater.
- Strive to submit diffs that can be merged on the first review. Try to make all revisions before the code review.
- Small diffs are always preferred.
- Don’t hesitate to reject a diff, even if it’s to clarify some points before you feel it’s safe to merge.
- Don’t optimize prematurely or account for situations that probably will never happen in your code.
- Build for the appropriate level of complexity.
- Don’t be an arse.