3
\$\begingroup\$

Our programs often have to take in command line arguments to load a file from somewhere, specify the mode to run in etc. For this purpose we have developed a class to make it easier to load the arguments in and wrap some further behaviours.

The arguments are in the form -name:value with the system being able to specify multiple names if required in a config file.

This is the CommandLineArgumentsHelper class:

public class CommandLineArgumentsHelper
{
    private string[] _args;
    private CommandLineArgs _commandLineArgs;

    private string _help;

    public Settings Settings { get; }

    public string Help
    {
        get
        {
            if (!String.IsNullOrEmpty(_help))
            {
                return _help;
            }

            int nameColumnWidth = Math.Max(4, _commandLineArgs.Arguments.Max(a => a.FriendlyName.Length));
            int argumentNameColumnWidth = Math.Max(14, _commandLineArgs.Arguments.Max(a => String.Join(", ", a.Names).Length));

            _help = $"{"Name".PadRight(nameColumnWidth)}  {"Argument Names".PadRight(argumentNameColumnWidth)}  Description";
            _help += Environment.NewLine + new string('-', _help.Length);

            foreach (Argument argument in _commandLineArgs.Arguments)
            {
                _help += $"{Environment.NewLine}{argument.FriendlyName.PadRight(nameColumnWidth)}  {String.Join(", ", argument.Names).PadRight(argumentNameColumnWidth)}  {argument.Description}";
            }

            return _help;
        }
    }

    public CommandLineArgumentsHelper()
        : this(Environment.GetCommandLineArgs().Skip(1).ToArray())
    {

    }

    public CommandLineArgumentsHelper(string[] args)
    {
        if (args == null || args.Length < 1)
        {
            throw new ArgumentNullException($"The passed in '{nameof(args)}' were either null or empty!");
        }

        _args = args;

        Settings = LoadSystemSettings();
        if (Settings == null)
        {
            throw new ArgumentNullException(nameof(Settings), $"The '{nameof(Settings)}' file was unable to be loaded.");
        }

        string argsFile = Path.Combine(Settings.RootPath, "Pars", "Settings", "CommandLineArgs.xml");
        if (!File.Exists(argsFile))
        {
            throw new FileNotFoundException($"Was unable to find the file supplied by '{nameof(argsFile)}' at the location '{argsFile}'.", argsFile);
        }

        _commandLineArgs = XmlHelper.Load<CommandLineArgs>(argsFile);
        if (_commandLineArgs == null)
        {
            throw new ArgumentNullException(nameof(_commandLineArgs), $"The '{nameof(_commandLineArgs)}' file was unable to be loaded.");
        }
    }

    public string GetArgumentValue(string name, StringComparison stringComparison = StringComparison.OrdinalIgnoreCase)
    {
        if (String.IsNullOrEmpty(name))
        {
            throw new ArgumentNullException(nameof(name), "You must supply a value to the parameter " + nameof(name));
        }

        Argument argument = _commandLineArgs.Arguments.FirstOrDefault(a => a.FriendlyName.Equals(name, stringComparison)
                                                                        || a.Names.Any(n => n.Equals(name, stringComparison)));
        if (argument == null)
        {
            throw new ArgumentOutOfRangeException(nameof(name), "Was unable to find a argument in the settings file with a name of " + name);
        }

        if (String.IsNullOrEmpty(argument.Value))
        {
            argument.Value = _args.FirstOrDefault(a => argument.Names.Any(n => a.StartsWith($"-{n}:", stringComparison)));
            if (String.IsNullOrEmpty(argument.Value))
            {
                throw new ArgumentOutOfRangeException(nameof(argument.FriendlyName), "Was unable to find the relevant argument in the command line arguments.");
            }

            argument.Value = argument.Value.Substring(argument.Value.IndexOf(':') + 1);
        }

        return argument.Value;
    }

    private Settings LoadSystemSettings()
    {
        string pathArg = _args.FirstOrDefault(a => a.StartsWith("-p:", StringComparison.OrdinalIgnoreCase)
                                                || a.StartsWith("-path:", StringComparison.OrdinalIgnoreCase));
        if (String.IsNullOrEmpty(pathArg))
        {
            throw new ArgumentException($"The '{nameof(_args)}' must have a path passed in with either: 'p' or 'path'.", nameof(_args));
        }

        string path = pathArg.Substring(pathArg.IndexOf(':') + 1);
        if (!File.Exists(path))
        {
            throw new FileNotFoundException($"Was unable to find the file supplied by the '{nameof(_args)}' with the type '{pathArg.Substring(0, pathArg.IndexOf(':'))}' at the location '{path}'.", path);
        }

        return XmlHelper.Load<Settings>(path);
    }
}

Where the XmlHelper class is just a wrapper around the XmlSerializer class. The Settings class is a wrapper around an XML file used to specify system settings values. In this case it is used to load the systems root path.

The CommandLineArgs class is the class for the XML file is as follows:

[XmlRoot]
public class CommandLineArgs
{
    [XmlElement("Argument")]
    public List<Argument> Arguments { get; set; }

    public CommandLineArgs()
    {
        Arguments = new List<Argument>();
    }
}

public class Argument
{
    [XmlAttribute]
    public string FriendlyName { get; set; }

    [XmlAttribute]
    public string Description { get; set; }

    [XmlIgnore]
    public string Value { get; set; }

    [XmlElement("Name")]
    public List<string> Names { get; set; }

    public Argument()
    {
        FriendlyName = string.Empty;
        Description = string.Empty;
        Value = string.Empty;
        Names = new List<string>();
    }
}

I'd appreciate any feedback on this.

\$\endgroup\$

1 Answer 1

5
\$\begingroup\$

Just a few things:

  1. CommandLineArgumentsHelper is a bad name for an instantialble class. That is, everything in it is non-static, so it's not a 'helper' class, it's a utilitarian class. I'd call it CommandLineArgumentsParser.
  2. CommandLineArgs is an acceptable name, if you're going to abbreviate the Parser / Serializer class. Either use Args in both, or use Arguments.
  3. The args.Length < 1 would get so annoying to me, that I would never use this class. In fact, this whole class is useless to me. You should allow the user to send empty arguments, and consume them, so that I can consume it as follows:

    var parser = new CommandLineArgsParser(args);
    var file = DEFAULT_FILE;
    var arg = parser.Args.FirstOrDefault(x => x.Name == "File");
    
    if (arg != null)
    {
        file = arg.Value;
    }
    

    Instead of that, though, I have to make sure I only instance the parser if all the commands are specified. That's wrong.

    Right now this whole class is literally useless for me. If the argument doesn't exist in the passed args, you just error on it. There should be a valid state for each argument where it doesn't exist or doesn't get set, because if my command line program supports 50 switches and arguments, my users would be furious to have to type that many every time.

    Consider reworking the API to be a specialized collection, so that I can call TryGetValue and such on it. In fact, I'd consider implementing it like a Dictionary<TKey, TValue>, with the exception that TKey and TValue are string implicitly.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.