19 Things To Consider Before You Create a Pull Request
Having your code analyzed can feel invasive and uncomfortable. Even worse, reviewing other people’s code can feel like a painful and ambiguous exercise where you’re searching for problems and not even knowing where to begin.
Code reviews are a necessary part of the software engineering process because you alone can’t catch every problem in a piece of code you write. That’s OK, though. Even the best basketball players in the world miss shots.
Having others review our work ensures that we deliver the best product to users with the least number of errors.
This article aims to provide some solid points that you and your team can follow to be productive.
Things To Avoid
1. Basic formatter and linter
Avoid discussing details that can be handled by a static analysis tool. Don’t argue about nuances, such as code formatting and whether to use let or var. Having a formatter and linter can save your team a lot of time by having your computer do reviews for you.
Summary: Code reviews should be as automated as possible.
2. Basic discussion or confusion
These discussions should happen before the code is even written. Don’t try to argue about the floor plan once you’ve poured the concrete foundation.
Summary: Code reviews should avoid API discussion.
3. Unclear commit message
Commit messages should be clear and should accurately describe the new code. Avoid writing messages like the following.
Changed some crap
damn it
one more to fix this stupid bugThese are funny and satisfying but not helpful when you’re up on a Saturday morning because you pushed code on a Friday night and can’t figure out what the bad code was doing when you git blamed the commit. Write commit messages that describe the code accurately, and include a ticket number from your issue tracking system if you have one. That will make searching through your commit log much easier.
4. Avoiding to-do comments in PR
To-do comments are great for letting you and your fellow engineers know that something needs to be fixed later. They indicate something is missing and should not make it feel that a task complete.
Things To Follow
5. Typos should be corrected
Avoid nitpicking as much as you can, and save that for your linter, compiler, and formatter. When you can’t, such as in the case of typos, leave a kind comment suggesting a fix. It’s the little things that make a big difference sometimes!
6. Variable and function names should be clear
Naming is one of the hardest problems in computer science. We’ve all given names to variables, functions, and files that are confusing. Help your teammate out by suggesting a clearer name if the one you’re reading doesn’t make sense.
// This function could be better named as namesToUpperCase
function u(names) {
// ...
}7. Functions should be short
Functions should do one thing! Long functions usually mean that they are doing too much. Tell your teammate to split out the function into multiple different ones.
// This is both emailing clients and deciding which are active. Should be
// 2 different functions.
function emailClients(clients) {
clients.forEach(client => {
const clientRecord = database.lookup(client);
if (clientRecord.isActive()) {
email(client);
}
});
}8. Files should be cohesive, and ideally short
Just like functions, a file should be about one thing. A file represents a module and a module should do one thing for your codebase.
For example, if your module is called fake-name-generator, it should just be responsible for creating fake names, like “Keyser Söze.” If the fake-name-generator also includes a bunch of utility functions for querying a database of names, that should be in a separate module.
There’s no rule for how long a file should be, but if it’s long, like the one below, and includes functions that don’t relate to one another, then it should probably be split apart.
// Line 1
import _ from 'lodash';
function generateFakeNames() {
// ..
}// Line 1128
function queryRemoteDatabase() {
// ...
}
9. Complex code should be commented
If you have named things well and the logic is still confusing, then it’s time for a comment.
function leftPad(str, len, ch) {
str = str + '';
len = len - str.length;while (true) {
// This needs a comment, why a bitwise and here?
if (len & 1) pad += ch;
// This needs a comment, why a bit shift here?
len >>= 1;
if (len) ch += ch;
else break;
}return pad + str;
}
Consider Side Effects
10. Functions should be as pure as possible
// Global variable is referenced by the following function.
// If we had another function that used this name, now it'd be an array and it
// could break it. Instead it's better to pass in a name parameter
let name = 'Ryan McDermott';function splitIntoFirstAndLastName() {
name = name.split(' ');
}splitIntoFirstAndLastName();
11. I/O functions should have failure cases handled
Any function that does I/O should handle when something goes wrong.
function getIngredientsFromFile() {
const onFulfilled = buffer => {
let lines = buffer.split('\n');
return lines.forEach(line => <Ingredient ingredient={line} />);
};// What about when this rejected because of an error? What do we return?
return readFile('./ingredients.txt').then(onFulfilled);
}
Consider Edge Cases
12. Null cases should be handled
If you have a list component, for example, all is well and good if you display a nice beautiful table that shows all its data. Your users love it, and you get a promotion! But what happens when no data comes back? What do you show in the null case? Your code should be resilient to every case that can occur. If there’s something bad that can happen in your code, eventually it will happen.
13. Large cases should be handled
In the list above, what would happen if 10,000 items came back from the inventory? In that case, you need some form of pagination or infinite scroll. Be sure to always assess the potential edge cases in terms of volume, especially when it comes to UI programming.
14. User input should be limited
Users can potentially input an unlimited amount of data to send to you. It’s important to set limits if a function takes in any kind of user data.
router.route('/message').post((req, res) => {
const message = req.body.content;// What happens if the message is many megabytes of data? Do we want to store
// that in the database? We should set limits on the size.
db.save(message);
});
15. Functions should handle unexpected user input
Users will always surprise you with the data they give you. Don’t expect that you will always get the right type of data, or even any data, in a request from a user. And don’t rely on client-side validation alone.
router.route('/transfer-money').post((req, res) => {
const amount = req.body.amount;
const from = user.id;
const to = req.body.to;// What happens if we got a string instead of a number as our amount? This
// function would fail
transferMoney(from, to, amount);
});
Security
16. XSS should not be possible
Cross-site scripting (XSS) is one of the largest vectors for security attacks on a web application. It occurs when you take user data and include it in your page without first properly sanitizing it. This can cause your site to execute source code from remote pages.
17. Personally identifiable information (PII) should not leak
You bear an enormous weight of responsibility every time you take in user data. If you leak data in URLs, in analytics tracking to third parties or even expose data to employees that shouldn’t have access, you greatly hurt your users and your business. Be careful with other people’s lives! It's serious.
Lastly, Testing
18. New code should be tested
All new code should include a test, whether it fixes a bug or is a new feature. If it’s a bug fix, it should have a test proving that the bug is fixed. And if it’s a new feature, then every component should be unit tested and there should be an integration test ensuring that the feature works with the rest of the system.
19. Tests should actually test all of what the function does
Tests should cover everything that a function does, including the edge cases and the limits that it operates on.
