Does this small piece of code seem ok?

Hi,

I am not sure why but I am uncomfortable with this piece of code.

All I am doing after this loop is inserting the generated $code value (not for crypotgraphic purposes) into a unique column, in a later query. So to make sure the code is unique I am doing this:

PHP Code:

$loop = TRUE;
 while($loop)
 {
    $code = random_string('alnum', 16);
    $this->db->where('guest_code', $code);
    $check = $this->db->count_all_results('guests');
    if ($check == 0)
    {
       $loop = FALSE;
    }
 } 


But I am uncomfortable with the loop though. Is this a sensible way to do this?

It is working and I have moved on for now but it is bugging me because of the potential for an infinite loop. Is this a bad thing to do or am I being silly.

Any advice would be welcomed,

Best wishes,

Paul.

The only potential for an infinite loop is if every possible combination that random_string can return is already stored in your table.
You could add a counter and have the loop exit when it hits 1000 for example.

Thanks for the reply.

I will do that and then add an error message particular for that failure. "You could not add that guest as we were unable to generate a unique code for them" or similar.

I still don't like it though (the loop above that is)

Best wishes,

Paul.

For generating a unique code you will need to check if it already exists, and if it does regenerate.
If you don't want the loop you could create this as an iterative method.

PHP Code:

public function generate_code() {
    $code = random_string('alnum', 16);
    $this->db->where('guest_code', $code);
    $check = $this->db->count_all_results('guests');
    if ( $check >= 1 ) {
       return $this->generate_code();
    }
    return $code;
} 


That is an awesome solution!!!!

At first I was concerned that it was worse as it might hit the memory limit as php has to remember all the deeper and deeper levels of functions within functions, but that is not how PHP works in this case (I think). It seems that it assigns the function to the variable, returns that, 'level' closed, then re-evaluates it.

That lead me to this: http://www.phptherightway.com/pages/Func...mming.html which was interesting too but I really must get back to what I was doing :-)

I am definitely going to use that function - thank you!

Paul.

Code:

$guest_code = bin2hex(pack('N', microtime(TRUE))).bin2hex(get_instance()->security->get_random_bytes(4));

Almost guaranteed uniqueness and better randomness.

With a random 16 alphanumeric string, I don't even check if it is duplicated. The chances are too low. If you're still worried increase the string length.

Practical guide to IgnitedCMS - Book coming soon, www.ignitedcms.com

With a random 16  alphanumeric string, I don't even check if it is duplicated. The chances are too low. If you're still worried increase the string length.

Just because the changes are low does not mean you should ignore the fact it CAN and at some point WILL happen. You can't cover every single scenario in programming, but to ignore something like this that is quite easy to avoid is just bad practice. But that is my opinion

Just because the changes are low does not mean you should ignore the fact it CAN and at some point WILL happen

When you say will happen, can you determine the probability of a random 16 alphanumeric collision.

http://betterexplained.com/articles/the-...-to-guids/

Interesting, but I'm sure a 16 digit number is quite safe for low volume visitors anyway.

Practical guide to IgnitedCMS - Book coming soon, www.ignitedcms.com

I said at SOME POINT. No way of knowing when or where, but fact remains that it CAN happen. If something can happen that could cause an error that is avoidable, I choose to avoid it.