PDO sign up function inserting data into multiple tables

This is a sign up function called on form submission. It firstly inserts key user data into the users table. If successful, secondary data is then inputted into respective tables (such as user job titles and user experience). I'm not really sure whether stacking queries 2-5 is the best way to do it, so I would be interested in knowing how this function could be improved and/or made more secure.

```
public function registerFreelancer($firstname, $lastname, $email, $password, $location, $portfolio, $jobtitle, $priceperhour, $experience, $bio, $userType){

global $bcrypt;
global $mail;

$time = time();
$ip = $_SERVER['REMOTE_ADDR'];
$email_code = sha1($email + microtime());
$password = $bcrypt->genHash($password);// generating a hash using the $bcrypt object

$query = $this->db->prepare("INSERT INTO " . DB_NAME . ".users
(firstname, lastname, email, email_code, password, time_joined, location, portfolio, bio, ip)
VALUES
(?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
");

$query->bindValue(1, $firstname);
$query->bindValue(2, $lastname);
$query->bindValue(3, $email);
$query->bindValue(4, $email_code);
$query->bindValue(5, $password);
$query->bindValue(6, $time);
$query->bindValue(7, $location);
$query->bindValue(8, $portfolio);
$query->bindValue(9, $bio);
$query->bindValue(10, $ip);

try{
$query->execute();

// Send email code usually here

$rows = $query->rowCount();

if($rows > 0){

$last_user_id = $this->db->lastInsertId('user_id');

$query_2 = $this->db->prepare("INSERT INTO " . DB_NAME . ".freelancers (freelancer_id, jobtitle, priceperhour) VALUE (?,?,?)");

$query_2->bindValue(1, $last_user_id);
$query_2->bindValue(2, $jobtitle);
$query_2->bindVal

-
If the second or later query fails you'll have inconsistent data in your database. Consider using atomic transactions.

-
$query_2, $query_3 are bad names. You could pick something more descriptive, like $usersInsert, $freelancersInsert etc.

-

$query  = $this->db->prepare("INSERT INTO " . DB_NAME . ".users
    (firstname, lastname, email, email_code, password, time_joined, location, portfolio, bio, ip) 
    VALUES 
    (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
");

$query->bindValue(1, $firstname);
$query->bindValue(2, $lastname);
...

I'd use named binds here. As far as I see they works with INSERT statements too.

$query  = $this->db->prepare("INSERT INTO " . DB_NAME . ".users
    (firstname, lastname, email, email_code, password, time_joined, location, portfolio, bio, ip) 
    VALUES 
    (:firstname, :lastname, ...)
");

$query->bindValue(":firstname", $firstname);
$query->bindValue(":lastname", $lastname);
...

It would be less error-prone (harder to mix parameters up) and would be easier to read/follow.

-
Consider Hayley Watson's comment on the die manual page:


It is poor design to rely on die() for error handling in a web site because it results in an ugly experience for site users: a broken page and - if they're lucky - an error message that is no help to them at all. As far as they are concerned, when the page breaks, the whole site might as well be broken.


If you ever want the public to use your site, always design it to handle errors in a way that will allow them to continue using it if possible. If it's not possible and the site really is broken, make sure that you find out so that you can fix it. die() by itself won't do either.

Furthermore, at least log the error to a log file, otherwise you might be never know if your future users can't even register.

-

if($userType == 'designer') {
    $query_5 = $this->db->prepare("INSERT INTO " . DB_NAME . ".designer_titles (job_title_id, job_title) VALUE (?,?)");

    $query_5->bindValue(1, $last_user_id);
    $query_5->bindValue(2, $jobtitle);              

    $query_5->execute();

} else if ($userType == 'developer') {
    $query_5 = $this->db->prepare("INSERT INTO " . DB_NAME . ".developer_titles (job_title_id, job_title) VALUE (?,?)");

    $query_5->bindValue(1, $last_user_id);
    $query_5->bindValue(2, $jobtitle);              

    $query_5->execute();
}

Both cases are almost the same. You could create a function for that with a $tableName parameter to remove the duplication.

It can be a sign that you could have another database structure with only one table instead of two:

TABLE titles:
    - role (possible values: developer, designer)
    - job_title_id
    - job_title

-

if($userType == 'designer') {
    ...
} else if ($userType == 'developer') {
    ...
}

You could be more defensive here, if $userType contains something else (not designer nor developer) and if it's should be considered as a programming (or input validation) error sing it somehow. I'd throw an exception and log it in a catch block. (The Pragmatic Programmer: From Journeyman to Master by Andrew Hunt and David Thomas: Dead Programs Tell No Lies.)

-
You could save a few indentation level with guard clauses which would be readable. You wouldn't have to read through the whole function to figure out what happens when $rows > 0 is false:

$rows = $query->rowCount();

if($rows > 0){
    return;
}
$last_user_id =  $this->db->lastInsertId('user_id');
...

It might be more unambiguous using return false instead of implicit return here.

-
The comment doesn't say too much, the code is already obvious here:

$password   = $bcrypt->genHash($password);// generating a hash using the $bcrypt object

I'd remove it. (Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)

-
A lot of parameters is a code smell: Long Parameter List. Their type are the same, it's easy to mix them up. Consider using a parameter object instead which would contain named fields.

public function registerFreelancer($firstname, $lastname, $email, $password, $location, $portfolio, $jobtitle, $priceperhour, $experience, $bio, $userType){

See: Martin Fowler's Refactoring: Improving the Design of Existing Code book, Chapter 3. Bad Smells in Code, Long Parameter List

-
I've found this kind of formatting is rather hard to maintain:

$time       = time();
$ip         = $_SERVER['REMOTE_ADDR'];
$email_code = sha1($email + microtime());
$password   = $bcrypt->genHash($password);// generating a hash using the $bcrypt object

If you have a new variable with a longer name you have to modify seven other lines too to keep it nice. It also looks badly on revison control diffs and could cause unnecessary merge conflicts.

From Code Complete, 2nd Edition by Steve McConnell, p758:


Do not align right sides of assignment statements


[...]


With the benefit of 10 years’ hindsight, I have found

StackExchange Code Review Q#45046, answer score: 7