code-review-tips【翻译】 - 起源地下载网

介绍

代码检视会给检视者和被检视者带来恐惧，当你的代码被别人拿来分析时你可能也会感到被侵犯或者不适，更糟糕的是当你检视别人的代码而不知道从哪里开始时，那将是一场非常痛苦和混乱的体验。

本篇文章旨在为你检视你或者你的团队的代码时提供一下扎实的建议，虽然例子都是JavaScript写的，但是这些建议适用于任何项目，任何编程语言。这绝对不是一个详尽的列表，但是希望它可以帮助您在用户看到您的功能之前就捕获尽可能多的错误。

为什么要进行代码检视

代码检视是软件工程开发时比不可少的一部分，因为你自己一个人是不可能发现你编写的代码中的所有问题，这很正常，即使是最好的篮球运动员也不可能每次都投中。

让别人检视我们的代码能够确保我们在交付给用户我们的产品时犯下尽可能少的错误，所以确保在代码库中引入新代码前让你的团队进行代码检视，找到适合你和你的团队的流程。没有一个适合所有人的方案，但是重要的是要尽可能定期进行代码检视。

Basics 基础

代码检视要尽可能自动化

避免讨论可以由静态分析工具处理的细节。不要争论诸如代码格式之类的细微差别以及是否应该使用let或var。格式化程序和lint可以为你的团队节省大量时间。

代码检视要避免讨论API

类似的讨论要在写代码之前就要明确下来，打完地基之后再去讨论平面图已经失去意义了。

代码检视要体现友好

即使是最有经验的程序员在代码被检视时也会产生不安的情绪，组织语言时尽量保持积极态度，让你的同事在工作时尽可能的放松安心！

Readability 可读性

错别字需要纠正

类似的问题尽量留给formatter或者linter这些工具去提示解决，如果仍然有类似的错别字问题，留下一个善意的提醒，建议开发修复，虽然这是很小的问题，但是有时会有意向不到的效果。

变量和函数的命名要有意义

命名是计算机科学里最难得问题之一。我们都给变脸，函数或者文件起过令人困惑的名字。如果你看到类似无意义的命名，请给你的同事一些命名的建议。

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

函数应该短一些

一个函数只应该做一件事情，太长的函数通常意味着它做了非常多的事情，请告诉你的同事把一个长函数拆分成多个函数来实现独立的功能。

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

文件内容应该是内聚的，理想情况下内容尽可能短

和函数一样，一个代码文件也应该尽量去做一件事情，一个文件代表着一个模块，一个模块应该只为你的代码库做一件事情。

举个例子，如果你的模块叫fake-name-generator那它应该只用来创建一个假名，如果这个模块还包含一大堆功能来查询数据库中的名字，那它就应该被拆分成另外的模块。

关于文件多长并没有一个明确的规则，但是如果它像下面的代码这样，并且包含彼此不相关的功能，那我们就应该对它进行拆分。

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

// Line 1128
function queryRemoteDatabase() {
  // ...
}

导出函数时需要做文档注释

如果你的函数有意向被其他的库使用，那为你的函数加上文档注释将有助于其他开发在使用时知道他是干什么的。

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

复杂的代码需要说明注释

如果你觉得你代码中的命名起的可以但是逻辑依然让人困惑，那是时候做个说明注释了。

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

Side Effects 副作用

函数要尽可能纯净

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

进行IO操作的函数需要处理失败场景

任何函数如果有关于IO操作的场景，都需要有处理失败或者异常场景的逻辑。

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

Limits 边界范围

需要处理变量为空的场景

比如你有一个列表组件，也可以用表格完美地呈现出所有数据，你的用户非常喜欢它，你获得了嘉奖和晋升。但是万一你的组件获得的数据为空时会怎么展示？你的代码应该对各种可能发生的场景都是兼容的，如果你的代码存在某些隐患，那么它最后就会发生。

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

超大数据的场景也要被处理

上面的组件如果号要展示10000个以上的数据要怎么处理？也许你需要为组件增加分页的逻辑而不是无限的向下滚动，确保在容量的角度对潜在的边界场景进行评估，特别是在进行UI方面的编程时。

单一数据场景也要处理

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

用户的输入需要做限制

用户可能会输入的数据可能会超出你的代码的限制，这个时候在你的函数中做一些输入限制就非常重要。

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

函数需要处理用户的异常输入

用户传给你的数据总是会给你很多惊喜，不要期望用户给你的数据永远是正确的类型，也不要只依赖客户端的验证。

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 安全

数据安全是你的应用中非常重要的一个环节，如果用户没办法放心把数据交给你管理，那生意自然也做不成。根据编程语言和运营环境的不同，你的应用可能会有大量的安全隐患，下面我会列举一个常见安全问题的小清单，绝不完整，不要依赖这个，尽可能的注意每一次代码体交中的安全问题，把这方面当做一个规定来执行。

不能允许XSS隐患

跨站点脚本(XSS)是关于网页应用安全攻击中最多的方式之一，当你拿到用户的输入信息后没有进行数据净化而直接去使用，那XSS攻击就很可能发生。这可能导致你的网站被客户端页面操纵在服务端执行一些代码操作。

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

  /**
   * What if nameQueryParam was `<script>sendCookie(document.cookie)</script>`?
   * 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;
}

不能遗漏个人信息验证

每次处理用户数据时，您都要承担巨大的责任。如果你在 url 中泄露数据，在分析跟踪中泄露给第三方，或者甚至将数据暴露给不应该访问的员工，你会极大地伤害你的用户和你的企业。小心别人的生活！

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

Performance 性能

函数应该使用高效的算法和数据结构。

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

应该记录重要的操作

日志记录有助于提供性能指标和对用户行为的洞察力。不是所有的行动都需要记录，但是要和你的团队一起决定什么对于数据分析来说是有意义的。确保没有个人身份信息暴露在外！

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

Testing 测试

应该测试新的代码

所有的新代码都应该包含一个测试，无论它是修复了一个错误，还是一个新特性。如果这是一个 bug 修复，它应该有一个测试证明这个 bug 是修复的。如果这是一个新特性，那么每个组件都应该经过单元测试，并且应该有一个集成测试来确保这个特性能够与系统的其余部分一起工作。

应该测试函数的所有功能

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

Miscellaneous 杂项

TODO注释应该被跟踪

TODO 注释非常适合让你和你的工程师同事知道有些东西需要以后修复。有时候你需要发送代码，然后等待以后修复它。但是最终你必须清理它！这就是为什么您应该跟踪它，并从问题跟踪系统中提供相应的 ID，以便您可以安排它并跟踪问题在代码库中的位置

提交消息应该清晰准确地描述新代码

我们都写过诸如“修改了一些垃圾”、“该死的”、“再修复一个愚蠢的 bug”之类的提交消息。这些都很有趣，也很令人满意，但是当你在周六早上起床的时候，这些并没有什么帮助，因为你在周五晚上推送代码，当你把错误的代码归咎于提交的时候，你不知道它们在做什么。编写能够准确描述代码的提交消息，如果有问题跟踪系统的话，还要包含一个票证号码。这将使搜索提交日志变得更加容易。

代码应该做它应该做的事情

这似乎是显而易见的，但大多数审阅者没有时间或花时间手动测试每一个面向用户的更改。确保每个更改的业务逻辑都是按照设计进行的，这一点很重要。当您只是在代码中寻找问题时，很容易忘记这一点！

介绍