Simple Command system

Question

Below I have coded a simple command system that passes a string to a method and checks if the first word split by (' ') is a key in the registered commands dictionary, if it is, it executes the class that is stored as the dictionary entry's value.

few examles of what is typed for commands

@mycommand hello
:ohnicecommand John
.setamount 10000

CommandHandler.cs:

using System;
using System.Collections.Generic;
namespace Sirius.Base.Game.Habbo.Commands
{
 using Commands;
 using Commands.Habbo;
 using Other.GameClients;
 internal class CommandHandler
 {
 private readonly Dictionary<string, ICommand> _commandsRegistered;
 internal CommandHandler()
 {
 _commandsRegistered = new Dictionary<string, ICommand>();
 RegisterCommands();
 }
 private void RegisterCommands()
 {
 RegisterCommand(":info", new InfoCommand());
 }
 public bool IsCommandInput(Player player, string input)
 {
 ICommand command;
 var cmdString = input.Split(' ')[0];
 if (!_commandsRegistered.TryGetValue(cmdString, out command))
 {
 return false;
 }
 if (command.PermissionRequired != "" && !player.GetPlayerData().GetPermissions().HasCommand(command.PermissionRequired))
 {
 return false;
 }
 if (command.NumberOfParameters < input.Split(' ').Length)
 {
 player.SendWhisper("Invalid command syntax -- " + cmdString + " " + command.CommandParameters);
 return true;
 }
 DateTime lastExecuted;
 if (player.GetPlayerData()._commandCooldowns.TryGetValue(cmdString, out lastExecuted))
 {
 var commandSpan = DateTime.Now - lastExecuted;
 if (command.CommandCooldown && commandSpan.TotalSeconds < (command.CommandCooldownMs / 1000))
 {
 player.SendNotification("You're cooling down from this command! [" + commandSpan.TotalSeconds + "/" + (command.CommandCooldownMs / 1000) + "]");
 return true;
 }
 }
 else
 {
 player.GetPlayerData()._commandCooldowns.Add(cmdString, DateTime.Now);
 }
 command.ProcessCommand(player);
 return true;
 }
 private void RegisterCommand(string commandName, ICommand command)
 {
 _commandsRegistered.Add(commandName, command);
 }
 }
}

ICommand:

namespace Sirius.Base.Game.Habbo.Commands.Commands
{
 using Other.GameClients;
 internal interface ICommand
 {
 void ProcessCommand(Player player);
 string PermissionRequired { get; }
 int NumberOfParameters { get; }
 string CommandParameters { get; }
 string CommandDescription { get; }
 bool CommandCooldown { get; }
 int CommandCooldownMs { get; }
 }
}

InfoCommand (just 1 of many commands)

using System;
using System.Text;
namespace Sirius.Base.Game.Habbo.Commands.Commands.Habbo
{
 using Other.Communication.Packets.Outgoing.Notifications;
 using Other.GameClients;
 internal sealed class InfoCommand : ICommand
 {
 public string PermissionRequired => "";
 public int NumberOfParameters => 0;
 public string CommandParameters => "";
 public string CommandDescription => "Lets you view information about the server.";
 public bool CommandCooldown => true;
 public int CommandCooldownMs => 5000;
 public void ProcessCommand(Player player)
 {
 // do what ever you want the command to do here...
 }
 }
}

• Der Kommissar: You should, at the very least, include the body of ProcessCommand for the InfoCommand to help reviewers find more potential for optimization.
• Der Kommissar: Also, what does a command with CommandParameters set look like?

Answer 1

The biggest issue I see is you have to manually register each and every command. Why bother? Why not just get a list of all commands that implement ICommand through Reflection? Speed shouldn't be a huge problem because I assume you do this on some sort of start-up.

var baseCommandInterface = typeof(ICommand);
var commands = AppDomain.CurrentDomain.GetAssemblies()
 .SelectMany(s => s.GetTypes())
 .Where(p => baseCommandInterface.IsAssignableFrom(p));

Now just enumerate commands and cast and operate on them. Life should be a lot easier this way.

---

Next, why bother with a bool and int for cool downs? Just use an int?, if it's null then there's no cool down, if its >0 then that's the cool down time. Pretty simple.

---

You use the term Command way too many times for my tastes.

internal interface ICommand
{
 void Process(Player player);
 string PermissionRequired { get; }
 int NumberOfParameters { get; }
 string Parameters { get; }
 string Description { get; }
 int? CooldownMs { get; }
}

Even then, NumberOfParameters is truly unnecessary. I'd bet there is a separator of some sort between commands in the Parameters string, which really shouldn't be a string, but should be something like the following:

public class CommandParameter
{
 public string Name { get; set; }
 public Type Type { get; set; }
 public string Description { get; set; }
}

Then Parameters becomes an IEnumerable<CommandParameter> which means you can use Parameters.Count in place of NumberOfParameters. This also means you can extrapolate further and define an abstract class for this, and add a CanProcess(Player player) method:

public abstract class BaseCommand : ICommand
{
 private List<CommandParameter> _parameters = new List<CommandParameter>();
 public IEnumerable<CommandParameter> Parameters => _parameters;
 public int? Cooldown { get; protected set; }
 public string Description { get; protected set; }
 public string PermissionRequired { get; protected set; }
 public abstract void ProcessCommand(Player player);
 public bool CanProcess(Player player)
 {
 // Check cool down, permissions, ban list, etc.
 }
}

Then in an actual command:

public class InfoCommand : BaseCommand
{
 public InfoCommand()
 {
 Cooldown = 5000;
 Description = "Lets you view information about the server.";
 }
 public override void ProcessCommand(Player player)
 {
 if (!CanProcess(player))
 {
 // I'm throwing an exception, but you could just `return` and send a message or whatever
 throw new InvalidOperationException("You have either exceeded the cool down, do not have permissions, or are banned. Please try again later.");
 }
 // Process our command here
 }
}

Then you created your own system that requires boilerplate code, and extracted that boilerplate code away.

• Ashkru: Thank you. I am not sure what you mean by your first comment about loading the commands, where do I put the custom input that triggers each individual command?
• Der Kommissar: @Ashkru Give each command a Trigger property that is the :info thing or whatever.
• Chandler Turner: @Ashkru Is this the answer? I've noticed that you ask a lot of questions yet never choose to pick a selected answer.