0
\$\begingroup\$

The function below simple allows a logged in user to "star" another user for convenience when trying to find the that user another time. At the moment the function works as it should, it inserts the user giving the star and the user receiving the star into a database table, then grabs the first name and email from the users table to get more information about the user receiving the star and finally uses that data to send an email to the user receiving the star.

As it stands however, this function is pretty slow when used. Based on user research, users tend to press the "Star user" button numerous times as the application seems unresponsive; when in reality, it is just taking longer as it is sending an email (I've tested the function without the email trigger and it is much faster).

Is there a better way of triggering emails to increase performance using this as an example? Any improvements/recommendations would also be appreciated.

public function addstar($user_id, $starredBy) {

    $query = $this->db->prepare("INSERT INTO " . DB_NAME . ".user_stars (star_id, starred_by_id) VALUES (?, ?)");

    $query->bindValue(1, $user_id);
    $query->bindValue(2, $starredBy);

    $this->db->beginTransaction();

    try{
        $query->execute();

        $getUserInfo = $this->db->prepare("SELECT u.firstname, u.email, u.user_id FROM " . DB_NAME . ".users AS u WHERE u.user_id = ?");
        $getUserInfo->bindValue(1, $user_id);
        $getUserInfo->execute();
        $to = $getUserInfo->fetch();

        $firstName = $to['firstname'];
        $email = $to['email'];

        $this->emails->sendStarEmail($firstName, $email);

        $this->db->commit();

        header('Location: ' . $_SERVER['HTTP_REFERER']);

    }catch(PDOException $e) {
        $this->db->rollback();
        $users = new Users($db);
        $debug = new Errors();
        $debug->errorView($users, $e);    
    }
}
\$\endgroup\$
1
\$\begingroup\$
  • Lock the UI until a response comes back (or a reasonable timeout occurs) to prevent the multiple clicking.
  • Don't send emails directly from the code, push them to spool or MQ and process separately

As for the code itself - there's nothing much to comment except that it's completely out of touch with modern best practices in PHP world. Consider using a proper framework, ORM or at least a DBAL. See phptherightway for more details.

\$\endgroup\$
  • \$\begingroup\$ Could you perhaps elaborate on how it is "completely out of touch" - what parts? \$\endgroup\$ – jshjohnson May 11 '14 at 11:57
  • \$\begingroup\$ It's not so much a specific "part" I can point to and say "you do a(b);, but should do b(a);", it's just fundamentally dated. That said, here's a couple of points: using PDO instead of proper model layer; using classes that handle way too much; using header(); (and inside what's supposed to be a model at that); from the code piece I can also infer (but might be wrong) that you're not running MVC architecture, you're not separating concerns of your classes & you're not applying proper design patterns within class architectures. \$\endgroup\$ – Inoryy May 11 '14 at 12:32

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.