How to reduce cyclomatic complexity for these if else statements

Question

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");

Answer 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");
}

Answer 2

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.

Answer 3

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") {
  ...
}