1
\$\begingroup\$
class User {

    protected $DBH;
    protected $STH;

    public function __construct($DBH) {
        $this->DBH = $DBH;
    }

    public function logged_in() {
        if (isset($_SESSION['userid']) && isset($_SESSION['hash'])) {
            $hash = sha1($_SESSION['userid'] . $_SERVER['HTTP_USER_AGENT']);

            if ($_SESSION['hash'] == $hash)
                return true;
        }
        return false;
    }

    public function login($username, $password) {
        $password = sha1($password);
        $this->STH = $this->DBH->prepare("SELECT id, banned, activated FROM users WHERE username = ? AND password = ?");
        $this->STH->setFetchMode(PDO::FETCH_OBJ); 
        $this->STH->execute(array($username, $password));

        if ($this->STH->rowCount() > 0)
            return $this->STH->fetch();
    }

    public function create_account($username, $password, $email) {
        $password = sha1($password);
        $activation_key = md5($username . $email);

        $this->DBH->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

        try {
            $this->STH = $this->DBH->prepare("INSERT INTO users(username, password, email, activation_key, created) VALUES(:username, :password, :email, :activation_key, :created)");
            $this->STH->bindParam(':username', $username);
            $this->STH->bindParam(':password', $password);
            $this->STH->bindParam(':email', $email);
            $this->STH->bindParam(':activation_key', $activation_key);
            $this->STH->bindParam(':created', time());
            $this->STH->execute();
        } catch (PDOException $e) {
            //$e->getMessage();
            return false;
        }

        return true;
    }

    public function check_username($username) {
        $this->STH = $this->DBH->prepare("SELECT username FROM users WHERE username = ?");
        $this->STH->execute(array($username));

        if ($this->STH->rowCount() > 0)
            return false;

        return true;
    }

    public function check_email($email) {
        $this->STH = $this->DBH->prepare("SELECT email FROM users WHERE email = ?");
        $this->STH->execute(array($email));

        if ($this->STH->rowCount() > 0)
            return false;

        return true;        
    }

    public function activate($key) {
        $this->STH = $this->DBH->prepare("SELECT activation_key FROM users WHERE activation_key = ?");
        $this->STH->execute(array($key));

        if ($this->STH->rowCount() > 0) {
            $this->STH = $this->DBH->prepare("UPDATE users SET activated = 1, activation_key = null WHERE activation_key = ?"); 
            $this->STH->execute(array($key));

            return true;
        }

        return false;
    }  
}

Am I doing this right? What should I change? Add, etc. Should I use more try { } catch blocks/etc. Give me some tips on how to improve the code. (Better performance, etc).

\$\endgroup\$
3
  • 1
    \$\begingroup\$ It's quite impressive that you went from this to that so quickly. @vstm has excellent points, but your code is far from someone's who's unsure on OO. Nice. \$\endgroup\$
    – yannis
    Commented Dec 1, 2011 at 6:57
  • \$\begingroup\$ Thanks, anything I could improve? :) \$\endgroup\$
    – John
    Commented Dec 1, 2011 at 17:10
  • \$\begingroup\$ @vstm answer covers me, I've added a comment there about exception logging, something I recently enquired about on Programmers SE programmers.stackexchange.com/questions/121434/… \$\endgroup\$
    – yannis
    Commented Dec 1, 2011 at 17:23

1 Answer 1

1
\$\begingroup\$
  • Regarding try/catch: this depends on whether your user class can handle errors or not.

    • An exception you can't handle is for example if the SQL-statement has a syntax error or the SQL-server being down. You can't do anything about it programatically during runtime except for displaying a nice error message to the user. In that case I wouldn't surround the DB-code with try/catch but implement a global try/catch which does just display a nice error message to the user and log the errors for you, the developer.

    • But if you have an exception because for example your username-field is UNIQUEand someone tries to insert an username which already exists you could resolve that problem automatically by just adding a number to the username and retry the insert (okay it's a dumb example, but I hope the point comes across). The pseudocode for this dumb example could be like:

      $statement = $db->prepare("INSERT INTO user (username, ....) values (?, ...)");
      $tryInsert = true;
      $tries = 0;
      while($tryInsert) {
          $insertName = $username;
          if($tries > 0) {
              $insertName .= $tries;
          }
          try {
              $statement->execute(array($insertName, ...));
              $tryInsert = false; // it worked, stop trying
          } catch(PDOException $e) {
              if($e->code() == "23000") {
                  // 23000 means unique constraint violation, probably 
                  // duplicate username
                  ++$tries;
              } else {
                  // we can't handle the exception, therefore we just re-throw it
                  throw $e;
              }
          }
      }
      

    I think in your code are no exceptions which you could handle. If an exception occurs it is most probably due to some issue with the SQL-server or the SQL-statement is wrong. In that case it doesn't make sense to wrap any of your code in try/catch. But you should add some kind of global exception handler which displays a nice error message (like: oops something went wrong, the developers have been informed).

  • In login change the last two lines to:

    if (($row = $this->STH->fetch()) !== false)
        return $row;
    

    The problem with PDOStatement::recordCount() is, that it's not portable for SELECT queries and the above code does the same thing as your code.

  • Then in create_account, you use PDO::setAttribute to change the PDO error mode of the supplied PDO-instance. Change that so that this is called when the PDO is initialized and not deep down in some mapper-class.

  • Your activate method could be rewritten to just execute one query:

    public function activate($key) {
        $this->STH = $this->DBH->prepare("UPDATE users SET activated = 1, activation_key = null WHERE activation_key = ?"); 
        $this->STH->execute(array($key));
    
        // rowCount is set to the number of rows affected by UPDATE
        return $this->STH->rowCount() > 0;
    }
    
  • Optional: This could be simplified:

    if (isset($_SESSION['userid']) && isset($_SESSION['hash'])) {
    

    To that:

    if (isset($_SESSION['userid'], $_SESSION['hash'])) {
    
  • Optional: Consider using phpass for hashing your passwords. It's definitely more secure that just sha1 with some salt thrown in and the class is simple to use. If you're doing this you would have to rework your login method a bit (meaning the password verification must be done in PHP when using phpass).

  • If you like to change your check_*-methods you could do something like that:

    protected function check_record($field, $value) {
        $this->STH = $this->DBH->prepare("SELECT COUNT(*) FROM users WHERE {$field} = ?");
        $this->STH->execute(array($value));
    
        $count = $this->STH->fetchColumn();
    
        if($count !== false) {
            return (int)$count > 0;
        }
    
        return null;
    }
    
    public function check_username($username) {
        return $this->check_record("username", $username);
    }
    
    public function check_email($username) {
        return $this->check_record("email", $username);
    }
    
\$\endgroup\$
6
  • \$\begingroup\$ Could you rephrase/rewrite the first about try/catch I didn't really understand what you meant. Otherwise, thanks for a very good answer, I'll edit my code right away. \$\endgroup\$
    – John
    Commented Nov 30, 2011 at 19:15
  • \$\begingroup\$ If rowCount isn't portable with SELECT, does that mean I must change check_email, and check_username method? How? \$\endgroup\$
    – John
    Commented Nov 30, 2011 at 19:21
  • \$\begingroup\$ @John: thank you for your feedback. I have rewritten the part about try/catch - I hope it's clearer now. As for the check_* functions: I have added my version without rowCount it at the end of my answer. \$\endgroup\$
    – vstm
    Commented Nov 30, 2011 at 20:03
  • \$\begingroup\$ Thanks for the tips, Anything else you can think off? Else I will set this answer as correct... \$\endgroup\$
    – John
    Commented Nov 30, 2011 at 20:27
  • \$\begingroup\$ But how I can I log the "error" in example create_account method, If I don't do a try/catch there. \$\endgroup\$
    – John
    Commented Nov 30, 2011 at 21:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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