0
\$\begingroup\$

So I'm writing a big server and I've made a simple start on console commands. I'm not sure if the way I'm doing it in is correct so I'd like to have it reviewed.

I have a class called ServiceCollectionExtensions where extension methods are defined that can be used on ServiceCollection. In there I have a function to register all console commands:

public static void RegisterConsoleCommands(this IServiceCollection serviceDescriptors)
{
    serviceDescriptors.AddTransient<ICommand, ClearCommand>();
}

In my Nordlys class in my entry point, I have the following code:

IServiceCollection serviceDescriptors = new ServiceCollection();
serviceDescriptors.AddLogging(builder => builder.AddConsole());
serviceDescriptors.AddSingleton<ConsoleCommandHandler>();
serviceDescriptors.RegisterConsoleCommands();

And at the end of the entry point I have this code to wait for user input:

while (true)
{
    string input = await System.Console.In.ReadLineAsync();

    if (input.Length > 0)
    {
        await serviceProvider.GetService<ConsoleCommandHandler>().TryHandleCommandAsync(input);
    }
}

And this is my ConsoleCommandHandler class:

public class ConsoleCommandHandler
{
    private readonly ILogger<ConsoleCommandHandler> logger;
    private readonly Dictionary<string, ICommand> commands;

    public ConsoleCommandHandler(ILogger<ConsoleCommandHandler> logger, IEnumerable<ICommand> commands)
    {
        this.logger = logger;
        this.commands = commands.ToDictionary(command => command.Command);
    }

    public async Task TryHandleCommandAsync(string input)
    {
        int spacePosition = input.IndexOf(' ');
        string header = spacePosition >= 0 ? input.Substring(0, spacePosition) : input;

        if (commands.Any(command => command.Key == header))
        {
            await commands[header].RunAsync(input.Substring(spacePosition + 1).Split(' '));
        }
        else
        {
            logger.LogWarning("'{0}' is not recognized as an internal command", header);
        }
    }
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Is this working code? You should also include entry class as there is not enough context in the currently provided post to provide a meaningful review. \$\endgroup\$
    – Nkosi
    Feb 3 '20 at 0:43
1
\$\begingroup\$

If you're not using C# 8's nullable references, you should be checking for null whenever you're supplied with input.

  • In the ConsoleCommandHandler constructor, throw ArgumentNullException if logger or commands is null.
  • In ConsoleCommandHandler.TryHandleCommandAsync, throw ArgumentNullException if input is null. Even though you're only ever passing non-empty strings from your entry point, this method is publicly visible which sends a signal that anybody could potentially use it differently in the future.

In ConsoleCommandHandler, you create a Dictionary that maps command names to ICommands. You would do this if you wanted constant-time lookup of commands from command names. But then you wrote commands.Any(command => command.Key == header) which will instead do a linear search over every key-value pair in the dictionary until it finds a matching key. This defeats the purpose of using a dictionary. Use TryGetValue to check-and-retrieve a value from a dictionary in one shot:

    if (commands.TryGetValue(header, out var command)
    {
        await command.RunAsync(input.Substring(spacePosition + 1).Split(' '));
    }
    else
    {
        logger.LogWarning("'{0}' is not recognized as an internal command", header);
    }

TryHandleCommandAsync really does two things -- it parses user input into a command + arguments, and then it finds/runs the associated command. It may be worth separating those concerns into methods. Then, if the format of those command strings ever changes then you'll know exactly where and what to change in the code.

Depending on how sophisticated your command line interface needs to be, consider a third-party library that does all the gross string parsing stuff for you. For example, do you need to support commands or arguments that contain spaces? Your implementation currently doesn't.

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.