Putting the “fun” into refactoring functions

Right now we have two functions in our todo app: getRemaining and filteredTodos:

// Returns the remaining number of undone todos.
function getRemaining(todos) {
  var count = 0;
  todos.forEach((todo) => {
    count += todo.done ? 0 : 1;
  });
  return count;
}

// Returns the undone todos.
function filteredTodos(todos) {
  var oldTodos = todos;
  todos = [];
  oldTodos.forEach((todo) => {
    if (!todo.done) {
      todos.push(todo);
    }
  });
  return todos;
}

Let’s start with the getRemaining function.

Refactoring the getRemaining function

In essence, this function is doing the following:

• Looping through each item in the todos array
• Returning the count of todos that are not done.

Suppose we were writing a function to get the count for all the todos. We could write something like:

function getCountForAll(todos) {
  var count = 0;
  todos.forEach((todo) => {
    count += 1;
  });
  return count;
}

This would work, but the function is still quite cumbersome for what it’s trying to do, because at this point, I’m just returning the length of the todos array. So why don’t I just put this using the array length property?

function getCountForAll(todos) {
  return todos.length;
}

The result will be the same, except now I’ve reduced my lines of code from seven to three.

Now we need to filter the array so that it only counts the todos that are not done. We can do that using the array filter method:

function getRemaining(todos) {
  return todos.filter((todo) => !todo.done).length;
}

Now the function will only return the length of the array that has been filtered to only show ones that aren’t done.

Refactoring the filteredTodos function

In our filteredTodos function, we have this:

function filteredTodos(todos) {
  var oldTodos = todos;
  todos = [];
  oldTodos.forEach((todo) => {
    if (!todo.done) {
      todos.push(todo);
    }
  });
  return todos;
}

Like the previous function, this function is trying to do something with todos that aren’t done, but instead of returning just the length, it’s returning a new array. So why don’t we use the same filter method we used before?

function filteredTodos(todos) {
  return todos.filter((todo) => !todo.done);
}

Now this function is reduced from ten lines of code to three, and it’s a little easier to understand what’s happening.

Don’t repeat yourself

We’ve done a good job refactoring these two functions, but now there’s an issue where we’re repeating ourselves:

function getRemaining(todos) {
  return todos.filter((todo) => !todo.done).length;
}

function filteredTodos(todos) {
  return todos.filter((todo) => !todo.done);
}

This isn’t a big deal now since we’re dealing with only two functions. But what if I start adding more like this, and what if down the road, I decide to rename the done property to completed?

We can solve this by creating a new function that just handles returning the incomplete todos like this:

function incompleteTodos(todos) {
  return todos.filter((todo) => !todo.done);
}

function getRemaining(todos) {
  return incompleteTodos(todos).length;
}

function filteredTodos(todos) {
  return incompleteTodos(todos);
}

Now if down the road I decide to change the done property, I can do it within the incompleteTodos function without having to change the other functions.

Do I even need these functions now?

We’ve done such a good job refactoring that our functions and methods are down to only three lines each.

class Controller
  remaining() {
    return getRemaining(this.todos);
  }

  archive() {
    this.todos = filteredTodos(this.todos);
  }
  ...
}

function incompleteTodos(todos) {
  return todos.filter((todo) => !todo.done);
}

function getRemaining(todos) {
  return incompleteTodos(todos).length;
}

function filteredTodos(todos) {
  return incompleteTodos(todos);
}

Since the getRemaining and filteredTodos functions are only one line within the block, let’s just put those directly into the Controller so we don’t have unnecessary functions:

class Controller
  remaining() {
    return incompleteTodos(this.todos).length;
  }

  archive() {
    this.todos = incompleteTodos(this.todos);
  }
  ...
}

function incompleteTodos(todos) {
  return todos.filter((todo) => !todo.done);
}

Now it’s even clearer what these methods do and we’ve been able to unnecessary functions.