A story moves people. Code moves data. If your code tells a story, it moves people and data.

Dude, you’re not making any sense. What’s this about code telling a story?

Recently, I’ve been reading Clean Code by Robert Martin (or as he's affectionately called, Uncle Bob). It’s one of those must read books for anyone serious about working in or around Software Engineering. It feels like every page holds a nugget of information that I can chisel at to adapt to my work, so I’m taking my time with it.

For instance, take this quote by Grady Brooks, it appears on the introduction for the book:

Clean code is simple and direct. Clean code reads like well-written prose. Clean code never obscures the designer’s intent but rather is full of crisp abstractions and straightforward lines of control.

The first time I read that clean code reads like well-written prose, the light-bulb in my head turned on 💡. It made so much sense, seemed to simple, and yet I’d been neglecting this part of my craft for a while. I mean, take a look at this code:

const newOrUpdateTask = (todoLi, action, todoId) => {
  const todoDiv = todoLi.lastElementChild.previousElementSibling;
  const addBtn = todoLi.firstElementChild.nextElementSibling;
  const taskText = todoDiv.textContent;
  todoDiv.classList.add('noselect');
  todoDiv.contentEditable = 'false';

  const todo = {
    name: taskText,
    status: 'new',
  };

  db[todoId] = todo;
  todoLi.dataset.status = action;

  if (action === 'new') {
    todoLi.dataset.id = todoId;
    todoDiv.dataset.action = 'update';
    addBtn.dataset.action = 'update';
    todoLi.parentNode.append(todoLi);
    todosList.prepend(todosTaskTemplate.cloneNode(true));
    todosList.firstElementChild.lastElementChild.previousElementSibling.focus();
  }
  writeToDBEveryThreeSecs();
  return null;
};

There is so much wrong with this code that I’m not going to go through all of it. Let’s start with what should be the obvious; this function is doing too much!

The name itself is an indicator that it’s doing too much. newOrUpdateTask, you don’t want to rely on a single function for such different actions as creating and updating objects. You're smashing in half of the CRUD convention in a single function.

Good functions should do one thing only, strive to keep your functions pure when you can. This reminds me of another quote from Clean Code:

I like my code to be elegant and efficient. The logic should be straightforward to make it hard for bugs to hide, the dependencies minimal to ease maintenance, error handling complete according to an articulated strategy, and performance close to optimal so as not to tempt people to make the code messy with unprincipled optimizations. Clean code does one thing well.

Bjarne Stroustrup, inventor of C++

Clean code does one thing well.

This is a principle that repeats across software engineering, think of DRY (Don't Repeat Yourself), it's basically another way of saying don't do something more than once. [https://blog.ordehi.dev/posts/one-thing](I posted a similar sentiment recently), but there are great resources online that go into more detail about this. Check out Curly's Law by Coding Horror if you want to look further into it.

But back to our very own coding horror. Even if newOrUpdateTask did only one thing, newTask isn’t quite clear about the intent. Sure, you can say that a new task is one that you create now so a function called that probably means it creates a new task, but your code shouldn’t require these mental gymnastics to communicate intent.

We know that it creates a task, but what kind of task? Task can stand for a lot of things, especially in software engineering, so calling it createTask while slightly better, doesn't convey enough meaning.

This code comes from a todo list app, so the tasks being created are todos. I think a good name for the function, then, would be createTodo simple, clear, clean.

You might think that adding a comment to the function is a good compromise between an intent-revealing name, and maintainability. Here's what Uncle Bob thinks:

The name of a variable, function, or class, should answer all the big questions. It should tell you why it exists, what it does, and how it is used. If a name requires a comment, then the name does not reveal its intent.

Robert C Martin - Clean Code

With that in mind, perhaps createTodo falls short. It doesn't quite tell me how I'm supposed to use it. To create a todo, we see that we need a list item called todoLi, we can do away with action since we're only going to create, not update an existing todo.

We could call createTodo like this then:

createTodo(todoListItem, todoId);

Now the function and its parameters communicate what it does and how its used. You can further improve this, I'm sure, but I'll leave it up to you to think about that. 😉

One solid improvement that comes to mind would be the use of types, such as TypeScript allows. I'm not going to go into detail about that this time around, but if you used types you'd get more information about the data the function expects when you're writing it in your IDE/text editor, plus you'll get shouted at by TypeScript if you pass in the wrong type.

Also, another issue with this function is that it creates a todo in the UI and it also writes to the database, that's bad! It's doing two things, and it's doing them all on its own. These processes should be managed by a manager function, so let's only create the worker function for the UI todo with this one.

Finally, let's take a stab at how the createTodo funtion would look like at this point:

const createTodo = (todoListItem, todoId) => {
  const todoDiv = todoLi.lastElementChild.previousElementSibling;
  const addBtn = todoLi.firstElementChild.nextElementSibling;
  const taskText = todoDiv.textContent;
  todoDiv.classList.add('noselect');
  todoDiv.contentEditable = 'false';

  todoLi.dataset.id = todoId;
  todoLi.dataset.status = 'new';
  todoDiv.dataset.action = 'update';
  addBtn.dataset.action = 'update';

  todoLi.parentNode.append(todoLi);
  todosList.prepend(todosTaskTemplate.cloneNode(true));
  todosList.firstElementChild.lastElementChild.previousElementSibling.focus();
};

Cut from 20+ lines to 13 lines without the formatting. Still a bit long for my taste, but it'll have to do, no pun intended, for now.

changelog

[1.0.0] - 2022-05-23

• Post main content