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?

Refactoring or any code rewrite, especially after your code works, is really a matter of writing the code is understandable (comments help), maintainable (fixing emerging bugs and adding new functionality), and ensuring that your code is fairly efficient (i.e., combining code and reducing repetitive code). Yes, there is a point where each of these goals can be taken too far, but as long as you can understand and maintain the code, then you've probably not taken the code 'too far'. Remember, the code is for you and possibly for your current/future team members. Beyond that, it's up to you. — , Jun 23 '20 at 13:01

Maximo Mussini · Accepted Answer · 2020-06-23T13:48:47.760

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.

score 0 · Answer 2 · answered Jun 23 '20 at 12:57

IMHO the criteria are testability and readability. First means the function can be easily tested. If the number of params will increase, the size of unit test of the function will also increase. If your function do something else (not one exact operation) your test function will have to test it also. All the control structures of your function forces you to test them.

So your function is small enough if it can be easily and completely be tested by unit testing.

Refactoring a big function into functions within the function?

2 Answers2

First Example

Second Example