Command handlers in node.js

Question

Description

This javascript code uses tmi.js to listen for chat messages in a Twitch chat and perform certain actions depending on their content. In the sample code provided, if the message starts with !add, !taskadd, !addtask or !taska it will call the addTask command handler which tries to add the task to a list of tasks. If the user who typed the message has a task already, it will not allow that and if the task is empty it will respond with an informative message.

const options = {
  options: { debug: true },
  connection: {
    reconnect: true,
    secure: true
  },
  identity: {
    username: TWITCH_BOT_USERNAME,
    password: TWITCH_BOT_PASSWORD
  },
  channels: [TWITCH_CHANNEL]
};

const client = new Client(options);
client.connect().catch(console.error);

const extractCommandAndMessage = (message) => {
  const index = message.trim().indexOf(' ');
  if (index === -1) {
    return { command: message, message: '' };
  }

  return {
    command: message.trim().substring(0, index),
    message: message.substring(index + 1)
  };
};

const addTaskCommands = ['!add', '!taskadd', '!addtask', '!taska'];

const twitchMessageHandler = async (channel, user, message, self) => {
  if (self) return;

  const { command, message: text } = extractCommandAndMessage(message);

  if (!command) return;
  const lowerCaseCommand = command.toLowerCase();

  const username = user['display-name'];

  // Tasks
  if (addTaskCommands.includes(lowerCaseCommand)) {
    addTask(channel, text, username, client);
  }

  if (editTaskCommands.includes(lowerCaseCommand)) {
    editTask(channel, username, client, text);
  }

  if (completeTaskCommands.includes(lowerCaseCommand)) {
    completeTask(channel, username, client);
  }
}

const tasks = {};
const addTask = (channel, task, username, client) => {
  if (!task) {
    client.say(channel, 'Try adding a task after the command.');
    return;
  }

  if (username in tasks) {
    client.say(channel, 'You need to !complete or !edit your current task.');
    return;
  }

  client.say(channel, `@${username}, adding "${task}" to the list...`);
  tasks[username] = task;
};

const editTask = (channel, username, client, task) => {
  if (!(username in tasks)) {
    client.say(channel, 'You need to add a task in order to edit it.');
    return;
  }

  if (!task) {
    client.say(channel, 'You need to include a description of your new task');
    return;
  }

  client.say(channel, `Okay ${username}, your task now says "${task}"`);
  tasks[username] = task;
};

const completeTask = (channel, username, client) => {
  if (!(username in tasks)) {
    client.say(channel, 'You need to add a task first.');
    return;
  }

  delete tasks[username];
  taskCounter++;

  if (!(username in individualcounters)) {
    individualcounters[username] = 1;
  } else {
    individualcounters[username] += 1;
  }

  const userTaskCount = individualcounters[username];

  client.say(
    channel,
    `BOOM ${username}! You completed your task! You have kicked ${userTaskCount} goal${
      userTaskCount === 1 ? '' : 's'
    } this stream!`
  );
  individualcounters[username] = userTaskCount;
};

client.on('message', twitchMessageHandler);

Specific issues

Message handler vs command handler responsibility

The addTask command handler is responsible for adding the task and responding with the client, when in reality its core and only function should be adding tasks as its name suggests. Is it a good idea to instead throw exceptions or return a Result object from the addTask function and handle them in the higher level message handler? Something more like;

const twitchMessageHandler = async (channel, user, message, self) => {
...
  if (addTaskCommands.includes(lowerCaseCommand)) {
    if (!text) {
      client.say(channel, 'Try adding a task after the command.');
    }

    try {
      addTask(channel, text, username, client);
      client.say(channel, `@${username}, adding "${text}" to the list...`);
    } catch (ex) {
      if (ex.name === 'UsernameAlreadyHasTask') {
        client.say(
          channel,
          'You need to !complete or !edit your current task.'
        );
      }
      console.log(ex)
    }
  }
...
}


const addTask = (task, username) => {
  if (!task) {
    throw NoTaskProvided();
  }

  if (username in tasks) {
    throw UsernameAlreadyHasTask();
  }
  tasks[username] = task;
};

Perhaps theres a better way to separate the concerns from here?

Persistence

It's pretty obvious that the persistence method for tasks is a simple in memory JS object. Would it make sense to introduce a repository to handle task persistence something like;

// repository/task.js
const saveTask = (user, task) => {
    // DB implementation or the in memory option
}

Conclusion

Besides those two specific questions, any other general improvements and tips would be greatly appreciated. I don't like how the twitchMessageHandler function works, but it was the simplest method to implement. I am fairly new to javascript and the learning I have been doing have been towards a functional-like method which is very new to me so there are some learning difficulties.

Answer 1

General comments

Your code is clean and you use up-to-date JavaScript:

• async
• const/let, no var
• object deconstruction

Naming is decent and you don't have any unnecessary comments in the code.

One thing that caught my eye is the "if ladder" to check which command was issued. This:

if (addTaskCommands.includes(lowerCaseCommand)) {
  addTask(channel, text, username, client);
}

if (editTaskCommands.includes(lowerCaseCommand)) {
  editTask(channel, username, client, text);
}

if (completeTaskCommands.includes(lowerCaseCommand)) {
  completeTask(channel, username, client);
}

Even if the first one, adding a task, matches, it still checks the two other ones. You could use an else if instead. This is not a big thing, but it stood out enough to comment on. Alternatively, you could build a map from keyword to command handler, look up the correct handler and invoke it. Something like this:

const addTask = (user, task) => {
  // implementation here
};

const editTask = (user, task) => {
  // implementation here
};

const completeTask = (user, task) => {
  // implementation here
};

// map command keywords to handler functions above.
const commandMap = {
    "!add": addTask,
    "!taskadd": addTask,
    "!addtask": addTask,
    "!taska": addTask,
    "!edit": editTask,
    "!taskedit": editTask,
    "!edittask": editTask,
    "!taske": editTask,
    "!complete": completeTask,
    "!taskcomplete": completeTask,
    "!completetask": completeTask,
    "!taskc": completeTask
};
const keywords = Object.keys(commandMap);

const twitchMessageHandler = async (channel, user, message, self) => {
  if (self) return;
  const { command, message: text } = extractCommandAndMessage(message);

  if (!command) return;
  if (!keywords.includes(command)) {
    client.say(channel, `Unknown command ${command}`);
    return;
  }

  const {error, result} = await commandMap[command](user, text);
  if (error) {
    client.say(channel, `Something went wrong: ${error}`);
  } else {
    client.say(channel, result);
  }

Here we first, create a mapping commandMap of all the keywords pointing to their respective command handler function. We also extract all the keywords to an array keywords using Object.keys. In the main Twitch handler, the message is parsed to split the keyword from the arguments. If the command is in the keyword list, we call it, receive our results object (see below), and answer to the Twitch chat. This has the advantage that you can add commands without touching the main message handler: just create the command handler function and add it to commandMap.

---

Command handler

Is it exceptional that users would provide invalid data, such as a missing task description? Unfortunately not... So I think a results object would be more appropriate here than try-catch with custom exceptions. Save the exceptions for truly exceptional states, like network or database issues. The result can be a simple object literal with properties error and result. If error is null (or absent), the operation was successful. Otherwise, it would contain the error description. If error is non-null, no guarantees are made that the result property exists and it should not be used. Object deconstruction makes this quite neat:

const addTask = (user, task) => {
  if (!task) {
    return {error: "Please provide a task description"}
  }
  // save the task
  return {result: `Task "${task}" saved successfully`
};

// below would be in the Twitch message handler
const {error, result} = addTask("john doe", "do the laundry");
if (error) {
  console.error(`Something went wrong: ${error}`);
} else {
  console.log(result);
}

---

Persistence

With in-memory storage, you lose all tasks when you restart the server. If this is okay for your use case, why bother with other storage? If not, you obviously need to store the tasks somewhere out-of-process. Files, SQLite, Redis, Postgresql, ... I've used Knex.js and like it. It's a query builder, not an ORM, so it doesn't introduce a lot of "magic" into the project. If you want a higher-level solution, have a look at e.g. Sequelize, an ORM.