如何去掉 if-else if 或者 switch 的设计

Uncle Bob has written a article about craftsmanship, professionalism, and refactoring (Discussion ).

However, after reading this wonderful article (If you have not read it, please start now), I found that Im very uncomfortable with the "Switch-Case" (or if - elseif - elseif - else) statements, just like:

private   void  parseSchemaElement(String element)  throws  ArgsException 
{
     char  elementId  =  element.charAt( 0 );
    String elementTail  =  element.substring( 1 );
    validateSchemaElementId(elementId);
     if  (elementTail.length()  ==   0 )
        marshalers.put(elementId,  new  BooleanArgumentMarshaler());
     else   if  (elementTail.equals( " * " ))
        marshalers.put(elementId,  new  StringArgumentMarshaler());
     else   if  (elementTail.equals( " # " ))
        marshalers.put(elementId,  new  IntegerArgumentMarshaler());
     else   if  (elementTail.equals( " ## " ))
        marshalers.put(elementId,  new  DoubleArgumentMarshaler());
     else   if  (elementTail.equals( " [*] " ))
        marshalers.put(elementId,  new  StringArrayArgumentMarshaler());
     else
         throw   new  ArgsException(INVALID_ARGUMENT_FORMAT, elementId, elementTail);
}

And

public  String errorMessage() 
{
     switch  (errorCode) 
     {
         case  OK:
             return   " TILT: Should not get here. " ;
         case  UNEXPECTED_ARGUMENT:
             return  String.format( " Argument -%c unexpected. " , errorArgumentId);
         case  MISSING_STRING:
             return  String.format( " Could not find string parameter for -%c. " ,errorArgumentId);
         case  INVALID_INTEGER:
             return  String.format( " Argument -%c expects an integer but was '%s'. " ,errorArgumentId, errorParameter);
         case  MISSING_INTEGER:
             return  String.format( " Could not find integer parameter for -%c. " ,errorArgumentId);
         case  INVALID_DOUBLE:
             return  String.format( " Argument -%c expects a double but was '%s'. " ,errorArgumentId, errorParameter);
         case  MISSING_DOUBLE:
             return  String.format( " Could not find double parameter for -%c. " ,errorArgumentId);
         case  INVALID_ARGUMENT_NAME:
             return  String.format( " '%c' is not a valid argument name. " ,errorArgumentId);
         case  INVALID_ARGUMENT_FORMAT:
             return  String.format( " '%s' is not a valid argument format. " ,errorParameter);
    }
     return   "" ;
}

Though those code is clean, but everytime if we wanna add a new type of Argument, we have to add new if-else statement in "parseSchemaElement" method, and add new case statement in "errorMessage".  If your Args have to support more than 10 types argument, your if-else(switch-case) statements will be huge and ugly.

Bad smell.

So, can we avoid the Switch-Case statement in our system? Fortunately, the answer is yes: Just use the loop statement instead of it.

Let us see a new version of Args library:
First, the Args Class:

namespace  ArgsUtility
{
     public   class  Args
     {
         // ArgumentName:ArgumentMashaler
         private  Dictionary < char , ArgumentMarshalerBase >  _myArgumentMashalers  =   new  Dictionary < char , ArgumentMarshalerBase > ();
        
         // ArgumentName:ArgumentValue
         private  Dictionary < char ,  string >  _myArgumentValues  =   new  Dictionary < char , string > ();

         public  Args( string  schema,  string [] argumens) 
         {
             this .ParseSchema(schema);
             this .ParseArgumentStrings(argumens);
        }

         private   void  ParseSchema( string  schema)
         {
             if  ( string .IsNullOrEmpty(schema))
             {
                 throw   new  Exception( " Null Schema " );
            }

             foreach  ( string  s  in  schema.Split( ' , ' ))
             {
                 string  schemaNameAndSymbol  =  s.Trim();
                ValidateSchemaElement(schemaNameAndSymbol);

                 char  argumentName  =  schemaNameAndSymbol[ 0 ];
                 char  argumentTypeSymbol  =  schemaNameAndSymbol[ 1 ];

                ArgumentMarshalerBase amb  =  SupportedArguments.Instance.GetArgumentMarshaler(argumentTypeSymbol);
                 this ._myArgumentMashalers.Add(argumentName, amb);
            }
        }

         private    void  ValidateSchemaElement( string  s)
         {
             // each schema element should like this: 'x*', only two char
             if  (s.Length  !=   2 )
             {
                 throw   new  Exception( " Bad Schema:  "   +  s);
            }
        }

         private   void  ParseArgumentStrings( string [] values)
         {
            IEnumerator ie  =  values.GetEnumerator();

             // ugly code, i think, but all tests can pass, so let me ignore it at this time.
             while  (ie.MoveNext())
             {
                 if  (ie.Current.ToString().StartsWith( " - " ))
                 {
                     char  argumentName  =  ie.Current.ToString()[ 1 ];
                    
                     if  (ie.MoveNext())
                     {
                         string  argumentValue  =  ie.Current.ToString();
                         this ._myArgumentValues.Add(argumentName, argumentValue);
                    }
                }
            }
        }

         public   object   this [ char  argumentName]
         {
             get
             {
                ArgumentMarshalerBase amb;
                 if  ( this ._myArgumentMashalers.ContainsKey(argumentName))
                 {
                    amb  =   this ._myArgumentMashalers[argumentName];
                }
                 else
                 {
                     throw   new  Exception( string .Format( " Argument {0} unexpected. " , argumentName));
                }

                 string  argumentValue;
                 if  ( this ._myArgumentValues.ContainsKey(argumentName))
                 {
                    argumentValue  =   this ._myArgumentValues[argumentName];
                }
                 else
                 {
                     throw   new  Exception( string .Format( " Can't find {0} parameter for {1}. " , amb.ArgumentType, argumentName));
                }

                 return  amb.ParseInputValue(argumentValue);
                
            }
        }
    }
}

Look, in new version of "ParseSchema" method, the if-else statements were gone, and a static method which named "SupportedArguments.Instance.GetArgumentMarshaler(argumentTypeSymbol);" instead of them.

So, let see how "GetArgumentMarshaler(string argumentTypeSymbol)" works:

namespace  ArgsUtility.ArgumentMarshaler
{
     class  SupportedArguments
     {
         private  List < ArgumentMarshalerBase >  _commonArgs  =   new  List < ArgumentMarshalerBase > ();
         readonly   public   static  SupportedArguments Instance  =   new  SupportedArguments();

         private  SupportedArguments()
         {
             this ._commonArgs.Add( new  BooleanArgumentMarshaler());
             this ._commonArgs.Add( new  IntegerArgumentMarshaler());
             this ._commonArgs.Add( new  DoubleArgumentMashaler());
             this ._commonArgs.Add( new  StringArgumentMashaler());
             // todo: add more argumentMarshalers
        }

         public  ArgumentMarshalerBase GetArgumentMarshaler( char  typeSymbol)
         {
             foreach  (ArgumentMarshalerBase amb  in   this ._commonArgs)
             {
                 if  (amb.IsSymbolMatched(typeSymbol))
                 {
                     return  amb;
                }              
            }
             throw   new  Exception( string .Format( " Unknow Argument Symbol:{0}. " ,typeSymbol));
        }

    }
}  

"GetArgumentMashaler" method check all supported argumentMashalers in _commonArgs collection, check them one by one and find the correctly Argument Mashaler.

And the ArgumentMarshalerBase is:

namespace  ArgsUtility.ArgumentMarshaler
{
     abstract   class  ArgumentMarshalerBase
     {
         private   char  _schemaSymbol;
         public  ArgumentMarshalerBase( char  schemaSymbol)
         {
             this ._schemaSymbol  =  schemaSymbol;
        }

         public   bool  IsSymbolMatched( char  targetSymbol)
         {
             return  targetSymbol  ==   this ._schemaSymbol;
        }

         public   object  ParseInputValue( string  value)
         {
             try
             {
                 return   this .DoParse(value);
            }
             catch  (FormatException ex)
             {
                 // catch all format problems
                 throw   new  Exception( string .Format( " '{0}' is an invalid {1} format. " , value,  this .ArgumentType),ex);
            }

        }

         abstract   protected   object  DoParse( string  value);
         abstract   public   string  ArgumentType
         {
             get ;
        }
                      
    }
}

 

The BooleanArgumentMarshaler implement as below:

namespace  ArgsUtility.ArgumentMarshaler
{
     class  BooleanArgumentMarshaler:ArgumentMarshalerBase 
     {
         public  BooleanArgumentMarshaler()
            :  base ( ' ? ' )
         {
        }

         protected   override   object  DoParse( string  value) 
         {
             return  Boolean.Parse(value);
        }

         public   override   string  ArgumentType
         {
             get
             {
                 return   " Boolean " ;
            }
        }
    }
}

Pretty simply，huh? Now, if we wanna add new type of argument, we just need:
1. Write a new class which implement abstract class: ArgumentMashalerBase
2. In the new class constructor, set the a symbol for it. This symbol will be used in schema string. E.g. the symbol of boolean is "?" and symbol "#" is for integer type argument.

You can will see the new argument will works well.

[Test]
         public   void  TestComplexArguments()
         {
             /**/ /*  this schema includes 7 arguments:
             * Integer Arguments: i, j
             * Boolean Arguments: b
             * Double Arguments: d, x
             * String Arguments: m, n
              */
             string  schema  =   " i#,b?,j#,d&,m*,n*,x& " ;

             // the argument values
             string [] values  =   new   string []  {
                 " -i " ,  " 32 " ,
                 " -d " ,  " 33.4 " ,
                 " -m " ,  " string 1 " , 
                 " -b " ,  " false " ,
                 " -j " ,  " 53 " ,
                 " -x " ,  " 11.2 " ,
                 " -n " ,  " string 2 " } ;

            Args args  =   new  Args(schema, values);

            Assert.AreEqual( 32 , ( int )args[ ' i ' ]);
            Assert.AreEqual( 33.4 , ( double )args[ ' d ' ]);
            Assert.AreEqual( " string 2 " , ( string )args[ ' n ' ]);
            Assert.AreEqual( " string 1 " , ( string )args[ ' m ' ]);
            Assert.AreEqual( 53 , ( int )args[ ' j ' ]);
            Assert.IsFalse(( bool )args[ ' b ' ]);
            Assert.AreEqual( 11.2 , ( double )args[ ' x ' ]);
        }

Is that all? NO! Please, do not forget add new test cases for your new argumet ;)

Full source code and unit test are HERE . Any suggest is welcome.

linkcd
2006/02/21

Update 2006/02/22

Uncle Bob said:
"As for the argument about if/else and switch, that's a religious argument IMHO. Switches and if/else chains may be badly abused, and may never be necessary, but sometimes they are pleasant to read. My rule for them is that they should never appear more than once."

My Reply:
" But Uncle Bob, EVERYTIME if we wanna add a new argument type, we have to modify the if-else statement in Args class, and switch-case statement in exception class. We are hit by SAME bullet again and again. I think it violates "Open Close Principle". Yes, i agree the Switch statement are pleasant to read, but if our users ALWAYS want us to support new argument type, we should better avoid if-else/switch statements."

Uncle Bob reply:
"You make a good point. The two switch statements, while not identical, are related; and this violates my rule. On the other hand there is no benefit to turning the switch statement in ArgsException into a table, since it would still need to be modified under the same conditions. I could eliminate the ArgsException switch statement by deferring the messages back to the ArgumentMarshaller, but that couples Messages (UI) with business rules and therefore violates SRP.

I think yours is a good argument for eliminating the messages in the ArgsException class altogether. On the other hand, that just pushes the switch statement out into the user code.

Another solution would be to create a hierarchy of ArgsException derivatives. That would get rid of the switch, but does not completely solve your OCP complaint, since we'd have to add new exception derivatives every time we added a new type.

That leaves us with eliminating the messages and pushing that switch out to the application code. I'm not happy that. So I think the switch in ArgsException, though it violates OCP, helps to keep user code cleaner, and maintains a good separation of concerns (SRP)."