【翻译】

This article is translated from github.com/ryanmcdermo… Any resemblance is purely coincidental.

introduce

Code review can be scary for both reviewers and those under review. It can be intrusive or uncomfortable when your code is being analyzed by someone else. Worse, it can be a painful and confusing experience to review someone else’s code without knowing where to start.

The purpose of this article is to provide solid advice for reviewing your or your team’s code. Although the examples are written in JavaScript, these tips can be applied to any project, in any programming language. This is by no means an exhaustive list, but hopefully it will help you catch as many bugs as possible before users see your functionality.

Why code review

Code review is an integral part of software engineering development, because you can’t find every problem in the code you write by yourself, and that’s normal. Even the best basketball players can’t make every shot.

Having someone else review our code ensures that we make as few mistakes as possible when delivering our product to users, so be sure to have your team review your code before introducing new code into your code base to find the process that works for you and your team. There is no one-size-fits-all solution, but it is important to review code as regularly as possible.

Basics basis

Code review should be as automated as possible

Avoid discussing details that can be handled by static analysis tools. Don’t argue about nuances like code format and whether you should use let or VAR. Formatters and Lint can save your team a lot of time.

Code reviews avoid discussion of apis

These kinds of discussions need to be made clear before the code is written, and it doesn’t make sense to talk about floor plans after the foundation has been laid.

Code review should be friendly

Even the most experienced programmers feel uneasy when their code is being reviewed. Keep a positive attitude when you structure your language and make your colleagues as relaxed as possible!

Readability Readability

Typos need to be corrected

Leave it to formatter or Linter to help you solve the problem. If you still have a typo problem, leave a friendly reminder and suggest that you fix it. Although this is a minor problem, it can sometimes have unintended effects.

Name variables and functions meaningfully

Naming is one of the hardest problems in computer science. We’ve all given faces, functions or files confusing names. If you see this sort of nonsense, please give your colleagues some naming suggestions.

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

Copy the code

The function should be shorter

A function should only do one thing. Too long a function usually means it does too many things. Please tell your colleagues to break up a long function into multiple functions to achieve independent 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); }}); }Copy the code

File content should be cohesive, ideally as short as possible

Like functions, a code file should try to do one thing. A file represents a module, and a module should only do one thing for your code base.

For example, if your module is called fake-name-generator it should only be used to create a pseudonym, and if it has a bunch of functions to query names in the database it should be split into separate modules.

There is no definitive rule on how long a file should be, but if it looks like the code below and contains functions unrelated to each other, it should be split.

// Line 1
import _ from 'lodash';
function generateFakeNames() {
  // ..
}

// Line 1128
function queryRemoteDatabase() {
  // ...
}
Copy the code

You need to annotate the documentation when exporting functions

If your function is intended to be used by other libraries, documenting your function will help other developers know what it does when they use it.

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

Complex code requires explanatory comments

If you think the naming in your code is ok but the logic is still confusing, it’s time to make 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;
}
Copy the code

Side Effects

The function has to 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();
Copy the code

Functions that perform IO operations need to handle failure scenarios

Any function that has IO operation scenarios needs to have logic to handle failure or exception scenarios.

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);
}
Copy the code

Limits

You need to handle scenarios where variables are empty

Let’s say you have a list component, or you can display all your data perfectly in a table, your users love it, you get rewards and promotions. But what if your component gets empty data? Your code should be compatible with every possible scenario, and if your code has a bug in it, it will eventually happen.

class InventoryList {
  constructor(data) {
    this.data = data;
  }

  render() {
    return (
      <table>
        <tbody>
          <tr>
            <th>ID</th>
            <th>Product</th>
          </tr>
          // We should show something for the null case here if there's // nothing
          // in the data inventory
          {Object.keys(this.data.inventory).map(itemId => (
            <tr key={i}>
              <td>{itemId}</td>

              <td>{this.state.inventory[itemId].product}</td>
            </tr>
          ))}
        </tbody>
      </table>); }}Copy the code

Large data scenarios are also processed

What will happen to the above component if it wants to display more than 10000 data? Perhaps you need to add pagination logic to your components rather than infinite scrolling down, and make sure to evaluate potential boundary scenarios from a capacity perspective, especially when programming UI.

Single data scenarios are also processed

class MoneyDislay {
  constructor(amount) {
    this.amount = amount;
  }

  render() {
    // What happens if the user has 1 dollar? You can't say plural "dollars"
    return (
      <div className="fancy-class">
        You have {this.amount} dollars in your account
      </div>); }}Copy the code

User input needs to be restricted

The user may enter more data than your code can handle, so it’s important to put some input limits in your functions.

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);
});
Copy the code

The function needs to handle abnormal input from the user

The data that users send you will always surprise you. Don’t expect the data to always be of the right type, and don’t rely solely on client validation.

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);
});
Copy the code

Security safety

Data security is a very important part of your application, and if your users can’t trust you with their data, your business won’t be successful. Depending on your programming language and operating environment, your application may have a large number of security vulnerabilities. Here is a short list of common security vulnerabilities. Don’t rely on this.

XSS vulnerabilities cannot be allowed

Cross-site scripting (XSS) is one of the most common forms of web application security attacks. XSS attacks can occur when you take user input and use it without cleansing the data. This can cause your site to be manipulated by client pages to perform code operations on the server side.

function getBadges() {
  let badge = document.getElementsByClassName('badge');
  let nameQueryParam = getQueryParams('name');

  /** * What if nameQueryParam was ``? * If that was the query param, a malicious user could lure a user to click a * link with that as the `name` query param, and have the user unknowingly * send their data to a bad actor. */
  badge.children[0].innerHTML = nameQueryParam;
}
Copy the code

Do not omit personal information verification

You have a huge responsibility every time you process user data. If you leak data in urls, give it away to third parties in analytics tracking, or even expose it to employees who shouldn’t be accessing it, you’re doing your users and your business a huge disservice. Watch out for other people’s lives!

router.route('/bank-user-info').get((req, res) = > {
  const name = user.name;
  const id = user.id;
  const socialSecurityNumber = user.ssn;

  // There's no reason to send a socialSecurityNumber back in a query parameter
  // This would be exposed in the URL and potentially to any middleman on the
  // network watching internet traffic
  res.addToQueryParams({
    name,
    id,
    socialSecurityNumber
  });
});
Copy the code

The Performance properties of

Functions should use efficient algorithms and data structures.

// 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;
}
Copy the code

Important actions should be recorded

Logging helps provide performance metrics and insight into user behavior. Not all actions need to be documented, but work with your team to determine what makes sense for data analysis. Make sure no personally identifiable information is exposed!

router.route('/request-ride').post((req, res) = > {
  const currentLocation = req.body.currentLocation;
  const destination = req.body.destination;

  requestRide(user, currentLocation, destination).then(result= > {
    // We should log before and after this block to get a metric for how long
    // this task took, and potentially even what locations were involved in ride
    // ...
  });
});
Copy the code

Testing the test

The new code should be tested

All new code should include a test, whether it fixes a bug or a new feature. If this is a bug fix, it should have a test that proves the bug is fixed. If this is a new feature, each component should be unit tested, and there should be an integration test to make sure the feature works with the rest of the system.

All functionality of a function should be tested

function payEmployeeSalary(employeeId, amount, callback) {
  db.get('EMPLOYEES', employeeId)
    .then(user= > {
      return sendMoney(user, amount);
    })
    .then(res= > {
      if (callback) {
        callback(res);
      }

      return res;
    });
}

const callback = res= > console.log('called', res);
const employee = createFakeEmployee('john jacob jingleheimer schmidt');
const result = payEmployeeSalary(employee.id, 1000, callback);
assert(result.status === enums.SUCCESS);
// What about the callback? That should be tested
Copy the code

Miscellaneous Miscellaneous

Everything can be filed under miscellaneous

George Bernard Shaw

TODO comments should be traced

TODO comments are great for letting you and your fellow engineers know that something needs to be fixed later. Sometimes you need to send code and wait to fix it later. But eventually you have to clean it up! Is that why you should track it and provide the appropriate ID from the problem tracking system so that you can schedule it and track where the problem is in the code base

Commit messages should clearly and accurately describe the new code

We’ve all written commit messages like “fixed some junk,” “Damn it,” “Fixed one more stupid bug.” It’s all fun and satisfying, but it doesn’t help when you get up on a Saturday morning, because you push code on A Friday night, and when you blame bad code on a commit, you don’t know what it’s doing. Write a submission message that accurately describes the code and includes a ticket number if there is a problem tracking system. This will make searching the commit log much easier.

The code should do what it should do

This may seem obvious, but most reviewers don’t have the time or time to manually test every user-facing change. It is important to ensure that the business logic for each change is as designed. It’s easy to forget this when you’re just looking for problems in your code!