User lib php v2

Question

I've been working some forth looking on design patterns, and I would like some help to improve the code (fixes, tips, just generally improvements, or things you consider bad practise, etcetera in my code).

And finally: Am I on the right track, or should just back the wheel and start over?

UserMapper

<?php

class UserMapper {
    protected $db;

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

    public function save(User $user_object)
    {
        $data = array(
            'username' => $user_object->username,
            'email' => $user_object->email,
            'password' => $user_object->password,
        );

        if (is_null($user_object->id)) {
            $data['salt'] = $user_object->salt;
            $data['created'] = time();
            $sth = $this->db->prepare("INSERT INTO users (username, email, password, salt, created) VALUES (:username, :email, :password, :salt, :created)");  
            $sth->execute($data);
        } else {
            $data['id'] = $user_object->id;
            $sth = $this->db->prepare("UPDATE `users` SET username = :username, email = :email, password = :password WHERE `id` = :id");
            $sth->execute($data);
        }
    }

    public function getUserById($id)
    {
        $sth = $this->db->prepare("SELECT * FROM users WHERE id = ?");
        $sth->execute(array($id));

        $sth->setFetchMode(PDO::FETCH_OBJ); 

        if ($sth->rowCount() == 0) {
            throw new Exception('User not found');
        }

        $row = $sth->fetch();
        $user_object = new User($row);

        return $user_object;
    }
}

?>

User:

<?php

class User {

    private $id;
    private $username;
    private $email;
    private $password;
    private $salt;
    private $created;

    public function __construct($user_row = null)
    {
        if (!is_null($user_row)) {
            $this->id = $user_row->id;
            $this->username = $user_row->username;
            $this->email = $user_row->email;
            $this->password = $user_row->password;
            $this->salt = $user_row->salt;
            $this->created = $user_row->created;
        }
    }

    public function __set($name, $value)
    {
        switch ($name) {
            case 'password':
                $value = sha1($value . $this->salt);
                break;
        }

        $this->$name = $value;
    }

    public function __get($name)
    {
        return $this->$name;
    }  
}

Auth...

A empty class with logged_in, login, logout, methods.

function save(User $user) { $data = array();

$data['username'] = $user->username;
$data['password'] = $user->password;

if (is_null($user->id)) {
    $data['salt'] = $user->salt;

    $columns = implode(', ', array_keys($data));
    $values = implode(', :', array_keys($data));

    try {
        $sth = $this->db->prepare("INSERT INTO `users` ($columns) VALUES (:$values)");
        $sth->execute($data);
    } catch (PDOException $e) {
        echo $e->getMessage();
    }
} else {
    $data['id'] = $user->id;

    $dataSet = '';
    foreach (array_keys($data) as $key) {
        $dataSet .= "$key = :$key, ";
    }
    $sth = $this->db->prepare("UPDATE `users` SET $dataSet WHERE `id` = :id");
    $sth->execute($data);
}

}

test: (all files needed included)

$host = 'localhost';
$dbname = 'code';
$user = 'root';
$pass = '';


try {
  $db = new PDO("mysql:host=$host;dbname=$dbname", $user, $pass);
}
catch(PDOException $e) {
    echo $e->getMessage();
}



$user = new User;

$user->username = 'A123123ro';
$user->password = 'h1231233';
$user->salt = 'hej';
$user->email = 'j213123e';

$user_mapper = new UserMapper($db);

$user_mapper->save($user);

Answer 1

Not much "wrong" that I can see. Maybe a couple of improvements.

I just did the following with another post and liked how it turned out. It looks cleaner to me and requires less repitition. After $data is completely set, do the following.

$users = implode(',', array_keys($data));
$values = implode(',:', array_keys($data));

$sth = $this->db->prepare("INSERT INTO users ($users) VALUES (:$values)");

And, if you do the following after you initially set the $data array, but before you add anything else to it, it will only have the values you want to use in your SQL SET statement. So you can use that to set that string as well. It wont hurt anything to have this set even if it isn't used, so don't worry about putting it in an if/else statement.

$dataSet = array();
foreach(array_keys($data) as $key) {
    $dataSet[] = "$key = :$key";
}
$dataSet = implode(',', $dataSet);

Finally, I would set up the user information in the User class to use an array rather than numerous variables. It will make it easier to access and more extensible if you were to ever add another field to it later. And doesn't require too much change to your current code.

public $userInfo = array();

public function __construct($user_row = null)
{
    if (!is_null($user_row)) {
        foreach($user_row as $key => $value) {
            $this->userInfo[$key] = $value;
        }
    }
}

So you'd then do the following in UserMapper to access it. Of course there are a few other minor changes you'll have to do, but I'm sure you will spot them.

$data = $user_object->userInfo;

And of course this last suggestion will mean that the $dataSet suggestion I mentioned above wont work anymore. You can either set a new array for this information, or find some way of separating it from the new array, or you could just go back to manually typing it. If I think of anything for this, I'll let you know :)

Final thoughts: If you are going to be overloading the get and set methods for the User class, why did you bother making the variables private? Just make them public, then you won't need to overload the get method and can continue to use your set method. Of course this is just a thought, I don't know if what you are doing makes sense for your program, or if there are other benefits of doing it this way. To me though, it just seems a little pointless. Good luck!

UPDATE:

Here's one last thing I noticed, in your save() function you use the prepare() and execute() functions twice, merely separated by an if/else statement. This is redundant, you could just save the SQL statement as a string and then run it once at the end of the function.

if(is_null($user->id)) {
    //rest of code
    $statement = "INSERT INTO `users` ($columns) VALUES (:$values)";
    //rest of code
} else {
    //rest of code
    $statement = "UPDATE `users` SET $dataSet WHERE `id` = :id";
    //rest of code
}

$sth = $this->db->prepare($statement);
$sth->execute($data);

Answer 2

There's no verification that the sql was executed correctly in your UserMapper::save method. It just assumes that everything went ok after execute (actually all your execute calls go un-checked). Always verify your results. You should at the very least be wrapping all your pdo executes in if statements as it returns true or false as stated in the php pdo docs