It’s the details, here. According to the comment, we expect 25 characters, but according to the call, it looks like it’s actually 50- GetRandomAlphanumericString(50). If, after ten tries, there isn’t a unique and random code, give up and chuck an exception- an untyped exception, making it essentially impossible to catch and respond to in a useful way.

As the comment points out- the odds of a collision are exceedingly small- at least depending on how the “random alphanumeric string” is generated. Even with case insensitive “alphanumerics”, there are quadrillions of possible strings at twenty five characters. If it’s actually fifty, well, it’s a lot.

Now, sure, maybe there’s a bias in the random generation, making collisions more likely, but that’s why we try and design our applications to avoid generating the random numbers ourselves.

James pointed out that this was silly, but the original developer misunderstood, and thought the for loop was the silly part, so now the code looks like this:

Might as well have gone all the way to a do...while. The best part is that regardless of which version of the code you use, since it’s part of a multiuser web application, there’s a race condition- the same code could be generated twice before being logged as an existing code in the database. That’s arguably more likely, depending on how the random generation is implemented.

Based on a little googling, I suspect that the GetRandomAlphanumericString was copy-pasted from StackOverflow, and I’m gonna bet it wasn’t one of the solutions that used a cryptographic source.