1

I am learning PHP and trying to use the while and continue expressions correctly.

I have a script that creates a 6 digit PIN, and I want to ensure that it is unique, otherwise I want to generate another PIN.

while(1) {
    $pin = rand(111111,999999);
    $sel = mysql_query("SELECT * FROM formusers WHERE pin = '$pin'");
    if(mysql_num_rows($sel) != 0) { continue; }

    mysql_query("INSERT INTO formusers(email,password,pin) VALUES('".$_POST['srEmail']."','".$_POST['srPass']."','".$pin."')");
    if(mysql_affected_rows()!=-1)  {
        echo "Pin:" . $pin;
        exit;
    } else {
        echo "Existing email, try again<br />";
    }
    break;
}

Do I have the syntax for the loop correct? It seems to work, but there's no way for me to debug it in the instance the rand() function creates the same PIN twice.

Also, in the event I did run out of unique PINs, what would happen here? Presumably it would loop indefinitely? Is there any way to prevent this?

alias51
  • 8,178
  • 22
  • 94
  • 166
  • 2
    is this pin for a password-reset email? Or a unique ID for the user? If it's a unique id, then don't make it a random number. You'll end up with collisions FAR sooner than you expect (look up 'birthday paradox'). Use an incrementing INT for ids. If it's a password reset, then it's far far too easy to guess. – Marc B Jul 22 '13 at 18:52
  • His logic looks right to me. Yes, he is breaking without a condition, but he is continuing with a condition. Yes, it's a little convoluted, but the logic looks correct, albeit this could be done a little more clearly. – mti2935 Jul 22 '13 at 18:52
  • The solution for this question has a great explanation about random unique values. [Solution Here](http://stackoverflow.com/questions/7579570/loop-until-passcode-is-unique) – Peter Jul 22 '13 at 18:54
  • 1
    It is more secure to allow duplicate pins than to restrict against them. You weaken the strength of your distribution slightly, and can multiply weaknesses in your distribution exponentially. – Paul Jul 22 '13 at 18:58
  • You should just make sure that you don't ever store your user's password in your database. Instead generate a random salt for each user and store a hashed salted version of their password. That will be much more secure. – Paul Jul 22 '13 at 18:59
  • It's a unique ID for the user, but needs to be a 6 digit unique number. @mti2935 How would I add a condition to the break? – alias51 Jul 22 '13 at 19:01
  • @RobM: Why does it *need* to be a 6-digit number? Why does it need to be random? Why not consecutive? – gen_Eric Jul 22 '13 at 19:02

3 Answers3

2

Yeah, the code works, but as stated the infinite loop is concerning. You could possibly solve that like this:

var $i = 0;

while($i < 10) {
    $i++;

    $pin = rand(111111,999999);
    $sel = mysql_query("SELECT * FROM formusers WHERE pin = '$pin'");
    if(mysql_num_rows($sel) != 0) { continue; }

    mysql_query("INSERT INTO formusers(email,password,pin) VALUES('".$_POST['srEmail']."','".$_POST['srPass']."','".$pin."')");
    if(mysql_affected_rows()!=-1)  {
        echo "Pin:" . $pin;
        exit;
    } else {
        echo "Existing email, try again<br />";
    }
    break;
}

This would ensure you would never make more than 10 iterations. However, the larger problem is probably that even that sampling size won't work in the future. This presents a bit of a conundrum. One possible approach is to determine how many unique numbers exist before starting the loop by counting them and then iterate that many times, but that approach is something like n2 (maybe, I'm not real good on big-O, I just know it would be bad).

Mike Perrenoud
  • 66,820
  • 29
  • 157
  • 232
2

You can leverage the unique nature of php array keys for this.

function getPins($qty){
    $pins = array();
    while (count($pins) < $qty){
        $pins[rand(111111,999999)] = ""; 
    }
    return array_keys($pins);
}

This will make sure that for a given run you will get $qty number of unique pins. You might want to look at appending a unique prefix to each run of the function to avoid multiple runs creating collisions between the two.

Orangepill
  • 24,500
  • 3
  • 42
  • 63
0

You can seed random, so it gives you the same values every time, you run it:

srand (55 );  //Or some other value.

The while loop syntax looks okay to me.

Pete B.
  • 3,188
  • 6
  • 25
  • 38