Cleaning up control flow when dealing wtih command

Question

I'm dealing with a program that does plenty of if...else branching based on command line arguments. This is in C# but I'm sure it's applicable to Java, C++ etc. Here's the general outline:

if (args.Length == 0)
{
  //do something
}

if (args.Length > 0 && args.Length < 2)
    {
        Console.WriteLine("Only one argument specified. Need two arguments");
        return 0;

    }
            else if (args.Length > 0 && args.Length >= 2)
            {
                //Process file - Argument 1
                if(args[0].Trim() == PROCESS_OPTION_ONE
                    || args[0].Trim() == PROCESS_OPTION_TWO)
                {
                    //Process file - Argument 2
                    if(args[1].Trim() == PROCESS_CUSTOMER
                        || args[1].Trim() == PROCESS_ADMIN
                        || args[1].Trim() == PROCESS_MEMBER
                        || args[1].Trim() == PROCESS_GUEST
                        || args[1].Trim() == PROCESS_USER
                        )
                    {

So as you can tell, it's kind of a mess. Is there a design pattern or two that would be most applicable toward cleaning things up some? Command pattern, perhaps? Thanks for the advice and tips.

Answer 1

I'm partial to using switch statements on the arguments array and setting properties in a configuration class of some kind for each anticipated argument. It appears you're expecting a very specifically formatted argument string rather than allowing set values, you could try:

if(args[0].Trim() == PROCESS_OPTION_ONE || args[0].Trim() == PROCESS_OPTION_TWO) 
{ 
    //Process file - Argument 2
    switch(args[1].Trim()
    {
        case PROCESS_CUSTOMER, PROCESS_ADMIN, PROCESS_MEMBER, PROCESS_GUEST, PROCESS_USER:
            // Do stuff
            break;
        default:
            // Do other stuff
            break;
    }
}

My preferred method would be something like

foreach(string arg in args)
{
    switch(arg)
    {
        case PROCESS_CUSTOMER:
            // Set property
            break;
        ...
        default:
            // Exception?
            break;
    }
}

NOTE: args.Length == 1 is faster than args.Length > 0 && args.Length < 2. It's also a little more readable.

Answer 2

Stop nesting.

You can switch like (+1) Joel said, or you can just break your logic into clear method calls.

if(args.Length <= 1)
{
  Console.WriteLine("Need 2 args kthx");
  return;
}
if(args.Length > 2)
{
  Console.WriteLine("More than 2 args don't know what do");
  return;
}

var arg1 = args[0].Trim();
var arg2 = args[1].Trim();

switch(arg1)
{
  case PROCESS_OPTION_ONE:
     ProcessOptionOne(arg2);
     break;
  case PROCESS_OPTION_TWO:
     ProcessOptionTwo(arg2);
     break;
  default:
     Console.WriteLine("First arg unknown I give up");
     return;
}

then, in your process methods...

private static void ProcessOptionTwo(string argumentTwo)
{
  if(argumentTwo == PROCESS_CUSTOMER ||
     argumentTwo  == PROCESS_ADMIN ||
     /* etc blah blah */
}

Keep your methods as simple as possible and break up longer, confusing algorithms into distinct method calls which, through their name, give a clear indication of what they are doing.

Answer 3

I took the code from this Code Project article a long time ago and made my own version of it to use for command line applications. I did my own modifications to it, like making the class inherit from Dictionary, etc. But the regex part of the code is very good, and makes these sort of command line switches easy as pie.

Answer 4

You don't need the else if you've already returned. That might cut out a lot of your nesting. You could also try using a switch instead of a bunch of nested ifs.