6
\$\begingroup\$

Originally, this isn't how I would have done this at all. I was told by someone I know who is a programmer that I needed to include several functions: one for removing and adding users, one for searching for users information etc.

I would have made it so you can pass a username into the constructor and then get all the user info and leave it to another UserManagement class to take care of things like adding and removing users, but I was told (very very forthrightly) that, that was NOT the way of going about doing what I'm doing.

Am I using OOP correctly? I have asked 2 people and their opinions are almost directly opposite to each others. The code below works fine, performs well and does what I want it to just fine, but I'm worried that I'm not quite getting when and when not to use OOP.

class User{
    public $username;
    public $uid;
    public $date;
    public $location;

    public function __construct(){
    }

    public function getUser($user){

        $getUserInfo = new Database;
        $getuserInfo("SELECT * FROM users WHERE username = ?", array($user));
        $result = $getUserInfo->result;

        foreach ($result as $value) {
            $this->username = $value['username'];
            $this->uid = $value['uid'];
            $this->date = $value['date'];
            $this->location = $value['location'];
        }
    }

    public function getUserByUID($uid){
        $getUserInfo = $GLOBALS['databaseUserManagement']->prepare("SELECT * FROM users WHERE uid = ?");
        $getUserInfo->execute(array($uid));
        $result = $getUserInfo->fetchAll();

        foreach ($result as $value) {
            $this->username = $value['username']; 
            $this->uid = $value['uid'];
            $this->date = $value['date'];
            $this->location = $value['location'];
        }
    }

    public function deleteUser($username){
        $deleteSpecifiedUser = $GLOBALS["databaseUserManagement"]->prepare("DELETE FROM users WHERE username = ?");
        $deleteSpecifiedUser->execute($user);
    }

    public function addUser($username, $password, $email, $ip){
        $cryptOptions = [
            'cost' => 11,
            'salt' => mcrypt_create_iv(22, MCRYPT_DEV_URANDOM),
        ];

        $passwordCrypted = password_hash($password, PASSWORD_BCRYPT, $cryptOptions);
        $email = $_POST['email'];
        $date = date("y-m-d h:m:s");

        $registerQuery = $GLOBALS["databaseUserManagement"]->prepare("INSERT INTO  `users` (`id` , `uid`, `username`, `password`, `date`,`ip`, `email`) VALUES (NULL,  ?, ?, ?,  ?,  ?, ?, ?)");
        $registerQuery->execute(array(uniqid(), $username, $passwordCrypted, $date, $ip, $email));
    }

}
\$\endgroup\$
5
  • \$\begingroup\$ I'm having trouble analyzing this wordy post. Are you just asking for a review of this code? \$\endgroup\$
    – Jamal
    Jul 20, 2014 at 0:52
  • \$\begingroup\$ Im not sure what Im asking but I suppose yeah. Im more or less asking am I using OOP correctly? \$\endgroup\$
    – DribbleUI
    Jul 20, 2014 at 0:53
  • \$\begingroup\$ Okay. I'll try to shorten this post to its essentials. \$\endgroup\$
    – Jamal
    Jul 20, 2014 at 0:54
  • \$\begingroup\$ This shouldn't be working... $getuserInfo("query", array); isn't correct syntax. Also execute($user): $user is never defined. Did you mean $username? \$\endgroup\$
    – Alex L
    Jul 20, 2014 at 1:03
  • \$\begingroup\$ Yeah apologies, few mistakes I forgot to fix. Which is why theres an object at the top for database management and not further down. I should have fixed it before hand. Apologies. \$\endgroup\$
    – DribbleUI
    Jul 20, 2014 at 1:13

2 Answers 2

5
\$\begingroup\$

No, this design is not object-oriented, nor would I recommend you follow your friend's advice. I also would not recommend your original design either. Neither one adheres very well to object-oriented design principles, namely the SOLID principles.

In your friend's rendition, he is proposing a structure that is very active record-like, where a domain object also knows how to persist and retrieve itself. While this pattern is used in many places (Ruby on Rails, CakePHP, etc), it can make testing very difficult, it can cause dependency problems, and it is not single responsibility.

It is difficult to test (especially your rendition) because the object is dependent on the database. Since you instantiate the database object in your method, there is no easy way to mock it or stub it out either.

Nor is there an easy way to substitute different implementations. Your class is tied to the concrete database implementation. It has a dependency that cannot be isolated.

The class has numerous responsibilities that are completely unrelated to representing a user. For example, in the getUser method, we are responsible for:

  1. Getting a database connection object,
  2. Building a query,
  3. Running that query,
  4. Fetching the results,
  5. Translating the results into object properties.

This code has a lot of fingers in a lot of pies, and depends on the specific functionality of a lot of other parts.

Your proposal of simply having a data object, and a separate object to manage persistence is more on track. It falls down a bit in the details, though. To be truly object-oriented, you want to encapsulate your data. Having all public members does not encapsulate anything. The most basic way to make this more object-oriented is to have getters and setters (some may debate this, and there are other routes that can be taken, but this is a good start). This way, if the internal structure of your class changes, you can hide it by adjusting the implementations of the encapsulating methods while preserving the public API.

The pattern you are looking for is the Repository pattern. It allows you to isolate and encapsulate mapping between a persistence layer and domain objects, and allows the domain objects to focus on the single responsibility of representing the business concept they represent.

I would also recommend you learn about dependency injection to help you to understand why instantiating objects inline can cause problems, and what one potential solution is.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you so much for sending me this. I will improve upon my practices and do another post once thats done. From a database side of things, should I have a separate file for the database class or should I keep it in the same file as all my other classes? \$\endgroup\$
    – DribbleUI
    Jul 23, 2014 at 15:01
1
\$\begingroup\$

As cbojar said, you need to separate your concerns. This is a blown out example of how your code should look like:

The User class should not have any SQL interaction - it should delegate SQL interaction to a class built to handle just SQL interaction.

class User {
    private $Model = null;

    public function __construct(){
        $this->Model = new UserModel;
    }

    public function create(){
        $user_data = array(
            "username" => "jsanc623",
            "password" => "alskdf23$@#af098sdja",
            "date" => "07/21/14",
            "location" => "NY"
        );

        $this->Model->create($user_data);
    }

    public function read($username = null, $id = null){
        return $this->Model($username, $id);
    }

    public function update(){
        $user_data = array(
            "username" => "jsanc623",
            "password" => "alskdf23$@#af098sdja",
            "date" => "07/21/14",
            "location" => "CA"
        );

        return $this->Model->update($user_data);
    }

    public function delete($username){
        $this->Model->delete($username);
    }

}

This class should handle only User related SQL interactions. It will then send those to the Database class which is shared over other models.

class UserModel {
    private $db;

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

    public function create($user_data){
        $this->db->create("users", $user_data)
    }

    public function read($username = null, $uid = null){
        $query = array();
        $query_root = "SELECT `username`, `uid`, `date`, `location` FROM users WHERE ";

        if(!is_null($username)){
            $query = array("username = ?", $username)
        } else if(!is_null($id)){
            $query = array("uid = ?", $uid)
        } else {
            throw new Exception("No query in UserModel::read()", 400);
        }

        $query[0] = $query_root . $query[0];

        return (array)$this->db->query($query);
    }

    public function update($user_data_assoc_array){
        $this->db->update("users", $user_data_assoc_array);
    }

    public function delete($username){
        $this->db->delete("users", "username", $username);
    }
}

This is the Database class, which is dumb enough to just run queries and return an array with results.

class Database{
    $connection = null;

    public function __construct(){
        # Start your PDO connection here
    }

    /**
     * $query = array("SQLQuery", "variable data")
     */
    public function query($query){
        # Run your PDO query here
    }

    public function delete($table, $column, $data_array){
        # Run your PDO update query here
    }

    public function update($table, $data_array){
        # Run your PDO update query here
    }
}

This example ties more towards an MVC method though. I would take cbojar as the best answer.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ While getting a lot closer in the right direction, this has a lot of problems with it. Dependencies are not injected (new in a constructor is an anti-pattern), and there's a strange relationship between User and UserModel. User doesn't seem to have any clear purpose (why not just use UserModel?). \$\endgroup\$
    – Corbin
    Jul 21, 2014 at 21:30
  • \$\begingroup\$ As I said - this is leaning more towards an MVC type of structure, where User is the controller and UserModel is the Model. Sorry about the anti-pattern...bad habit I've picked up lately. As I said - this is solely to illustrate a bit of what cbojar said in his answer (not having the SQL in the class itself, etc). \$\endgroup\$
    – jsanc623
    Jul 21, 2014 at 21:36

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.