6
\$\begingroup\$

A little while ago I was fiddling with and came up with this small WPF application that I can launch with command-line arguments.

Not only this was my first experiment with DI, it was also my first experiment with parsing command-line arguments. The result works nicely, but looking at the code today I see a number of things I would have done differently.

What would you have done differently?

Looking mainly for feedback on readability and maintainability.


I have an interface/class that exposes getters for all possible command-line arguments; the implementation's constructor uses Ninject's InjectAttribute to specify the required ICommandLineArg implementation to receive:

public class AutoBuildCommandLineHelper : IAutoBuildCommandLineArgs
{
    public ICommandLineArg QuietInterfaceArgument { get; private set; }
    public ICommandLineArg BuildServerArgument { get; private set; }
    public ICommandLineArg CompletionMessageArgument { get; private set; }
    public ICommandLineArg LoggingDisabledArgument { get; private set; }
    public ICommandLineArg FailureNotificationOnlyArgument { get; private set; }

    public AutoBuildCommandLineHelper([QuietInterfaceArg] ICommandLineArg quietInterfaceArg,
                                       [BuildServerArg] ICommandLineArg buildServerArg,
                                       [NoLogArg] ICommandLineArg noLogArg,
                                       [MessageArg] ICommandLineArg msgArg,
                                       [FailureNotifyArg] ICommandLineArg failArg)
    {
        QuietInterfaceArgument = quietInterfaceArg;
        BuildServerArgument = buildServerArg;
        LoggingDisabledArgument = noLogArg;
        CompletionMessageArgument = msgArg;
        FailureNotificationOnlyArgument = failArg;
    }
}

Here are a couple ICommandLineArg implementations:

public class QuietInterfaceArgument : CommandLineArgumentBase
{
    public QuietInterfaceArgument(string[] args)
        : base(args, "q", "quiet") { }
}

public class NoLoggingArgument : CommandLineArgumentBase
{
    public NoLoggingArgument(string[] args)
        : base(args, "n", "nolog") { }
}

As you see there's really nothing going on here - this is the base class:

public abstract class CommandLineArgumentBase : ICommandLineArg
{
    private readonly string[] _args;
    private const char ParamSpecifier = ':';
    private readonly char[] _switches = { '-', '/' };

    public string Name { get; private set; }
    public string Alias { get; private set; }
    public bool MustHaveParameter { get; private set; }

    protected CommandLineArgumentBase(string[] args, string name, string alias) 
        : this(args, name, alias, false) { }

    protected CommandLineArgumentBase(string[] args, string name, string alias, bool mustHaveParam)
    {
        _args = args;
        Name = name;
        Alias = alias;
        MustHaveParameter = mustHaveParam;
    }

    public virtual string ParameterValue()
    {
        if (!IsSpecified()) throw new InvalidOperationException();
        var arg = GetArgument();

        var values = arg.Split(ParamSpecifier);
        if (MustHaveParameter && values.Length == 1) throw new InvalidOperationException();

        return values[1].TrimStart('"').TrimEnd('"');
    }

    private string GetArgument()
    {
        if (!IsSpecified()) throw new InvalidOperationException();
        return _args.Single(arg => new[] {Name, Alias}.Contains(TrimArgument(arg).Split(ParamSpecifier)[0]));
    }

    private string TrimArgument(string arg)
    {
        return arg.TrimStart(_switches).TrimEnd(_switches);
    }

    public bool IsSpecified()
    {
        return _args.Select(arg => arg.Split(ParamSpecifier)[0])
                    .Select(TrimArgument)
                    .Contains(Name) || _args.Any(arg => TrimArgument(arg).StartsWith(Name));
    }
}

This "thing" is used in Program.cs / Main(), like this:

        if (argsHelper.LoggingDisabledArgument.IsSpecified()) 
            logProvider.DisableLogging();

        if (argsHelper.QuietInterfaceArgument.IsSpecified() 
         || argsHelper.BuildServerArgument.IsSpecified())
        {
            // running with -quiet or -build command-line switches: just execute and exit.
            autoBuildApp.Execute();
        }
        else
        {
            // run with UI:
            var app = new App(autoBuildApp);
            app.Run();
        }

I like that arguments are strongly-typed and all I need to do to check if they're specified, is to call .IsSpecified() - the flipside is that to achieve this I'm passing the args string everywhere, so it gets "parsed" every time.

Thoughts?

\$\endgroup\$
  • \$\begingroup\$ Forgive me if this is a dumb question but should the [QuietInterfaceArg] part of [QuietInterfaceArg] ICommandLineArg quietInterfaceArg instead be [QuietInterfaceArgument]? \$\endgroup\$ – James Khoury Nov 28 '13 at 6:37
  • 1
    \$\begingroup\$ @JamesKhoury That still wouldn't work, attributes have to derive from Attribute. Which means that there has to be a type that the OP forgot to include, probably called QuietInterfaceArgAttribute. \$\endgroup\$ – svick Nov 28 '13 at 12:44
  • \$\begingroup\$ Indeed, I didn't bother including the Ninject InjectAttribute-derived classes. Should I add them in? \$\endgroup\$ – Mathieu Guindon Nov 28 '13 at 13:39
  • \$\begingroup\$ @retailcoder only for those like me who are not versed in ninject-ness \$\endgroup\$ – James Khoury Nov 28 '13 at 23:04
5
\$\begingroup\$

Style

  1. I don't like the name AutoBuildCommandLineHelper for a class which implements an interface like IAutoBuildCommandLineArgs. I would have named it AutoBuildCommandLineArgs
  2. You have made ParamSpecifier a named constant but not the quotes you strip of the parameter value.

Possible Bugs

  1. If I read IsSpecified() correctly then it accepts arguments starting with Name. Given that Name seems to be the one letter abbreviation this means if I specify an invalid argument which happens to start with a letter of a valid argument then IsSpecified() will return true.
  2. If you call ParameterValue() on an argument which does not have (i.e. doesn't require) a parameter it will throw an IndexOutOfRangeException as values will only have 1 entry

Design

When you want to add a new command line option you have to do 5 or 6 things:

  1. Add a new class representing the new option
  2. Register that class with the IoC container (this could possibly happen automagically depending on the container used)
  3. Add a new property to IAutoBuildCommandLineArgs
  4. Add a new property to AutoBuildCommandLineHelper
  5. Add a new property to AutoBuildCommandLineHelper constructor
  6. Add a new assignment in AutoBuildCommandLineHelper constructor to copy parameter into property

Seems a bit involved. I hacked together an alternative version which should fit DI just nicely:

public enum Option
{
    NoLog,
    Quiet,
    Server,
    LogLevel,
    Help,
}

public interface ICommandLineOptions
{
    bool Has(Option option);
    string ValueOf(Option option);
}

public class CommandLineOptions : ICommandLineOptions
{
    private class CommandLineOption
    {
        public CommandLineOption(Option type, string shortKey, string longKey, string description, bool requiresParam)
        {
            Type = type;
            Short = shortKey;
            Long = longKey;
            Description = description;
            RequiresParam = requiresParam; 
        }

        public readonly Option Type;
        public readonly string Short;
        public readonly string Long;
        public readonly string Description;
        public readonly bool RequiresParam;
    }

    private static readonly List<CommandLineOption> _AllOptions = 
        new List<CommandLineOption>
        {
            new CommandLineOption(Option.NoLog,     "n", "nolog",    "Don't log",          false),
            new CommandLineOption(Option.Quiet,     "q", "quiet",    "Run quietly",        false),
            new CommandLineOption(Option.Server,    "s", "server",   "Run in server mode", false),
            new CommandLineOption(Option.LogLevel, "ll", "loglevel", "Set log level",      true),
            new CommandLineOption(Option.Help,      "?", "help",     "Print help",         false),
        };

    private static const char _ParamSpecifier = ':';
    private static readonly char[] _Switches = { '-', '/' };
    private static readonly char[] _Quotes = { '"', '\'' };

    private Dictionary<Option, string> _AvailableOptions = new Dictionary<Option, string>();

    public CommandLineOptions(string[] args)
    {
        foreach (var arg in args.Select(a => a.Trim(_Switches)))
        {
            var parts = arg.Split(new char[] {_ParamSpecifier }, 2, StringSplitOptions.RemoveEmptyEntries);

            var option = _AllOptions.FirstOrDefault(o => o.Short == parts.First().ToLower() || o.Long == parts.First().ToLower());
            if (option == null)
                throw new ArgumentException(string.Format("Unknown command line option {0}", arg));
            if (option.RequiresParam && parts.Length == 1)
                throw new ArgumentException(string.Format("Command line option {0} is missing required parameter", arg));
            _AvailableOptions[option.Type] = option.RequiresParam ? parts.Last().Trim(_Quotes) : "";
        }
    }

    public bool Has(Option option)
    {
        return _AvailableOptions.ContainsKey(option);
    }

    public string ValueOf(Option option)
    {
        string value;
        if (!_AvailableOptions.TryGetValue(option, out value))
            throw new ArgumentException(string.Format("Option {0} not present", option));
        return value;
    }
}

When you want to add a new option you have to do 2 things:

  1. Add a new entry to the enum
  2. Add a new entry to the _AllOptions dictionary

From a DI perspective anything which needs access to the options just depends on ICommandLineOptions.

If you wanted to get real fancy you could make CommandLineOption public, add an interface ICommandLineOption for it and make CommandLineOptions take a dependency on a collection of ICommandLineOptions. Then you could register all the options you want to support in the container rather than hard coding them in CommandLineOptions.

| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

Default command line switches

It is recommended that tools specify the following standard switches. You should always define these. Also, on error, output the error and the help text.

- help (--help -h -?)
- about (--version -!)

Maintainability

The fact your arguments include the keywords used for parsing is a breach of Single Responsibility. I would extract all lexing in a seperate class behind some interface. This way, you can make different lexers for Posix, DOS, Windows, Google style.


Readability

Get rid of the postfix -Argument in your property names. It's redundant and verbose.


Command line parsing

Command line parsing is an art. A good command line parser is able to parse both readable and administrative command lines, different styles, grammars, literals, escape sequences.

There are different formatting styles.

Google dev style

tool --get-logs --in c:\data\in\ -quiet

POSIX short style

tool-gq-ic:\data\in\

Windows/DOS style

tool /gq /i c:\data\in\

Design Restrictions

I don't know your intend, but if you want to comply to at least the aforementioned command line styles, you have restricted yourself.

You have hardcoded your delimiters. This means you are only compliant to your own standards, not universal ones. For instance, other ParamSpecifier that could have been allowed are '=', ',' and even a blank space in the right context. Other _switches include the Posix long-option '--'.

 private const char ParamSpecifier = ':';
 private readonly char[] _switches = { '-', '/' };

Command switches could have 0..* arguments. However, you limit this to 0..1.

public bool MustHaveParameter { get; private set; }

You perform a naive switch-value split. This does not take into account escaped delimiters or file paths.

var values = arg.Split(ParamSpecifier);

- -a:literal\:value
- -a:literal":"value
- -a:c:\temp\

Quoted literals should not be restricted to the start and end of a string, but could also be inline.

return values[1].TrimStart('"').TrimEnd('"');


| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Blast from the past! Nice review. I've since made it a habit to download a nuget package for handling command-line args whenever I need them =) \$\endgroup\$ – Mathieu Guindon Jun 7 '19 at 19:26
  • \$\begingroup\$ @MathieuGuindon Good thinking not to reinvent the wheel for command line parsing. It could get you sucked into the rabbit hole :-p \$\endgroup\$ – dfhwze Jun 7 '19 at 19:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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