0
\$\begingroup\$

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);
\$\endgroup\$
1
\$\begingroup\$

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);
\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback, I will consider your points well, but do you think my script make sense, is it something I can go and continue develop on? Is it a smartway ? \$\endgroup\$ – John Apr 26 '12 at 9:02
  • \$\begingroup\$ @John: Yes, what you have is perfectly fine. I was only pointing out a few things that would make expanding it easier. You should take a look at some of my other answers on this site, yours is by far the smallest because there was little to add :) \$\endgroup\$ – mseancole Apr 26 '12 at 12:59
  • \$\begingroup\$ Does it make sense to have an Auth class, with the log in, forgot password, etcetra methods, or should i rather put them in UserMapper class? \$\endgroup\$ – John Apr 26 '12 at 13:50
  • \$\begingroup\$ Also a problem with the UPDATE, would be: UPDATE users SET username = :username, password = :password, WHERE id = :id as you can see there is a comma after the last SET value.. :) \$\endgroup\$ – John Apr 26 '12 at 16:10
  • \$\begingroup\$ I would add those methods to the UserMapper class as that class is already accessing the database and doing tasks that will be necessary for both. But that is just me, its really up to you. Sorry about that UPDATE bit, I have updated that to a workable solution. \$\endgroup\$ – mseancole Apr 26 '12 at 16:20
0
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ Can you give me some tips on verification? \$\endgroup\$ – John Apr 26 '12 at 9:03

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.