Return to Answer

• 1

• 14.1k
• 3
• 40
• 101

• 14.1k
• 3
• 40
• 101

Design Issues (Major)

There are a couple of issues concerning your design.

Lack of specification

It is unclear which features should be supported by your API. This makes reviewing a bit fuzzy.

Dependencies

The parser depends on arguments already pre-parsed correctly by a shell. This limits the control you have over command line parsing.

var args = new[] {"--log", "info", "1337", "3.1415"};

Consider breaking free from the shell and take on pre-parsing yourself.

var args = "--log info 1337 3.1415"; // <- unparsed command line string

Pollution

The API mixes language structs with user-defined options.

new CommandLineOption(new[] {"-l", "--log"}

You do not want - and -- to be part of the Tags. These are delimiters in the lexing phase of your parser. By seperating lexing from parsing, you could extend the API more fluently by allowing other command line languages. For instance /log.

##Review

Exception Classes

Define a base class for all your exceptions CommandLineException. This way, you allow calling code to determine the granularity of exception handling. Since you make several custom exceptions, take advantage of storing some data on them. DuplicateRequiredCommandLineOptionException could store the duplicate option, and so on. Also provide constructors that take an inner exception.

public class DuplicateRequiredCommandLineOptionException : CommandLineException
{
 public CommandLineOption Option { get; }
 // include more constructors ..
 public DuplicateRequiredCommandLineOptionException(
 string messageCommandLineOption option) : base(message) { Option = option; }
}

CommandLineOption & CommandLineValue

You have addressed you don't want to see too many changes for legacy reasongs. I do propose to override the default Equals and GetHashCode on both classes and substitute IEqualityComparer with IEquatable. This way, you could improve your code.

 public bool Equals(CommandLineValue other)
 {
 return Option.Equals(other.Option) && Values.SequenceEqual(other.Values);
 }

CommandLineParser

You have indicated yourself you have problems parsing a flattened list to a hierarchical structure. There are common techniques for handling such situations. Have a look at Abstract Syntax Tree. You should create a syntax tree from the provided string[] args. This can be done with a Stack and Iterator. There are tons of examples online how to create an AST.

// Check if the additional values are in the right format
// ToDo: Find more elegant solution
var values = args.ToList().GetRange(i + 1, i + additionalValues).ToList();
var types = option.ParameterTypes.ToList();

The second issue is - what I called pollution before - the lack of seperation of concerns. Your API is basically a simple compiler. The link shows you it's good practice to provide the following phases when building a compiler:

• pre-processing
• lexing
• parsing
• optimizing
• pretty printing

Your API should definitely include lexing and parsing as seperate phases.

• lexing: create command line tokens and strip all the keywords and language-specific delimiters
• parsing: create an AST from the lexed tokens, then create CommandLineValue instances from the AST.

Conclusion

In the end, the quality of the API depends on a good specification covered by many unit tests. I feel you haven't established this yet.