5
\$\begingroup\$

Trying to complete an add user script. Nothing out of the ordinary, however I have transitioned over to using prepared statements, and I am trying to get better at creating my own functions.

<?php
include("../include/sessions.php");

if(isset($_POST['criteria']))
{
  $value = $_POST['criteria'];
  $firstname = htmlspecialchars(trim($value['firstname']));
  $lastname = htmlspecialchars(trim($value['lastname']));
  // few more

  // set blank variable to be passed into function
  $dbusername = ''; 

  // pass necessary variables into the function
  // $dbc is the database connection needed for the prepared statement
  function createUsername($firstname, $lastname, $dbusername, $dbc)
  {
    // check if $user ever matches $dbusername, if so, rerun loop
    while($user == $dbusername) 
    {
      $digits = 4;
      $randomNumber = rand(pow(10, $digits-1), pow(10, $digits)-1);
      // get first 3 letters of firstname
      $first3 = substr($firstname, 0, 3); 
      // get first 3 letters of lastname
      $last3 = substr($lastname, 0, 3); 
      // set user to $first3, $last3, and $randomNumber
      $user = $first3 . '' . $last3 . '' . $randomNumber;   

      // prepare statement
      $select = "SELECT username FROM users WHERE username = ?;";        
      $stmt = $dbc->prepare($select);

      // pass in the new $user
      $stmt->bind_param("s", $user);
      $stmt->execute();
      $result = $stmt->get_result();
      $row = $result->fetch_assoc();    

      // if the created user happens to exist in the database, set it to $dbusername
      $dbusername = $row['username'];   
      // rerun the loop if this happens
    }
    // if no matches, return the new username
    return $user;
  }
}

$user = createUsername($firstname, $lastname, $dbusername, $dbc);

echo 'username is ' . $user; // username should look like this: johbea3647
?>

I tried to notate the code as much as possible. But just in case, I'll break it down here...

The idea is to take create a brand new username that begins with the first 3 letters of the first and last name, then add a random 4 digit number to the end.

Hopefully, as long as the seemingly random username does not exist in the database, then return the new username. However, in the event the random username happens to exist, then run the loop again until there is no match.

I hope this all makes sense. Please review my code and see if there are any problems, or if there is a way to possibly simplify it all.

So far, everything seems to work. I'm just not too sure about that while loop.

\$\endgroup\$
  • 2
    \$\begingroup\$ Nice question. That's a pretty unusual requirement for a username to contain random numbers, I'm curious why that is? If you need a unique ID for a user, why not just make a GUID column in the database and use that, and just check for a duplicate user name only? \$\endgroup\$ – Phrancis May 15 '18 at 3:26
  • \$\begingroup\$ @Phrancis, I actually recommended that to my coworkers. Having an auto-incremented UID might save me the headache, however I think it can also be good practice in my case, being that I am relatively new to prepared statements, as well as I need to get better at creating my own functions. \$\endgroup\$ – John Beasley May 15 '18 at 3:43
  • 1
    \$\begingroup\$ @JohnBeasley it depends on the scale of your project. For a large expected user account base (think >10k although the exact number can be quite different depending on specifics ), random numbers as described in your question, will lead to problems. \$\endgroup\$ – Gnudiff May 15 '18 at 8:46
  • \$\begingroup\$ Do you know what htmlspecialchars() does? The call seems highly inappropriate here. \$\endgroup\$ – xehpuk May 15 '18 at 11:23
5
\$\begingroup\$

I don't write PHP, but there are several general things I would suggest.


Your 2-space indent makes your code more difficult to read, I would recommend 4.


$digits is always 4, so I would declare it as a constant, and rename it accordingly. You can move it outside the while loop also.

const $NUMBER_OF_DIGITS = 4;

$first3 and $last3 are also not going to change, so I would move them out of your while loop, and also name them accordingly.

const $NUMBER_OF_DIGITS = 4;
const $FIRST_3 = substr($firstname, 0, 3);
const $LAST_3 =  substr($lastname, 0, 3); 
while($user == $dbusername) {
    // ...

Your comments point out what should be easy to deduct for anyone who writes code regularly. I would suggest to reserve comments for function/class documentation (for example using PHPDoc) and otherwise, just to document things that are not obvious, or may be unusual workarounds to a certain problem/use case.

I think, in particular, perhaps this line deserves a short explanation:

$randomNumber = rand(pow(10, $digits-1), pow(10, $digits)-1);

Comments can also cause more work for you as the maintainer, as you have to also maintain comments in addition to the code, if anything is changed.

Comments can also be misleading, take this for example:

  // if the created user happens to exist in the database, set it to $dbusername
  $dbusername = $row['username'];   
  // rerun the loop if this happens

The comment should really say something like:

  // set $dbusername to either NULL, or an existing username in the case it does exist, which will cause the loop to re-run if it already exists in the database

But that's already pretty obvious from the code, so I would just delete the comment instead, along with most or all of the other ones.


Lastly, I think the while loop works fine, but a do-while loop would make a bit more sense, in that it makes it clear that the loop should run at least once, but may run more times under certain conditions.

do {
    // your logic code as it is
} while($user == $dbusername);
\$\endgroup\$
  • \$\begingroup\$ Your input is greatly appreciated. Thank you, sir. \$\endgroup\$ – John Beasley May 15 '18 at 12:50
  • \$\begingroup\$ "Unlike with variables, you should not prepend a constant with a $" -php.net/const... I am not sure if that documentation would say the reason is to avoid confusion with variables but that would be my guess \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jun 11 '18 at 18:21
3
\$\begingroup\$

The idea for having random numbers in username is going to be OK in your case, because you are adding the first and last name parts to it as well. It allows for a very large number of possible combos - although valid starts of names and even surnames are a small subset of the total possible combos of 6 letters.

If you only had the 4 numbers, not only you would only be technically able to support 9999 names only,but as you started approaching the limit, your script would run ever so slower while the RNG would loop trying to find a digit combo that is not yet used up.

There was actually a question on SE exactly about the issue - a guy was using RNG to generate his invoice numbers and was surprised the system was slowing down with time to the point that his customers couldn't place orders - especially since when very little slots remained multiple customers might have been trying to use up the same number simultaneously.

So it only makes sense to use RNG for parts of any id (username, invoice, etc) where the total number of projected ids is very small compared to the total possible combinations of characters of the field.

In general I would advise against using RNG for where a sequence works in any significant production code.

Sequences also allow to easier timing of events , as you will automatically know that uid0006 is somebody who had created his uid before the guy who had uid6000.

\$\endgroup\$
  • 2
    \$\begingroup\$ For small values (10000 can be considered to be small for that purpose) you can add all available numbers to a list and shuffle it. \$\endgroup\$ – allo May 15 '18 at 8:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.