What's wrong with this code?

[Dru Lee Parsec]

I'm doing some maintenance of some old code and found this little gem. All I'll tell you is that the syntax is correct and it works. The original programmer was trying to find out if a zip code was in a 5 digit format or in a 9 digit format like this: 12345 or 12345-6789.

But just for fun, see if you can find all the things wrong with this little bit of code:


String zip = new String(req.getParameter("ZIP"));

// several lines deleted for clarity

StringTokenizer ziptk = new StringTokenizer(zip, "-");
int zipcount = ziptk.countTokens();
String zip1 = null;
String zip2 = null;
switch (zipcount) {
case 2:
while (ziptk.hasMoreElements()) {
zip1 = (String) ziptk.nextElement();
userBean.setZip(zip1);
zip2 = (String) ziptk.nextElement();
userBean.setZip1(zip2);
}
case 1:
while (ziptk.hasMoreElements()) {
zip1 = (String) ziptk.nextElement();
userBean.setZip(zip1);
userBean.setZip1("");
}
}



Bonus question, How would you make this code faster?

[Strike]

Well, you don't need to bother calling the count function. Just do:


String zip1 = "";
String zip2 = "";
try {
zip1 = (String)ziptk.nextElement();
zip2 = (String)ziptk.nextElement();
} catch (NoSuchElementException e) {
// Whatever diagnostics you want to print here, none needed
}
userBean.setZip(zip1);
userBean.setZip1(zip2);

[Dru Lee Parsec]

That's one way to clean it up. But if he did use the countTokens method then what's wrong with in switch statement?

[Strike]

Originally posted by Dru Lee Parsec
That's one way to clean it up. But if he did use the countTokens method then what's wrong with in switch statement?
Heh, I didn't even look for the problem in the existing code, just how I would do it :) I don't know about switch statements in Java really, but I'll look at it (I don't particularly care for the extra "complexity" of switch statement (well, really they are nothing but neutered if's, but it adds to the complexity of maintaining code when you have to learn the semantics of both))

[gufmn]

couldn't you just do something like:

String zip = "92868"

if (zip.length() <= 5) {
userBean.setZip(zip)
}else{
userBean.setZip1(zip);
}

[Strike]

Originally posted by gufmn
couldn't you just do something like:

String zip = "92868"

if (zip.length() <= 5) {
userBean.setZip(zip)
}else{
userBean.setZip1(zip);
}
Except that Zip and Zip1 both need to be set.

[inkedmn]

looks like he needs a "default" statement at the end (unless that was omitted) to protect against invalid user input.

also, the code doesn't check for the length of the tokens. it just tries to split it at the '-'.

i know i'm WAAAAY n00b at this, but i might do something a little different:
(i have no idea if this would even work, btw) :

String zip = new String(req.getParameter("ZIP"));

// several lines deleted for clarity

String zip1 = null;
String zip2 = null;

if (zip.indexOf("-") == -1) {
userBean.setZip(String.parseInt(zip1))
} else {
userBean.setZip(String.parseInt(zip.substring(0, zip.indexOf("-")));
userBean.setZip1(String.parseInt(zip.substring(zip.indexOf("-" + 1)));


seems like fewer steps to me, but i'm not totally sure...
:)

[Dru Lee Parsec]

// assumes zip is in one of two formats 12345 or 12345-6789
int index = zip.indexOf("-");
String zip1 = "":
String zip2 = "":
if (index != -1) {
zip1 = zip.substring(0,index-1);
zip2 = zip.substring(index, zip.length()-1);
}
else {
zip1 = zip;
}



Instead we have

int zipcount = ziptk.countTokens();

OK, so we know if there are 1 or 2 tokens. Although a String tokenizer is WAY slow. Why not just get the index of the - and do two substrings as I did above??


switch(zipcount) {
case 2:


STOP right there. if we got this far then we know there are 2 tokens right? Then WTF did he continue with a


while (ziptk.hasMoreElements()) {


AND THEN, he gets the 2 tokens. Well of course the while loop will allow entry the first time, you're in the case where you have 2 elements. BUT THEN HE GETS 2 ELEMENTS which is exactly the number he must have to get into this case of the switch statement. SO WTF is it wrapped with a "hasMoreElements" ???

To make matters worse, he does it again in the next case.

BTW, He better hope that there are always 1 or 2 tokens (not 0 or 3) And where's the default statement in the switch statement? What happens if there are 0 or 3 tokens?

And this is part of a 548 line class with ONE method.

Oh, and who decided to name the method that sets the 2nd part of the zip code "setZip1" ?? How about setZip1() and setZip2() ? Or better yet setZip() and setZipPlusFour(). Or maybe we could just pass the who damn string to one method in UserBean called setZip and let IT do the damn parsing. But that's getting dangerously close to Object Oriented thinking and we can't have that! Can we?

Geez I hate seeing this kind of crap in a product that I originally wrote. If we had time to do code reviews then the code would be cleaner and faster and he would have learned how to write code properly. It's feels like I built a nice house, left home for a bit, and then I came back to find that sombody had sprayed grafitti all over it.

BTW, the guy who wrote this doesn't work here any more. I hope he's a better programmer since he wrote this.


[edit] Inkedmn, you posted your answer while I was writing mine and we both came up with essentially the same solution. Good job!

[Strike]

Okay, just to cover my own ass - I don't know a lot about java text processing, so I just used the existing mechanism. I totally agree that using an indexOf would be much faster (and if it had something as nice as Python's contains method for each String then that's even better). In fact, for fun (and because I'm a python whore), here's the Python version:

if zip.contains("-"):
zip1, zip2 = zip.split("-")
# bean set stuff obviously isn't applicable in Python
else:
zip1 = zip

[Strike]

I just checked, Java has a split method too (http://java.sun.com/j2se/1.4.1/docs/api/java/lang/String.html#split(java.lang.String)), so if you wanted to do the Java equivalent of what I have above, then that'd work too.

[Dru Lee Parsec]

Split!?? I love that !

Rats, it's new since 1.4 and our server only has 1.3 on it. Oh well. That would be an even faster way to do it.

But still, the while loops that do NOTHING!? That was what really ticked me off.

[gufmn]

while (Dru.tickedOff ()){
rant++;
}
return rant;

I think this while loop is useful :)

[Dru Lee Parsec]

:D

[stuka]

Well, I got to this one late, but before reading Dru's solution, I thought of the basic structure. Interesting side note - I think I know why this guy used the hasMoreElements - he forgot his effin' break! He was probably trying to do 2 in the first part, and kept getting exceptions thrown, and then wrote in the while() loops to stop that, instead of putting a break in there!!!!!

[Dru Lee Parsec]

Hmm. Interesting idea. It doesn't seem that it would drop through because nothing in case 2: is modifying zipcount. If something in case 2: subtracted 1 from zipcount then it would drop through to dase 1:

But you're also right, he's missing break; and default: And he didn't need a StringTokenizer OR a switch statement in the first place.

Here's the kicker, I'm actually looking at this code to see what database code he was calling so I can insert the same data from a new source. But since this code is in production I can't just fix it and say "Put it up there" because we would have to go through all our QA and audit test again.

OK, I'm calming down now.

<dru chants to himself> Release the rant, release the rant, release the rant.

[stuka]

Dru: are Java switches that different from C/C++ switches? I haven't used 'em that much in Java, but I KNOW that in C/C++ processing falls through the cases if you don't break.

[Dru Lee Parsec]

You're right, it will drop through even if zipcount isn't modified. (I was wrong :( )

You're also correct that the right way to do this is with a break statement.