2

I have a (hopefully) simple C# question.

I am parsing arguments in a program where a file will be read from command line, I've allowed for both short and long arguments as input (so for my scenario /f and file are both valid)

The value after either of the above arguments should be the file name to be read.

What I want to do is find this file name in the array based off whichever argument is chosen and copy it in to a string while not leaving any loopholes.

I have functioning code, but I'm not really sure it's "efficient" (and secure).

Code (comments and writes removed):

if ( args.Contains("/f") || args.Contains("file"))
{
    int pos = Array.IndexOf(args, "/f");

    if (pos == -1)
        pos = Array.IndexOf(args, "file");

    if (pos > -1)
        pos++;

    inputFile = (args[pos]);

    if (File.Exists(inputFile) == false)
    {
        Environment.Exit(0);
    }
}

Is there a more efficient way to do this, perhaps using some nifty logic in the initial if statement to check which parameter is valid and then do a single check on that parameter? Using 4 ifs and 2 Array.IndexOf's seems horrible just to support 2 different ways to allow someone to say they want to input a file...

Thanks! And I'm sorry if this seems trivial or is not what SO is meant for. I just don't have any real way to get feedback on my coding practises unfortunately.

Improve this question
2
  • 3
    I suggest implementing a special class - command line parser - where you may want to include -f key support in addition to /f, help call (i.e. /?), error messages (what if I call your program as "program.exe /data c:\mydata.text"? does your program tell me that "data" key is a syntax error?) etc. Commented Feb 5, 2015 at 11:35
  • There is error checking for invalid arguments and other mismatches, with output explaining why where possible - a help function is also present. I'll take a look at some parser libraries such as the one Magos has linked, but mainly for inspiration. Just using other peoples code isn't going to teach me much :( Commented Feb 5, 2015 at 12:01

3 Answers 3

Reset to default
3

Your solution won't scale well. Imagine you have two different arguments with a short and long form. How many conditionals and index checks would that be?

You'd be better off using an existing tool (e.g. Command Line Parser Library) for argument parsing.

Improve this answer
Sign up to request clarification or add additional context in comments.

1 Comment

Yes, you're right, although this application is only meant to be very small and relatively simple, most parsers are far larger and more complex than my entire application. With the majority of arguments a large number of conditionals shouldn't be needed as the next option won't be specifically related to the previous (so a simple "a || b" would probably suffice?). In a worst case scenarion, the above code could be altered and put in to a function which is called in any such case.
0

One problem I see with the code you provided is that, it will fail if the /f or file is the last argument.

If you don't want to write or use complete argument parsing code, the following code will work slightly better.

var fileArguments = new string[] { "/f", "file" };

int fileArgIndex = Array.FindIndex(args, 
    arg => fileArguments.Contains(arg.ToLowerInvariant()));

if (fileArgIndex != -1 && fileArgIndex < args.Length - 1)
{
    inputFile = args[fileArgIndex + 1];

    if (!File.Exists(inputFile))
    {
        Environment.Exit(0);
    }
}
Improve this answer

1 Comment

Sorry for the delay in choosing an answer. I appreciate all the proposed answers and there are very valid points. However, this answer comes closest to answering the intent of my question, which related to efficiency and closing of loopholes in the code. While this code actually runs 6 times slower than the code I provided, it does fix a simple exception case that I missed without adding a huge amount of extra code not needed for a simple application.
0

You could write a simple argument parser for your specific need and still have support for "new" scenarios. For example, in your entry method have

// The main entry point for the application.
[STAThread]
static void Main(string[] args)
{
    // Parse input args
    var parser = new InputArgumentsParser();
    parser.Parse(args);
    ....
}

Where your InputArgumentsParser could be something similar to

public class InputArgumentsParser
{
    private const char ArgSeparator = ':';
    private Dictionary<string[],Action<string>> ArgAction = 
        new Dictionary<string[],Action<string>>();

    public InputArgumentsParser()
    {
        // Supported actions to take, based on args
        ArgAction.Add(new[] { "/f", "/file" }, (param) => 
            Console.WriteLine(@"Received file argument '{0}'", param));
    }

    /// Parse collection, expected format is "<key>:<value>"
    public void Parse(ICollection<string> args)
    {
        if (args == null || !args.Any())
            return;

        // Iterate over arguments, extract key/value pairs
        foreach (string arg in args)
        {
            string[] parts = arg.Split(ArgSeparator);
            if (parts.Length != 2)
                continue;

            // Find related action and execute if found
            var action = ArgAction.Keys.Where(key => 
                key.Contains(parts[0].ToLowerInvariant()))
                    .Select(key => ArgAction[key]).SingleOrDefault();

            if (action != null)
                action.Invoke(parts[1]);
            else
                Console.WriteLine(@"No action for argument '{0}'", arg);
        }
    }
}

In this case /f:myfile.txt or /file:myfile.txt would spit out to console

Received file argument 'myfile.txt'

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.