Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am very new to OOP and I am sure this code is very, very bad. I will appreciate if you review my classes and give me some advice to improve them. Any recommendations are welcome. I have also put some additional questions in the code and I hope you can give me an answer for some of them.

Database.php

<?php
class Database 
{
    private static $dsn = 'mysql:host=localhost;dbname=knight;charset=UTF-8';
    private static $user = 'root';
    private static $pass = '';
    private static $instance = null;

    private function __construct()
    {}

    //is PDO+Singleton a good combination?
    public static function getInstance()
    {
        if(!isset(self::$instance))
        {
            try 
            {
                self::$instance = new PDO(self::$dsn, self::$user, self::$pass);
                self::$instance->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            }
            catch (PDOException $e)
            {
                echo 'Error found: '.$e->getMessage();
            }
        }
        return self::$instance;
    }

    //Should i make methots prepare(), execute(), bindValue() etc. and why?
}
?>

Validator.php

<?php
class Validator
{
    //All the methods are static, is this a good practice for a class like this?

    public static function isValidMail($var)
    {
        return preg_match("/([\w\-]{3,40}+\@[\w\-]{3,120}+\.[\w\-]{2,6}+)/", $var);
    }

    public static function isLong($var, $max)
    {
        return(mb_strlen($var, 'UTF-8') > $max);
    }

    public static function isShort($var, $min)
    {
        return (mb_strlen($var, 'UTF-8') < $min);
    }

    public static function isValidID($var)
    {
        $var = (int)$var;
        return ($var > 0);
    }
}

User.php

<?php
class User 
{
    //Should I have properties such as username, password, email etc. and why?

    private $db = null;
    private $errors = array();

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

    public function register($username, $pass, $mail)
    {
        if($this->validate($username, $pass, $mail))
        {
            $username = $this->filter($username);
            $mail = $this->filter($mail);
            $exists = $this->exists($username, $mail);
            if(!$exists)
            {
                $pass = $this->hash($pass);
                $time = time();

                //Should I use exceptions for the SQL queries in this class?

                $rs = $this->db->prepare("INSERT INTO users(username, mail, pass, date_reg) VALUES(:name, :mail, :pass, :time)");
                $rs->execute(array(':name' => $username, ':mail' => $mail, ':pass' => $pass, ':time' => $time));
                header('Location: index.php');
                die();
            }
            else
            {
                $this->errors[] = 'Username or email already exists!';
            }
        }

        if(count($this->errors > 0))
        {
            $this->showErrors();
        }
    }

    public function login($username, $pass)
    {
        if($this->validate($username, $pass))
        {
            $username = $this->filter($username);
            $exists = $this->exists2($username, $pass);
            if($exists)
            {
                $_SESSION['logged'] = 1;
                $_SESSION['data'] = $exists->fetch(PDO::FETCH_ASSOC);
                header('Location: index.php');
                die();
            }
            else
            {
                $this->errors[] = 'Wrong username or password!';
            }
        }

        if(count($this->errors > 0))
        {
            $this->showErrors();
        }
    }

    public function exists($username, $mail)
    {
       $exists = $this->db->prepare("SELECT user_id FROM users WHERE username = :name OR mail = :mail");
       $exists->execute(array(':name' => $username, ':mail' => $mail));
       return ($exists->rowCount() > 0);
    }

    public function exists2($username, $pass)
    {
       $pass = $this->hash($pass);
       $exists = $this->db->prepare("SELECT * FROM users WHERE username = :name AND pass = :pass");
       $exists->execute(array(':name' => $username, ':pass' => $pass));
       if($exists->rowCount() > 0) return $exists;
       return false;
    }

    public function validate($username, $pass, $mail = null)
    {
        if(Validator::isShort($username, 4)) $this->errors[] = 'Short username!';
        elseif(Validator::isLong($username, 25)) $this->errors[] = 'Long username!!';
        if(Validator::isShort($pass, 5)) $this->errors[] = 'Short password!';
        if($mail !== null)
        {
            if(!Validator::isValidMail($mail))
            {  
               $this->errors[] = 'Invalid email!';
            }
        }
        return (count($this->errors) == 0);
    }

    public function getUserData($id)
    {
        if(Validator::isValidID($id))
        {
            $rs = $this->db->prepare("SELECT * FROM users WHERE user_id = ?");
            $rs->execute(array($id));
            if($rs->rowCount() == 1) return $rs->fetch(PDO::FETCH_ASSOC);
            return false;
        }
        return false;
    }

    private function hash($var)
    {
        return hash('sha512', $var);
    }


    public function filter($var)
    {
        //Is htmlentities enough if I use PDO?
        return htmlentities($var, ENT_QUOTES, 'UTF-8');
    }


    private function showErrors()
    {
        foreach($this->errors as $e)
        {
            //I am outputting text in the class, is that bad? 
            echo '<strong>'.$e.'</strong><br />';
        }
    }
}

Thank you in advance!

share|improve this question
add comment

1 Answer

up vote 1 down vote accepted

Let's first create some test code to show how this might all work to say register a new user. In your controller code to register a user which would accept POST :

$name = $_POST['name'];
$password = $_POST['password'];
$email = $_POST['email'];

$db = Database::getInstance();
$user = new User($db);
$user->register($name, $password, $email);

Right off the bat, I see that I have to pass name, pw, email to the register method. Some improvement:

$name = $_POST['name'];
$password = $_POST['password'];
$email = $_POST['email'];

$db = Database::getInstance();
$user = new User($db);
$user->setName($name);
$user->setPassword($password);
$user->setEmail($email);

if ($user->register() !== true) {
    //show some sort of error page, utilize the $user object in your view
    //i.e. foreach ($user->getErrors() as $error)...
} else {
    //show the success result page, utilize the $user object in your view
}

The register method can perform validations as necessary and insert the appropriate db record.

Onto the Database class. Hard coding the connection parameters is not very flexible. There's no real behavior behind your Database class other than a factory method to create an instance of itself. I think you'd be better off with a PDOFactory class which can accept a configuration (your db connection params) and return a single instance of a PDO object. Then, use the PDO object where needed.

The Validation class could be extended to provide what you now have for static methods:

abstract class Validator
{
    protected $value;

    public function setValue($value)
    {
        $this->value = $value;
    }

    abstract public function isValid();
}

class EmailValidator extends Validator
{
    public function isValid()
    {
        //implement your validation code and return true or false
    }
}

Create subclasses for the rest.

All this said, you should know that there are several frameworks which implement all of this already. I strongly recommend you look at either Zend Framework or Symfony. They've already thought of everything you are asking about here and more.

share|improve this answer
    
Thank you very much for the answer, Rob! What do you think about the exceptions - when should I use them in these classes? –  Stanimir Dec 3 '12 at 20:09
    
@Stanimir I am of the opinion that Exceptions should be thrown for, well, exceptional situations. Examples are: failed to connect to a service/database or unable to write to file - for things that normally work but in some instance did not. –  Rob Apodaca Dec 4 '12 at 1:24
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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