trjhtr

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.

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);

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.