Method to delegate command to other methods

Question

Since I am working alone on this, I would like someone who would take a look and point out any mistakes or things I could have done better.

My questions:

1. How could I improve this big switch statement?
2. Is there any obvious beginner flaws?

The executeModeration is a method I call when a command is an moderation command. The command.parameters[0] contains the parameters a commmand needs to be executed

const executeModeration = async (command, channel) => {
 switch (command.command) {
 case "ban":
 if(command.parameters[0])
 await moderation.ban(channel, command.parameters[0], command.parameters[1] !== undefined ? command.parameters[1] : '');
 break;
 case "unban":
 if(command.parameters[0])
 await moderation.unban(channel, command.parameters[0]);
 break;
 case "timeout":
 if(command.parameters[0])
 await moderation.timeout(channel, command.parameters[0], command.parameters[1] !== undefined ? command.parameters[1] : 300, command.parameters[2] !== undefined ? command.parameters[2] : '');
 break;
 case "emoteonly":
 await moderation.emoteonly(channel);
 break;
 case "emoteonlyoff":
 await moderation.emoteonlyoff(channel);
 break;
 case "followersonly":
 await moderation.followersonly(channel,command.parameters[0] !== undefined ? command.parameters[0] : 30);
 break;
 case "followersonlyoff":
 await moderation.followersonlyoff(channel);
 break;
 case "r9kbeta":
 await moderation.r9kbeta(channel);
 break;
 case "r9kbetaoff":
 await moderation.r9kbetaoff(channel);
 break;
 case "slow":
 await moderation.slow(channel, command.parameters[0] !== undefined ? command.parameters[0] : 30);
 break;
 case "slowoff":
 await moderation.slowoff(channel);
 break;
 case "subscribers":
 await moderation.subscribers(channel);
 break;
 case "subscribersoff":
 await moderation.subscribersoff(channel);
 break;
 case "mod":
 if(command.parameters[0])
 await moderation.mod(channel, command.parameters[0]);
 break;
 case "unmod":
 if(command.parameters[0])
 await moderation.unmod(channel, command.parameters[0]);
 break;
 case "vip":
 if(command.parameters[0])
 await moderation.vip(channel, command.parameters[0]);
 break;
 case "unvip":
 if(command.parameters[0])
 await moderation.unvip(channel, command.parameters[0]);
 break;
 case "clear":
 await moderation.clear(channel);
 break;
 case "host":
 if(command.parameters[0])
 await moderation.host(channel, command.parameters[0]);
 break;
 case "unhost":
 await moderation.unhost(channel);
 break;
 case "commercial":
 if(command.parameters[0])
 await moderation.commercial(channel, command.parameters[0]);
 break;
 }

• Patrick McElhaney: refactoring.guru/smells/switch-statements

Answer 1

Spread Similar to Mohammed's answer, but without mutation and with spread syntax, which I find easier to read than .apply, you can do something like:

const executeModeration = async (command, channel) => {
 await moderation[command.command](...command.parameters);
}

Default parameters

You use a number of conditional operators when calling functions, eg:

.ban(
 channel,
 command.parameters[0],
 command.parameters[1] !== undefined ? command.parameters[1] : ''
);

.timeout(
 channel,
 command.parameters[0],
 command.parameters[1] !== undefined ? command.parameters[1] : 300,
 command.parameters[2] !== undefined ? command.parameters[2] : ''
);

If possible, it would be good to define the methods such that they assign default parameters, rather than having to pass the defaults from up above. For example, for the timeout method, you could define it like:

timeout(arg1, arg2 = 300, arg3 = '') {
}

and then simply pass the spread parameters when calling timeout from the script in your question, and the default parameters will take care of the rest.

If you cannot modify the moderation object's methods, then a DRY way to create these default parameters would be to make an object indexed by command, whose values are functions which need default parameters and call the moderation method. For example, for .ban and .timeout:

const executeModeration = ({ command, parameters }, channel) => {
 const fnsWithDefaultParams = {
 ban(arg1, arg2 = '') {
 return moderation.ban(channel, arg1, arg2);
 },
 timeout(arg1, arg2 = 300, arg3 = '') {
 return moderation.timeout(channel, arg1, arg2, arg3);
 },
 // ...
 };
 return fnsWithDefaultParams[command]
 ? fnsWithDefaultParams[command](...parameters)
 : moderation[command](...parameters)
};

Note that there's now no need for an async function if the function explicitly returns a Promise, like above.

• Džuris: The original function did not return anything and just ended after the promise, so the async/await were not needed at all in the first place.
• Patrick McElhaney: In ES2020 you can use the null colescing operator. command.parameters[1] ?? ‘’
• CertainPerformance: @Džuris The original function awaited calls to moderation, so it returned a Promise which resolved only after the call was done. If it returned all the Promises instead, it wouldn't need to be async, yep
• CertainPerformance: @PatrickMcElhaney That would permit null as a value too, though, and could not be done inside the parameter list
• Džuris: @CertainPerformance the original function did not return anyhting, it just awaited before exiting.

Answer 2

I'm not a JavaScipt programmer, so my suggestions are limited

Is there any obvious beginner flaws?

Using a large case statement like this is a beginners flaw, because it is difficult to maintain (expansion and contraction are difficult) and performance could be improved. In C++ I would use a map of keywords to functions and I would do something similar in C or other languages. Indexing into an array or some other table lookup mechanism is faster than the switch/case logic, and it is easier to add keyword function pairs to a data structure.

Answer 3

My answers will comport several steps. Some steps won't be present anymore in the final answer, but I mention them because you might find interesting to follow that in other contexts.

Multiple usage of the same reference to command.parameters[0], command.parameters[1], ...

You are using command.parameters[0] literally everywhere. If what you're writing look like repetition, it's a good hint that there is something to factorize.

So let's start by removing that.

const executeModeration = async (command, channel) => {
 const [param0, param1, param2] = command.parameters;
 switch (command.command) {
 case "ban":
 if (param0)
 await moderation.ban(channel, param0, param1 !== undefined ? param1 : '');
 break;
 case "unban":
 if (param0)
 await moderation.unban(channel, param0);
 break;
 case "timeout":
 if (param0)
 await moderation.timeout(channel, param0, param1 !== undefined ? param1 : 300, param2 !== undefined ? param2 : '');
 break;
 case "emoteonly":
 await moderation.emoteonly(channel);
 break;
 case "emoteonlyoff":
 await moderation.emoteonlyoff(channel);
 break;
 case "followersonly":
 await moderation.followersonly(channel, param0 !== undefined ? param0 : 30);
 break;
 case "followersonlyoff":
 await moderation.followersonlyoff(channel);
 break;
 case "r9kbeta":
 await moderation.r9kbeta(channel);
 break;
 case "r9kbetaoff":
 await moderation.r9kbetaoff(channel);
 break;
 case "slow":
 await moderation.slow(channel, param0 !== undefined ? param0 : 30);
 break;
 case "slowoff":
 await moderation.slowoff(channel);
 break;
 case "subscribers":
 await moderation.subscribers(channel);
 break;
 case "subscribersoff":
 await moderation.subscribersoff(channel);
 break;
 case "mod":
 if (param0)
 await moderation.mod(channel, param0);
 break;
 case "unmod":
 if (param0)
 await moderation.unmod(channel, param0);
 break;
 case "vip":
 if (param0)
 await moderation.vip(channel, param0);
 break;
 case "unvip":
 if (param0)
 await moderation.unvip(channel, param0);
 break;
 case "clear":
 await moderation.clear(channel);
 break;
 case "host":
 if (param0)
 await moderation.host(channel, param0);
 break;
 case "unhost":
 await moderation.unhost(channel);
 break;
 case "commercial":
 if (param0)
 await moderation.commercial(channel, param0);
 break;
 }
}

Repetition of pattern variable !== undefined ? variable : value

Still another kind of repetition:

variable !== undefined ? variable : value and especially variable !== undefined ? variable : '' is also used everywhere...

Let's get ride of that.

const getOrDefault = (element, def) => element !== undefined ? element : def;
const getOrEmpty = (element) => getOrDefault(element, '');
const executeModeration = async (command, channel) => {
 const [param0, param1, param2] = command.parameters;
 switch (command.command) {
 case "ban":
 if (param0)
 await moderation.ban(channel, param0, getOrEmpty(param1));
 break;
 case "unban":
 if (param0)
 await moderation.unban(channel, param0);
 break;
 case "timeout":
 if (param0)
 await moderation.timeout(channel, param0, getOrDefault(param1,300), getOrEmpty(param2));
 break;
 case "emoteonly":
 await moderation.emoteonly(channel);
 break;
 case "emoteonlyoff":
 await moderation.emoteonlyoff(channel);
 break;
 case "followersonly":
 await moderation.followersonly(channel, getOrDefault(param0, 30));
 break;
 case "followersonlyoff":
 await moderation.followersonlyoff(channel);
 break;
 case "r9kbeta":
 await moderation.r9kbeta(channel);
 break;
 case "r9kbetaoff":
 await moderation.r9kbetaoff(channel);
 break;
 case "slow":
 await moderation.slow(channel, getOrDefault(param0, 30));
 break;
 case "slowoff":
 await moderation.slowoff(channel);
 break;
 case "subscribers":
 await moderation.subscribers(channel);
 break;
 case "subscribersoff":
 await moderation.subscribersoff(channel);
 break;
 case "mod":
 if (param0)
 await moderation.mod(channel, param0);
 break;
 case "unmod":
 if (param0)
 await moderation.unmod(channel, param0);
 break;
 case "vip":
 if (param0)
 await moderation.vip(channel, param0);
 break;
 case "unvip":
 if (param0)
 await moderation.unvip(channel, param0);
 break;
 case "clear":
 await moderation.clear(channel);
 break;
 case "host":
 if (param0)
 await moderation.host(channel, param0);
 break;
 case "unhost":
 await moderation.unhost(channel);
 break;
 case "commercial":
 if (param0)
 await moderation.commercial(channel, param0);
 break;
 }
}

Note that, while we still have the switch/case, and timeout line seems much more readable.

Let's get ride of the switch/case

We can now replace the switch/case with an object of actions.

const getOrDefault = (element, def) => element !== undefined ? element : def;
const getOrEmpty = (element) => getOrDefault(element, '');
const moderationCommandActions = {
 ban: async (channel, nick, param2) => {
 if (nick) {
 await moderation.ban(channel, nick, getOrEmpty(param2))
 }
 },
 unban: async (channel, nick) => {
 if (nick) {
 await moderation.unban(channel, nick);
 }
 },
 timeout: async (channel, nick, value, param2) => {
 if (nick) {
 await moderation.timeout(channel, nick, getOrDefault(value, 300), getOrEmpty(param2));
 }
 },
 emoteonly: async (channel) => await moderation.emoteonly(channel),
 emoteonlyoff: async (channel) => await moderation.emoteonlyoff(channel),
 [...],
};
const executeModeration = async (command, channel) => {
 const action = moderationCommandActions[command.command];
 if (action) {
 await action(channel, ...command.parameters);
 }
}

Note that allowed me to put some name like nick instead of param0 (but as I don't know your API, I'm not sure it's the correct meaning). The best thing here is to get ride of paramX names and use descriptive names.

Finally, getOrEmpty and getOrDefault are now only used to check action params, thus can be replaced by default params in the functions declarations.

Replacing getOrEmpty and getOrDefault

const moderationCommandActions = {
 ban: async (channel, nick, param2='') => {
 if (nick) {
 await moderation.ban(channel, nick, param2)
 }
 },
 unban: async (channel, nick) => {
 if (nick) {
 await moderation.unban(channel, nick);
 }
 },
 timeout: async (channel, nick, value=300, param2='') => {
 if (nick) {
 await moderation.timeout(channel, nick, value, (param2));
 }
 },
 emoteonly: async (channel) => await moderation.emoteonly(channel),
 emoteonlyoff: async (channel) => await moderation.emoteonlyoff(channel),
 [...],
};
const executeModeration = async (command, channel) => {
 const action = moderationCommandActions[command.command];
 if (action) {
 await action(channel, ...command.parameters);
 }
}

Note how the the timeout implementation is now much more readable than the original one.

Using destructuring assignement

You can finally replace:

const executeModeration = async (command, channel) => {
 const action = moderationCommandActions[command.command];
 if (action) {
 await action(channel, ...command.parameters);
 }
}

by

const executeModeration = async ({ command, parameters }, channel) => {
 const action = moderationCommandActions[command];
 if (action) {
 await action(channel, ...parameters);
 }
}

but be carrefull, if you need to check if the first parameter of the function is not null, you can't do that, and you are forced to do:

const executeModeration = async (command, channel) => {
 if (command) {
 const action = moderationCommandActions[command.command];
 if (action) {
 await action(channel, ...command.parameters);
 }
 }
}

Answer 4

You can use Function.prototype.apply(), your code can be like below:-

const executeModeration = async (command, channel) => {
 command.parameters.unshift(channel);
 await moderation[command.command].apply(null, command.parameters);
}

you can handle ternary operators logic inside corresponding method.

Answer 5

Like Mohammed suggested, Function.apply() could be used to replace the switch statement.

Variable naming

If the first argument ‘command‘ is an object that contains a string at key ‘command‘ then a better name would be in line for readability- e.g. ‘options‘.

Argument use

Because Ecmascript-2015 (A.K.A. ES-6) features like arrow functions are used (as well as ecmascript-2017 (A.K.A. ES-8) features like async/await) the arguments can be unpacked with destructuring assignment - e.g. instead of:

 const executeModeration = async (command, channel) => {

Use some like this:

const executeModeration = async ({ command, parameters}, channel) => {

That way command refers to the method to run instead of an object with the command and parameters.

Answer 6

This solution may not be appropriate for your project depending on how else commands are used in your codebase, or if the commands are provided by an external library, among other things. But based on the snippet you give here, it might be a more suitable way to structure your code.

Let's start by reviewing two key elements of your code here: commands and the moderation variable.

• There are several different variants of commands, specified by the command.command field. Commands can also include additional data, specified in the command.parameters array, which depend on which variant the command is. For example, the "unban" command contains one piece of data: the user to unban. On the other hand, the "ban" command contains two pieces of data, the user to ban and a string (which I'm going to assume states the reason the user was banned). Note that rewriting the code like this would make it easier to see the meaning each parameter has:

 case "ban": {
 const user = command.parameters[0];
 if(user) {
 const reason = command.parameters[1];
 await moderation.ban(channel, user, reason !== undefined ? reason : '');
 }
 }
 break;

• moderation is an object with several methods (functions) -- specifically, one method for each variant of command, which takes that command's particular parameters as arguments (in addition to the channel argument). For example, since "ban" is a command that includes a user parameter and a reason parameter, moderation has a method called ban that takes a user argument and a reason argument (and a channel argument).

The reason I spell this out is because the correspondence between the variants of commands and the methods of the moderation object resembles the Visitor Pattern from languages like Java and C#. If you want to see how it works in those languages, here is a link, but it mentions some things that aren't relevant in JavaScript, so I'll just mention the necessary bits here.

The moderation object plays the Visitor role of the Visitor Pattern, and it already closely matches Visitor implementation as is. The commands play the Visited (or Element) role of the Visitor Pattern, but aren't currently implemented like they are in the pattern. We want to make it so that command objects each have a method (which we'll call execute) that takes a moderation object and calls the moderation method associated with the command. That is, if command is a "ban" command, then command.execute(moderation) should call moderation.ban(command.user, command.reason). Of course, we also have to handle the channel argument and using a default reason if none is provided, but that is the basic idea.

Class-based option

To implement this functionality, we can use JavaScript classes to mirror the class-based implementation of Java/C#. For example, "ban" would be implemented as:

class Ban {
 constructor(user, reason) {
 this.user = user;
 this.reason = reason;
 }
 
 execute(channel, moderation) {
 if (this.user) {
 moderation.ban(channel, this.user, this.reason !== undefined ? this.reason : '');
 }
 }
}

A "ban" command object would be created like new Ban("YawarRaza7349", "for being confusing") rather than creating an object with a command field and a parameters array field.

You would create a class like this for each variant of command, so it does require more code, but it better fits an object-oriented style that some find preferable.

Let's finally go back to the executeModeration code. We replace the code for the "ban" case with:

 case "ban":
 await command.execute(channel, moderation);
 break;

You'll notice that every case ends up being implemented like that; the different implementations among the cases have been moved to the execute methods of their respective classes. Thus, we can remove the switch statement and implement executeModeration as:

const executeModeration = async (command, channel) => {
 await command.execute(channel, moderation);
}

The switch statement was originally needed to call a different method for each variant of command. Now we instead do this by associating each variant with a different execute method, meaning that calling the execute method on a command runs a different implementation of execute for each variant.

Closure option

If the only thing you ever need to do with these commands is execute them, then we can use closures instead of classes, as follows:

const ban = (user, reason) =>
 (channel, moderation) => {
 if (user) {
 moderation.ban(channel, user, reason !== undefined ? reason : '');
 }
 };

To create an command object, we call ban("YawarRaza7349", "for being confusing") (no new keyword). And we remove the .execute from executeModeration:

const executeModeration = async (command, channel) => {
 await command(channel, moderation);
}

(Don't mix and match the class-based option and closure option, pick one or the other.)