CodeSOD: A Random While

A bit ago, Aurelia shared with us a backwards for loop. Code which wasn't wrong, but was just... weird. Well, now we've got some code which is just plain wrong, in a number of ways.

The goal of the following Java code is to generate some number of random numbers between 1 and 9, and pass them off to a space-separated file.

StringBuffer buffer = new StringBuffer();long count = 0;long numResults = GetNumResults();while (count < numResults){ArrayList<BigDecimal> numbers = new ArrayList<BigDecimal>();while (numbers.size() < 1){int randInt = random.nextInt(10);long randLong = randInt & 0xffffffffL;if (!numbers.contains(new BigDecimal(randLong)) && (randLong != 0)){buffer.append(randLong);buffer.append(" ");numbers.add(new BigDecimal(randLong));}System.out.println("Random Integer: " + randInt + ", Long Integer: " + randLong);}outFile.writeLine(buffer.toString()); buffer = new StringBuffer();count++;}

Pretty quickly, we get a sense that something is up, with the while (count < numResults)- this begs to be a for loop. It's not wrong to while this, but it's suspicious.

Then, right away, we create an ArrayList<BigDecimal>. There is no reasonable purpose to using a BigDecimal to hold a value between 1 and 9. But the rails don't really start to come off until we get into the inner loop.

while (numbers.size() < 1){int randInt = random.nextInt(10);long randLong = randInt & 0xffffffffL; if (!numbers.contains(new BigDecimal(randLong)) && (randLong != 0)) ...

This loop condition guarantees that we'll only ever have one element in the list, which means our numbers.contains check doesn't mean much, does it?

But honestly, that doesn't hold a candle to the promotion of randInt to randLong, complete with an & 0xffffffffL, which guarantees... well, nothing. It's completely unnecessary here. We might do that sort of thing when we're bitshifting and need to mask out for certain bytes, but here it does nothing.

Also note the (randLong != 0) check. Because they use random.nextInt(10), that generates a number in the range 0-9, but we want 1 through 9, so if we draw a zero, we need to re-roll. A simple, and common solution to this would be to do random.nextInt(9) + 1, but at least we now understand the purpose of the while (numbers.size() < 1) loop- we keep trying until we get a non-zero value.

And honestly, I should probably point out that they include a println to make sure that both the int and the long versions match, but how could they not?

Nothing here is necessary. None of this code has to be this way. You don't need the StringBuffer. You don't need nested while loops. You don't need the ArrayList<BigDecimal>, you don't need the conversion between integer types. You don't need the debugging println.

[Advertisement] ProGet's got you covered with security and access controls on your NuGet feeds. Learn more.