如何去掉 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)."