Refactoring a big function into functions within the function?

Question

I have two functions that are not so big, and not so complex (maybe because I wrote them they don't seem so at the moment) and I have tried refactoring them (successfully), however, would that be considered overdoing it:

Original function:

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

Into:

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

Original second function:

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

Into:

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

Now obviously the first thing is that, this is much more code when refactored like this, and looking at the before and after of the functions while writing them here, the original approach is more readable, but I have them arranged one after another in order in my code and their internal code is "minimized", so I just see the names of the functions in their logical order.

So my question is, would this be considered like something that I have to do?

If not, what criteria should a function meet for me to have to do something like this?

And is that even the right way to do it?

Answer 1

One first tip, is to leverage that functions in JS are first-class citizens, so this one:

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

can be written as:

  robots.forEach(doSomethingOnRobots)

Something that might make the refactoring feel awkward is that some of these extracted functions need robot as a parameter, which in the original are accessed via closure.

First Example

You can look for ways to split the function in a way that preserves this closure. Since you used async in the example, you could leverage it for the first promise as well:

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)

By separating the core logic (checking the robot status) from the timer and the iteration, we make the pingRobot function easier to test.

Second Example

Regarding the second function, it might desirable to replace the iteration with a structure that allows you to obtain a type from the event DataType. An example using keyBy (which you can implement manually if needed):

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

The main trick is again to see which closures you were leveraging in the original code, and preserve them to avoid verbosity. Although it might be a bit longer, onRobotEvent has become easier to reason about, and test in isolation.