将一个大函数重构为函数内的函数？

Question

我有两个函数不是那么大，也不是那么复杂（也许是因为我写的，它们现在看起来并不那么复杂），我已经尝试重构它们（成功），但是，那会是考虑过头了：

原函数：

setInterval(
  () => {
    robots.forEach((robot) => {
      ping.promise.probe(robot.IP).then(async (resp) => {
        if (resp.alive) {
          for (let dataType of TypesOfDataToGet) {
            await dataType.Set(robot);
          }
        } else {
          console.log(`${robot.Name.EN} is offline!`);
        }
      });
    });
  }, 400);

进入：

function iterateRobots(robots, doSomethingOnRobots) {
  robots.forEach((robot) => {
    doSomethingOnRobots(robot);
  });
}
function pingRobots(robot) {
  ping.promise.probe(robot.IP).then(async (resp) => {
    getRobotDataIfRobotAlive(resp, robot);
  });
}
async function getRobotDataIfRobotAlive(resp, robot) {
  if (resp.alive) {
    for (let dataType of TypesOfDataToGet) {
      await dataType.Get(robot);
    }
  } else {
    console.log(`${robot.Name.EN} is offline!`);
  }
}

setInterval(() => {
  iterateRobots(robots, pingRobots);
}, 400);

原第二个函数：

robots.forEach((robot) => {
  robot.Events.forEach((event) => {
    socket.on(event, (data) => {
      let eventStartIndex = event.lastIndexOf("-");
      let eventDataType = event.substring(eventStartIndex + 1);

      for (let currentDataType of TypesOfDataToSet) {
        if (currentDataType.DataType === eventDataType.toUpperCase()) {
          currentDataType.Set(robot, data);
          break;
        }
      }
    });
  });
});

进入：

function iterateRobots(robots, doSomethingOnRobots) {
  robots.forEach((robot) => {
    doSomethingOnRobots(robot);
  });
}
function iterateEvents(robot) {
  robot.Events.forEach((event) => {
    sendDataBasedOnEventType(robot, event)
  });
}
function sendDataBasedOnEventType(robot, event) {
  socket.on(event, (data) => {
    let eventStartIndex = event.lastIndexOf("-");
    let eventDataType = event.substring(eventStartIndex + 1);

    for (let currentDataType of TypesOfDataToSet) {
      if (currentDataType.DataType === eventDataType.toUpperCase()) {
        currentDataType.Set(robot, data);
        break;
      }
    }
  });
}
    
iterateRobots(robots, iterateEvents);

现在显然第一件事是，像这样重构时代码要多得多，在这里编写函数时查看函数的前后，原来的方法更具可读性，但我已经安排了它们在我的代码中一个接一个地顺序排列，它们的内部代码是“最小化”的，所以我只看到函数名称的逻辑顺序。

所以我的问题是，这会被视为我必须做的事情吗？

如果不是，函数应该满足什么条件才能让我做这样的事情？

这是正确的方法吗？

Answer 1

恕我直言，标准是可测试性和可读性。首先意味着可以轻松测试该功能。如果参数的数量增加，函数单元测试的大小也会增加。如果您的函数执行其他操作（不是一个确切的操作），您的测试函数也必须对其进行测试。您函数的所有控制结构都迫使您对其进行测试。

所以如果可以容易并且完全通过单元测试来测试你的函数就足够小了。

Answer 2

第一个技巧是利用 JS 中的函数是第一个 class 公民，所以这个：

  robots.forEach((robot) => {
    doSomethingOnRobots(robot)
  })

可以写成：

  robots.forEach(doSomethingOnRobots)

可能会使重构感到尴尬的是，其中一些提取的函数需要 robot 作为参数，而在原始情况下，这些函数是通过闭包访问的。

第一个例子

您可以寻找以保留此闭包的方式拆分函数的方法。由于您在示例中使用了 async，因此您也可以将其用于第一个承诺：

async function pingRobot (robot) {
  const resp = await ping.promise.probe(robot.IP)

  if (!resp.alive) return console.log(`${robot.Name.EN} is offline!`)

  for (let dataType of TypesOfDataToGet) {
    await dataType.Set(robot)
  }
}

setInterval(() => robots.forEach(pingRobot), 400)

通过将核心逻辑（检查机器人状态）与计时器和迭代分开，我们使 pingRobot 功能更易于测试。

第二个例子

关于第二个函数，可能需要将迭代替换为允许您从事件中获取类型的结构 DataType。使用 keyBy 的示例（如果需要，您可以手动实现）：

const typesByDataType = keyBy(TypesOfDataToSet, 'DataType')

function onRobotEvent ({ robot, event, data }) {
  const eventStartIndex = event.lastIndexOf("-")
  const eventDataType = event.substring(eventStartIndex + 1).toUpperCase()
  const eventType = typesByDataType[eventDataType]
  if (eventType) eventType.Set(robot, data)
}

robots.forEach(robot => {
  robot.Events.forEach(event => {
    socket.on(event, data => {
      onRobotEvent({ robot, event, data })
    })
  })
})

主要技巧再次是查看您在原始代码中利用了哪些闭包，并保留它们以避免冗长。虽然它可能会更长一些，但 onRobotEvent 变得更容易推理和单独测试。

将一个大函数重构为函数内的函数？

Refactoring a big function into functions within the function?

javascript

software-quality

第一个例子

第二个例子