1. Introduction
2. Why Review Code?
3. Basics
4. Readability
5. Side Effects
6. User Input
7. Security
8. Performance
9. Testing
10. Miscellaneous

Code reviews can inspire dread in both reviewer and reviewee. Having your code analyzed can feel like getting screened by the TSA as you go off to a beautiful vacation. Even worse, reviewing other people's code can feel like a painful and ambiguous exercise, searching for problems and not even knowing where to begin.

This project aims to provide some solid tips for how to review the code you and your team write. All examples are written in JavaScript, but the advice should be applicable to any project of any language.This is by no means an exhaustive list but hopefully this will help you catch as many bugs as possible.

Code reviews are a necessary part of the software engineering process because you alone as a developer 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 and with the least amount of errors. Make sure your team implements a code review process for new code that is introduced into your code base. Find a process that works for you and your team, there's no one size fits all. The important point is to do code reviews as regularly as possible.

Code reviews should:

• Automate as much of the review as you can. That means avoid details that can be handled by a static analysis tool. Don't argue about details such as code formatting and whether to use let or var. Having a formatter and linter can save your team a lot of time from reviews that your computer can do for you.
• Avoid API discussion. 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.
• Be kind. It's scary to have your code reviewed and it can bring about feelings of insecurity in even the most experienced developer. Be positive in your language and keep your teammates comfortable and secure in their work!

Avoid nitpicking as much as you can and save it 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!

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 about by suggesting a clearer name if one doesn't make sense.

// This function could be better named as namesToUpperCase
function u(names) {
  // ...
}

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 functions.

// 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);
    }
  });
}

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 below and includes functions that don't relate to one another ,then it should probably be split apart.

1: import _ from 'lodash';
2: function generateFakeNames() {
3:   // ..
4: }
...
1128: function queryRemoteDatabase() {
1129:   // ...  
1130: }

If your function is intended to be used by other libraries, it helps to add documentation so users of it know what it does.

// This needs documentation. What is this function for? How is it used?
export function networkMonitor(graph, duration, failureCallback) {
  // ...
}

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;

  if (len <= 0) return str;

  if (!ch && ch !== 0) ch = ' ';

  ch = ch + '';

  if (ch === ' ' && len < 10) return cache[len] + str;

  var pad = '';

  while (true) {
    // This needs a comment, why a logical 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;
}

// Global variable referenced by following function.
// If we had another function that used this name, now it'd be an array and it could break it.
let name = 'Ryan McDermott';

function splitIntoFirstAndLastName() {
  name = name.split(' ');
}

splitIntoFirstAndLastName();

console.log(name); // ['Ryan', 'McDermott'];

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);
}

Users can potentially input an unlimited amount of data to send to you. It's important to set limits if a function takes any kind of user data in.

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);
});

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);
});

Data security is the most important aspect of your application. If users can't trust you with their data, then you won't have a business. There are numerous different types of security exploits that can plague an app, depending on the particular language and runtime environment. Below is a very small and incomplete list of common security problems. Don't rely on this alone! Automate as much security review as you can on every commit, and perform routine security audits.

This is different for every particular case, but use your best judgment to see if there are any ways to improve the efficiency of a piece of code. Your users will thank you for the faster speeds!

// If mentions was a hash data structure you wouldn't need to iterate through
// all mentions to find a user. You could simply return the presence of the
// user key in the mentions hash
function isUserMentionedInComments(mentions, user) {
  let mentioned = false;
  mentions.forEach(mention => {
    if (mention.user === user) {
      mentioned = true;
    }
  })

  return mentioned;
}

Logging helps give metrics about performance and insight into user behavior. Not every action needs to be logged, but decide with your team what makes sense to keep track of for data analytics. And be sure that no personally identifiable information is exposed!

"Everything can be filed under miscellaneous"

George Bernard Shaw