When "if/else if" Isn't Good Enough

I found this classic, albeit minor, WTF when going through some code recently. Of course, the variable names and constants have been changed to protect the stupid. Otherwise, this snippet of code is pretty much as I found it.

int argVal = 0;
int anotherVar = 0;
if(arg.equals("one")) argVal = 1;
else if(arg.equals("two")) argVal = 2;
else if(arg.equals("three")) argVal = 3;
// more "else if" statements go here
switch(argVal) {
	case 1:
		anotherVar = -1;
		break;
	case 2:
		anotherVar = -4;
		break;
	case 3:
		anotherVar = -9;
		break;
    // more case statements go here
	default:
		anotherVar = -5;
}

Medezark

So you're saying they should stop passing argvals as text and instead pass them as integers and replace the switch with

anotherVar = argVal * -argVal

?

frits

@Medezark said:

So you're saying they should stop passing argvals as text and instead pass them as integers and replace the switch with

anotherVar = argVal * -argVal

?

Surely there should be an Integer.parseInt() in there.

zelmak

@frits said:

@Medezark said:

So you're saying they should stop passing argvals as text and instead pass them as integers and replace the switch with

anotherVar = argVal * -argVal

?

Surely there should be an Integer.parseInt() in there.

Surely there needs to be a StringBasedNumber class which represents numbers as strings, allows conversions from one to the other, and also supports localization ....

frits

@zelmak said:

@frits said:
@Medezark said:

So you're saying they should stop passing argvals as text and instead pass them as integers and replace the switch with

anotherVar = argVal * -argVal

?

Surely there should be an Integer.parseInt() in there.

Surely there needs to be a StringBasedNumber class which represents numbers as strings, allows conversions from one to the other, and also supports localization ....

Good point. Probably want to be able to convert to different bases, too. e.g. Binary, hexadecimal, unary, etc.

Sorry for any confusion as a result of my poor choice of replacement constant values. This was not a bad attempt at localization. They were trying to set a numeric variable based on the value of a string variable, but the input strings weren't values which could be converted to numbers using existing (localization) API.

My initial thought was that they could have simply done without the "switch/case" statements and used only the "if/else if" statements, but upon further reflection I can see where the existing conditional logic could have been done away with altogether. Instead, a simple, static Map type object could have been used to look up the numeric value corresponding with the particular string input.

Cbuttius

Depends on language here:

- If It's C# you can switch..case on strings and potentially they should be doing that.

- If It's Java you can't and a map might be the right answer here

It seems pointless creating argVal just to then go and set anotherVar, when we could set anotherVar immediately.

Using switch..case at all generally rings a bell with me, you should probably using abstract factory pattern in a case like this. Where the number of possible strings is small nested ifs is often the best solution though eg.

 if( var.equals("true"))
   val = true;
else if (var.equals("false"))
  val = false;
else
  val=fileNotFound;

@Cbuttius said:

If It's Java you can't and a map might be the right answer here

It's Java.

@Cbuttius said:

It seems pointless creating argVal just to then go and set anotherVar, when we could set anotherVar immediately.

That was my thought precisely. This was just one minor WTF among thousands, most of which at this time cannot share be shared. Sadly, I'm not going to be making any changes to this code, so all I can do is point out these little gems and hope the original developer will learn or be replaced.

Of course, if I run into any more WTF which can safely be shared I'll be sure to pass them on.

DaveK1

@Cbuttius said:

you should probably using abstract factory pattern in a case like this

Holy over-engineering, Batman! Are you suffering from pattern-itis? You want to implement a trivial static look-up table of some sort and your first thought is to reach for an abstract factory pattern?

If you think that's a good idea, I've got a framework in London that I'd like to talk about selling to you...!

Medezark

HAI
CAN HAS STDIO?
GIMMEH arg
I HAS A anotherVar ITZ -5
WTF IZ arg?
  OMG "one"
   anotherVar R -1
  GTFO
  OMG "two"
   anotherVar R -4
  GTFO
  OMG "three"
   anotherVar R -9
  GTFO
  BTW Repeat all other case statements
  OMGWTF
   anotherVar R -5
OIC
VISIBLE AnotherVar
KTHXBYE

dhromed

That is beautiful.

Qwerty

@Medezark said:

KTHXBYE

"LOL" gets over-used but that really did make me laugh out loud. Considering I'm sitting in an office chair with wheels, that means I can ROFL.

Cbuttius

@DaveK said:

@Cbuttius said:
you should probably using abstract factory pattern in a case like this
Holy over-engineering, Batman! Are you suffering from pattern-itis? You want to implement a trivial static look-up table of some sort and your first thought is to reach for an abstract factory pattern?
If you think that's a good idea, I've got a framework in London that I'd like to talk about selling to you...!

No I do not suffer from pattern-itis, actually I am one who believes that design is something done with the brain, not with patterns. Patterns are just the pretty names for the solutions and unfortunately too many people judge the ability to do good design by the number of patterns you can mention by their names.

In this particular instance, you probably want to look up the string in some kind of table and invoke the proper handler for it. It is difficult to say how it needs to be done in this instance because I have no context, but abstract factory is good in the instance that you read one string then determine how to go next to handle the rest of the input.

And no thanks, I am not really interested in buying your framework, I prefer to write my own code, green-field.

frits

@Cbuttius said:

@DaveK said:

@Cbuttius said:

you should probably using abstract factory pattern in a case like this

Holy over-engineering, Batman! Are you suffering from pattern-itis? You want to implement a trivial static look-up table of some sort and your first thought is to reach for an abstract factory pattern?

If you think that's a good idea, I've got a framework in London that I'd like to talk about selling to you...!

No I do not suffer from pattern-itis, actually I am one who believes that design is something done with the brain, not with patterns. Patterns are just the pretty names for the solutions and unfortunately too many people judge the ability to do good design by the number of patterns you can mention by their names.

In this particular instance, you probably want to look up the string in some kind of table and invoke the proper handler for it. It is difficult to say how it needs to be done in this instance because I have no context, but abstract factory is good in the instance that you read one string then determine how to go next to handle the rest of the input.

And no thanks, I am not really interested in buying your framework, I prefer to write my own code, green-field.

Sounds like you're looking for some kind of "Dictionary" that contains "Actions". Someone should invent that.

When &quot;if/else if&quot; Isn't Good Enough

When "if/else if" Isn't Good Enough