10
\$\begingroup\$

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->bindValue(3, $priceperhour);                      

                $query_2->execute();

                $query_3 = $this->db->prepare("INSERT INTO " . DB_NAME . ".user_types (user_type_id, user_type) VALUE (?,?)");

                $query_3->bindValue(1, $last_user_id);
                $query_3->bindValue(2, $userType);              

                $query_3->execute();

                $query_4 = $this->db->prepare("INSERT INTO " . DB_NAME . ".user_experience (experience_id, experience) VALUE (?,?)");

                $query_4->bindValue(1, $last_user_id);
                $query_4->bindValue(2, $experience);                            

                $query_4->execute();

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

                return true;
            }

        }catch(PDOException $e){
            die($e->getMessage());
        }   
    }
\$\endgroup\$
  • 2
    \$\begingroup\$ Just so it's said, your code is pretty intimate with the structure of the DB. Which will end up being annoying later on, particularly if this is running outside of the model/DAL. I'd much rather say $user = (get a fresh User object); $user->someAttribute = $someValue; ... ; $user->insert();. \$\endgroup\$ – cHao Mar 22 '14 at 0:16
  • \$\begingroup\$ Could you perhaps elaborate on what you mean - I'm fairly new to OOPHP, so it would be good to know what you are talking about in more detail \$\endgroup\$ – jshjohnson Mar 22 '14 at 21:32
  • \$\begingroup\$ I mean that if this code is actually in the signup page, and other code that messes with these records is also in its respective page, then you have a bunch more places to worry about changing if you change anything about users. You'd be better off to take these functions out of their respective pages and group them together in one place (even if they're all just functions in the same file...although a popular option is to put them in a User class). The end result would be one reusable, ideally cohesive library of functions to manipulate users. (Sorry, just saw this.) \$\endgroup\$ – cHao May 3 '14 at 19:11
7
\$\begingroup\$
  1. If the second or later query fails you'll have inconsistent data in your database. Consider using atomic transactions.

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

  3. $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.

  4. 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.

  5. 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
    
  6. 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.)

  7. 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.

  8. 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)

  9. 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

  10. 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 that, while this indentation style might look attractive, it becomes a headache to maintain the alignment of the equals signs as variable names change and code is run through tools that substitute tabs for spaces and spaces for tabs. It is also hard to maintain as lines are moved among different parts of the program that have different levels of indentation.

\$\endgroup\$
  • \$\begingroup\$ Wow, this is seriously informative and exactly what I was looking for! Thanks. As an alternative to die(), would you recommend using something like: echo 'Caught exception: ', $e->getMessage(), "\n";? \$\endgroup\$ – jshjohnson Mar 22 '14 at 10:19
  • \$\begingroup\$ @jshjohnson: I'm happy that you've found it useful. I don't think that the exception message (which I guess contains some database-related message) would be useful for a user but anyway, test it and decide. (And don't forget to log it.) I've just written another answer here about the same topic, check #3. \$\endgroup\$ – palacsint Mar 22 '14 at 10:35
  • \$\begingroup\$ Just re-reading through this. Could you perhaps elaborate what you mean regarding a parameter object? If I declared a new Freelancers object, how would the function get the data passed to it? \$\endgroup\$ – jshjohnson Apr 7 '14 at 10:31
  • \$\begingroup\$ @jshjohnson: Huh, good question. Object relational mapping has its issues: en.wikipedia.org/wiki/Object-relational_impedance_mismatch. Anyway, I've put another link into the post and here are two ideas: create a builder; move the register method into the Freelancer class. In the latter case you might also get a better a API with a builder but it might not worth it. I guess it depend on the size of the project. \$\endgroup\$ – palacsint Apr 7 '14 at 20:41
  • \$\begingroup\$ As for point 4: Aside from stopping the flow of information (and in particular, error messages) to code that could do something useful with it, die() makes it more of a pain -- if not outright impossible -- to run automated tests (like, say, unit tests). I've run into this before; if you're not anticipating such ugliness, an error can cause the whole test suite to just stop dead in its tracks, possibly without even so much as a visible error message or nonzero exit code. \$\endgroup\$ – cHao Sep 10 '14 at 18:02

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.