CodeReview · 常见的一些问题

前言

在本文中，我们将深入探讨如何识别和解决常见的Code Review问题，包括但不限于：

• 代码风格和一致性：如何确保代码遵循团队约定的风格指南？
• 代码逻辑和架构：如何评估代码的可读性和可维护性？
• 错误和潜在的Bug：如何发现并修复潜在的问题和错误？
• 性能和优化：如何优化代码以提高性能和效率？
• 安全性和漏洞：如何确保代码不会引入安全风险？

示例

下面我们将通过几个例子，来解决一些在日常开发中经常见到的不那么友好的代码，甚至是 屎山

回调地狱

getUserInfo().then((userInfo) => {
  getArticles(userInfo).then((articles) => {
    Promise.all(articles.map(article => getArticleDetail(article)))
      .then((articleDetails) => {
        console.log(articleDetails)
      }
    )
  })
})

getUserInfo()
.then(getArticles)
.then((articles) => Promise.all(articles.map(getArticleDetail)))
.then(console.log)

将回调地狱巧妙地改成了 链式调用 ，即使不用 async await，代码看起来也十分优雅。

滥用try catch

const getUserInfo = async () => {
  try {
    const userInfo = await fetch('/api/getUserInfo')
  } catch (error) {
    // 为了避免报错的空白catch
  }
}

我们都知道 trycatch 是用来捕获错误然后处理异常的，但经常有的同学为了让程序不报错，而去做空的catch。

实际上 报错并不可怕 ，它能让你及时的发现问题，从而解决问题。很多时候我们要做各种各样的处理，让程序更加及时的把错误暴露出来，我们才能解决问题，而不是等程序上线之后才发现问题的存在。

泛滥的函数入参

const getUserInfo = (name,age,weight,gender,mobile,hobby,address){ // ... }

// 将所有的参数合并成一个对象，再去函数体内进行解构
const getUserInfo = (options){ 
  const {name,age,weight,gender,mobile,hobby,address} = options
  // ...
}

getUserInfo({
  name:'123',
  address:'abc',
  ...
})

这种情况相信大家也都看到过，它一般是刚开始写函数的时候没几个参数，但是随着业务的拓展，参数越加越多。

这种做法的弊端是他会给调用函数的人造成非常大的 心智负担 ，他不仅要知道有多少个参数，还需要知道每个参数的顺序。

将所有的参数合并成一个对象之后，使用它的人就没有了心智负担，只需要传一个参数，且不在乎顺序，同时代码也很好阅读。

魔法数字

// component1.js
if(state === 1 || state === 2){
  // ...
} else if (state === 3) {
  // ...
}

// component2.js
if(state === 1 || state === 2){
  // ...
}

const NORMAL_USER = 1
const VIP_USER = 2
const SUPER_VIP_USER = 3

// component1.js
if(state === NORMAL_USER || state === VIP_USER){
  // ...
} else if (state === SUPER_VIP_USER) {
  // ...
}

// component2.js
if(state === NORMAL_USER || state === VIP_USER){
  // ...
}

魔法数字 就是在你的代码里面出现的一些莫名其妙的数字，你也搞不清这个数字到底是啥意思，同时代码也很难阅读。

所以我们应该通过定义 常量 去让代码的可读性可维护性变得更高。

无意义的注释

let id = 1; // 给id赋值为1

let id = 1; // 定义要查询的文章id

注释的意义在于解释这行代码没有提供的信息，上面的例子中已经给我们提供的信息就是定义了一个变量id且赋值为1，所以再写一遍这个信息到注释里面，是毫无意义的。

我们需要在注释里提供的信息应该是为什么要把id赋值为1？以及你后续的代码要怎么来使用这个变量，写清楚 for what 和 why 。

合理利用命名空间缩减变量属性名前缀

class User {
  userName;
  userAge;
  userPwd;

  userLogin(){},
  getUserProfile(){}
}

class User {
  ame;
  age;
  pwd;

  login(){},
  getProfile(){}
}

这个问题实际上是命名的问题，我们已经定义了一个类名为User，再去定义内部的变量时，又将类名作为前缀，这是复杂且啰嗦的。

使用映射替换分支逻辑的使用映射

function couter(state = 0, action) {
  switch (action.type) {
    case 'INCREMENT':
      return state + 1
    case 'DECREMENT':
      return state - 1
    default:
      return state
  }
}

function couter(state = 0, action) {
  const ins = {
    INCREMENT: 1,
    DECREMENT: -1,
  }
  return state + (ins[action.type] || 0)
}

在软件开发中，使用 映射 替换分支逻辑是一种优化代码结构和提升可读性的有效策略。通过将条件判断和分支逻辑替换为映射（例如字典或对象），我们能够更简洁地管理不同情况下的处理方式。这种方法不仅减少了复杂的条件判断，还使代码更易于理解和维护，同时提高了代码的灵活性和可扩展性。

无法阅读的条件判断

function checkGameStatus() {
  if (remaining === 0
    || (remaining === 1 && remainingPlayers === 1)
    || remainingPlayers === 0
  ) {
    quitGame()
  }
}

function isGameOver() {
  return (remaining === 0
    || (remaining === 1 && remainingPlayers === 1)
    || remainingPlayers === 0
  )
}

function checkGameStatus() {
  if (isGameOver()){
    quitGame()
  }
}

这是最让人头疼的一类代码，写了一大堆的判断，根本没办法阅读，你无从知晓这个判断是用来干啥的。

我们可以将复杂的逻辑判断 抽离 成一个函数，这样代码就很好阅读，整体的逻辑也变得更加顺畅。

逻辑混乱

function publishPost(post) {
  if (isLoggedIn()) {
    if (post) {
      if (isPostDoubleChecked()) {
        doPost(post)
      } else {
        throw new Error('不要重复发布文章')
      }
    } else {
      throw new Error('文章内容不能为空')
    }
  } else {
    throw new Error('请先登录')
  }
}

function publishPost(post) {
  if(!isLoggedIn()){
    throw new Error('请先登录') 
  }
  if(!post){
    throw new Error('文章内容不能为空') 
  }
  if(!isPostDoubleChecked()){
    throw new Error('不要重复发布文章')
  }
  doPost(post)
}

经常有同学会将一些正常的业务处理和错误的处理放在一起，这样会让代码变得不是很好阅读。一般来说我们可以先处理错误，再去处理正常的业务，这是一种非常常见的开发技巧。

硬编码（hard code）

function responseInterceptor(resp) {
  const token = resp.headers.get('authorization') // hard code
  if (token) {
    localStorage.setItem('token', token)
  }
}

// ajax请求拦截
function requestInterceptor(config) {
  const token = localStorage.getItem('token')
  if (token) {
    config.headers.Authorization = `Bearer ${token}`
  }
  return config
}

const AUTH_HEADER_KEY = 'authorization'
const AUTH_KEY = 'token'

function responseInterceptor(resp) {
  const token = resp.headers.get(AUTH_HEADER_KEY)
  if (token) {
    localStorage.setItem(AUTH_KEY, token)
  }
}

// ajax请求拦截
function requestInterceptor(config) {
  const token = localStorage.getItem(AUTH_KEY)
  if (token) {
    config.headers.Authorization = `Bearer ${token}`
  }
  return config
}

硬编码就是在代码的某些地方直接写一些字面量，如果它不会重复的话还好，一旦发生重复的话，维护起来就会变得特别困难。而且在上面的例子中，两个函数在产生一个隐式的耦合，就是他们之间没有相互调用，但是仍然耦合了。耦合的地方就是 字面量 。

所以我们应该将这些字面量提取成常量，这样才能真正意义上的将两个函数完全解耦，也更加方便之后的维护。

总结

这是十个比较常见的在codereview中发现的问题，当然问题肯定也不止这些。代码审查在软件开发中的重要性可想而知，不仅可以改善代码质量，还能促进团队合作和知识共享。希望同学们能够在日常工作中应用这些方法，从而提升代码审查实践，进而提高团队的整体效率和开发质量。