2

I am trying to validate the command line arguments and print an error message if there is some error.

My problem is that if the number of command line parameters is increased (currently, I only have 3), then my code would turn into spaghetti code. How can I reduce the cyclomatic complexity of the given code?

var isCmdLineWrong = false;
var Arg1 = "Undefined";
var Arg2 = "Undefined";
var Arg3 = "Undefined";

var commandArguments = Environment.GetCommandLineArgs();
if (commandArguments.Contains("-r") && arguments[commandArguments.IndexOf("-r") + 1].StartsWith("-") == false)
    Arg1 = commandArguments[commandArguments.IndexOf("-r") + 1];
else
{
    isCmdLineWrong = true;
}
if (commandArguments.Contains("-n") && commandArguments[commandArguments.IndexOf("-n") + 1].StartsWith("-") == false)
    Arg2 = commandArguments[commandArguments.IndexOf("-n") + 1];
else
{
    isCmdLineWrong = true;
}
if (commandArguments.Contains("-p") && commandArguments[commandArguments.IndexOf("-p") + 1].StartsWith("-") == false)
    Arg3 = commandArguments[commandArguments.IndexOf("-p") + 1];
else
{
    isCmdLineWrong = true;
}
if (isCmdLineWrong) Console.WriteLine("Parameters structure is inconsistent");
stakx - no longer contributing
  • 83,039
  • 20
  • 168
  • 268
Akiner Alkan
  • 6,145
  • 3
  • 32
  • 68

3 Answers3

4

I suggest extracting CommandLine class:

public static class CommandLine {
  private static String FindValue(string value) {
    var commandArguments = Environment.GetCommandLineArgs();

    int index = commandArguments.IndexOf(value);

    if (index < 0)
      return null; 
    else if (index >= commandArguments.Length - 1)
      return null; // cmd like "myRoutine.exe -p" 
    else 
      return commandArguments[index + 1];  
  } 

  static CommandLine() {
    Arg1 = FindValue("-r");
    Arg2 = FindValue("-n");
    Arg3 = FindValue("-p");
  } 

  public static String Arg1 { get; private set; }

  public static String Arg2 { get; private set; }

  public static String Arg3 { get; private set; }

  public static bool IsValid {
    get {
      return Arg1 != null && Arg2 != null && Arg3 != null;
    }
  }
}

Having this class written you can put

if (!CommandLine.IsValid) {
  Console.WriteLine("Parameters structure is inconsistent");

  return;
} 

if (CommandLine.Arg1 == "quit") {
  ...
}  
Dmitry Bychenko
  • 180,369
  • 20
  • 160
  • 215
  • 2
    Exactly what I was thinking, but why did you make it all **static** ? – Maarten Nov 30 '16 at 08:44
  • @Maarten: we have exactly *one* command line for the application, so we don't want many instances of the `CommandLine` class (like `Application`, `Environment` and similar classes) – Dmitry Bychenko Nov 30 '16 at 08:51
  • 1
    @DmitryBychenko: That is correct, but it also makes your class much harder to test or replace. May I suggest these simple changes: **1.** make the class and its members non-static, **2.** augment the ctor with a `commandLine` string parameter that will be used instead of `Environment.GetCommandLineArgs()`, and finally **3.** add a static `Instance` property or field that returns / is initialized to an instance tied to the actual command line: `public static CommandLine Instance = new CommandLine(Environment.GetCommandLineArgs())`. – stakx - no longer contributing Nov 30 '16 at 09:26
  • 1
    @stakx: you're quite right, in case of *universal/generalized solution* (e.g. parsing and building command lines for some other processes). In *this very question* (*three* arguments and a primitive `IsValid` property) a simple `static class` will be, IMHO, good enough when the elaborated implementation you've proposed is an overshoot in this very context (being a better solution in *general case*). – Dmitry Bychenko Nov 30 '16 at 10:18
  • @Dmitry: Your assessment is reasonable, especially since testing might not be the OP's main concern. Still, I thought it was worth pointing out *how* your suggested class could be made testable with only little additional effort and lines of code. (+1 btw. :-) – stakx - no longer contributing Nov 30 '16 at 10:37
  • 1
    @stakx: If I had to test (implement unit tests) such a `static class` my first thought would be to put a *shim* on `Environment.GetCommandLineArgs()` https://msdn.microsoft.com/en-us/library/hh549175(v=vs.110).aspx (I can use VS Ultimate Edition). To call *static constructor* manually `System.Runtime.CompilerServices.RuntimeHelpers.RunClassConstructor(typeof(CommandLine).TypeHandle);`. I see you reason: it's *far easier* to test the solution you've proposed (all one need is to execute the constructor) – Dmitry Bychenko Nov 30 '16 at 11:24
1

Probably the most important thing to observe in your code is that you are doing the exact same thing several times, though with different inputs "-r" and Arg1, "-n" and Arg2, "-p" and Arg3. That is, you have the following code fragment appear three times (minus my reformatting):

if (commandArguments.Contains(…) &&
    arguments[commandArguments.IndexOf(…) + 1].StartsWith("-") == false)
{
    … = commandArguments[commandArguments.IndexOf(…) + 1];
}
else
{
    isCmdLineWrong = true;
}

The Don't Repeat Yourself (DRY) principle tries to warn us from writing copy-and-paste-style repetitious code, and your original code is a pretty clear violation of it.

I suggest that you extract the common code and put it in a separate method. For example:

static bool TryGetArg(string commandArguments, string name, out string value)
{
    // Debug.Assert(name.StartsWith("-"));
    if (commandArguments.Contains("-") &&
        arguments[commandArguments.IndexOf(name) + 1].StartsWith("-") == false)
    {
        value = commandArguments[commandArguments.IndexOf(name) + 1];
        return true;
    }
    else
    {
        value = null;
        return false;
    }
}

Now you replace your repeated if else with the following:

string commandArguments = Environment.GetCommandLineArgs();

string arg1 = null;
string arg2 = null;
string arg3 = null;
bool isCmdLineOk = TryGetArg(commandArguments, "-r", out arg1) &&
                   TryGetArg(commandArguments, "-n", out arg2) &&
                   TryGetArg(commandArguments, "-p", out arg3);
if (isCmdLineOk)
{
    // do something with `arg1`, `arg2`, `arg3`.
}
else
{
    // not all of `arg1`, `arg2`, `arg3` could be set to a value.
    Console.WriteLine("Parameters structure is inconsistent");
}
stakx - no longer contributing
  • 83,039
  • 20
  • 168
  • 268
1

This question is a simple example of how to reuse code.

  • Look for code which appears to have been copied/pasted,
  • Put it in a function,
  • Any differences between copies, pass them in as parameters,
  • Replace the copies with function calls.

The result is

    // Returns this option's value from args, or null on error
    public string OptionValue(string[] args, string option)
    {
        try
        {
            if (args.Contains(option))
            {
                string value = args[args.IndexOf(option) + 1];  // reuse expressions as well

                if (!value.StartsWith("-"))
                    return value;
            }

            return null;    // null meaning "undefined"
        }
        catch
        {
            return null;  
        }
     }

     // And now your code
     string[] args = Environment.GetCommandLineArgs();

     string Arg1 = OptionValue(args, "-r"); 
     string Arg2 = OptionValue(args, "-n"); 
     string Arg3 = OptionValue(args, "-p"); 

     bool isCmdLineWrong = (Arg1 == null ||
                            Arg2 == null ||
                            Arg3 == null);

Of course, all this rewriting could have been avoided if you didn't copy/paste code to start with.