OOP Review & Tips

[zealer]

Hi all,
I'm working on a site for myself, and in the meantime, am trying to implement it in an OOP fashion. It isn't a complicated site in terms of what it needs to do, but it is complicated for me in terms of figuring out and structuring all this OOP stuff!
The area in development is an area for my clients where they can view what jobs I have done for them. Essentially, it is a work/job log.
I.e: say I painted Joe's house, when he logs in, it'll show that in the log specific to him (along with some additional details, like how much it cost, how much he paid me, his balance, the date performed).
say I cut the grass and walked Mary's dog, each job will be entered separately and both will show up for Mary when she logs in.

With this in mind, these are the basic features my site has or will need.

1. Basic user system (login, logout - only admin creates accounts)
2. Work task (add task, remove task, show task log to client)
   

Here's what I've come up with for the user system (so far). It logs a user in/out, checks if they are logged in (for page access control), and has an admin child class that will be developed.

class User {
    
    protected $dbh;
    protected $userId;
    protected $userEmail;
    protected $userPassword;
    protected $userInfo;
    protected $dbTable;


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

    //We can log the user in if their account exists, their password matches, and they are not already logged in.
    function userLogin(){
        if($this->userExists() AND $this->userPasswordMatch()){
            $this->queryUserInfo();
            return TRUE;
        }else{
            return FALSE;
        }
    }

    //session for userIsLogged should be 1. If not, user is not logged in. Static
    public static function userIsLogged(){
        if(isset($_SESSION['user']['logged'])){
            if($_SESSION['user']['logged'] == 1){
                return TRUE;
            }
        }else{
            $_SESSION['user']['logged'] = 0;
        }
        return FALSE;
    }

    //Method is static so that it can be accessed without having to initiate a user object. It does not control any objects within the user class, just the login session.
    public static function userLogout(){
        if(self::userIsLogged()){
            $_SESSION['user']['logged'] = 0;
            $_SESSION['user']['id'] = NULL;
        }
    }

    function setUserId($userId){
        $this->userId = $userId;
    }

    function getUserId(){
        return $this->userId;
    }

    function setUserEmail($userEmail){
        $this->userEmail = $userEmail;
    }

    function setUserPassword($userPassword){
        $this->userPassword = $userPassword;
    }

    function setUserTable($dbTable){
        $this->dbTable = $dbTable;
    }

    function setUserInfo($userInfo){
        $this->userInfo = $userInfo;    
    }

    function getUserInfo(){
        return $this->userInfo;
    }

    
    protected function userExists(){
        //Check if the user exists
        $sql = "SELECT COUNT(*) FROM {$this->dbTable} WHERE client_id=? OR client_email=?";
        $q = $this->dbh->prepare($sql);
        $q->execute(array(':client_id'=>$this->userId,':client_email'=>$this->userEmail));
        
        
        if($q->fetchColumn() == 1){
            return TRUE;
        }else{
            return FALSE;
        }
    }

    protected function userPasswordMatch(){
        //Check that the password matches
        $sql = "SELECT COUNT(*) FROM {$this->dbTable} WHERE client_email=? AND client_password=?";
        $q = $this->dbh->prepare($sql);
        $q->execute(array($this->userEmail,$this->userPassword));

        if($q->fetchColumn() == 1){
            return TRUE;
        }else{
            return FALSE;
        }
    }

    /* Grab user related data. If object of User::, exclude password and check based on login, for later storage in SESSION. If obj of Admin, take all info based on userId (set via setUserId)
        Used at login (called by userLogin), but left public so admin can use. Is this polymorphism? I doubt it...
    */
    function queryUserInfo(){
    
        if(get_class($this) == 'Admin'){
            $sql = "SELECT * FROM {$this->dbTable} WHERE client_id=?";
            $query_array = array($this->userId);
        }else{
            //not an admin user, so we assume (not good to assume...) it is a regular user object
            $sql = "SELECT client_id,client_name,client_phone,client_email,client_address,client_balance,client_frequency FROM {$this->dbTable} WHERE client_email=? AND client_password=?";
            $query_array = array($this->userEmail,$this->userPassword);
        }
        
        $q = $this->dbh->prepare($sql);
        $q->execute($query_array);
        $result = $q->fetch(PDO::FETCH_ASSOC);
        
        //set userInfo data
        $this->setUserInfo($result);     
    }

}

class Admin extends User {
    function createUser(){

    }

    function deleteUser(){

    }

    /* some other adminy things that only admins can do... */

}

And the login page to process

session_start();
include(dirname(dirname(__FILE__)).'/config.php');
include(INCLUDE_PATH.'/class/user.class.php');
include(INCLUDE_PATH.'/views/login.form.php');

if(isset($_GET['mode']) AND $_GET['mode'] == 'logout'){
    User::userLogout();
}

if(isset($_POST['login_submit'])){
    $user = new User($dbh);
    $user->setUserTable('bccl_clients');
    $user->setUserEmail($_POST['user_email']);
    $user->setUserPassword($_POST['user_password']);

    if($user->userLogin()){
        $userInfo = $user->getUserInfo();
                
        //Rip table prefix out of name based on underscore. i.e: table_name -> name
        $_SESSION['user']['logged'] = 1;
        
        foreach($userInfo as $k => $v){
            $n_k = explode('_', $k)[1]; // new key for session array
            $_SESSION['user'][$n_k] = $v;
        }
        
    }else{
        echo 'Incorrect email or password. Please try again.';
    }

}
//Only show form if not logged in.
if(!User::userIsLogged()){
    echo $form;
}else{
    header("Location: customerlog.php");
}

It works very well (at least to me, and what I need it for), but I feel like I'm just writing normal functions and putting them in a class. I feel like they should be more abstract, but can't really specify why.
Have I done a decent job grasping OOP? Where could I improve?
I haven't written any of the other classes yet (work task class, client note class -> although I'm thinking both could be subclasses of a more generic class, because they would have similar functions. add note, add task, remove note, remove task etc. Really, the only difference would be the corresponding SQL).

I was debating whether a session class is worthwhile, but I don't think it would be. There will only be 5-6 pages to the site, if that. This is mostly an exercise for me in order to practice OOP. Doing the same procedurally wouldn't be a problem. Maybe it is better to do it procedurally with functions for such a small site, but I'd like to practice my OOP. So that's what I'm doing (or attempting to!)

Thoughts?

P.S: Oh, I should mention, I am aware that I have not checked/validated any of the user inputs, nor have I setup any error checking/catching. I have left it out for the moment (but will add it in). For now, I'm only testing on my own machine.

[zealer]

This is probably better off in the application design board... Oops

[Rifts]

It looks like you understand it pretty well. Do you know anything about mvc? I think it will fill that "whats missing" feeling. Check out cakephp or even codeigniter. 

[zealer]

I know a little about MVC, generally what it stands for. I'm trying to implement it on this site as well, in a basic fashion as recommended/outlined by the book I'm reading/referencing (PHP Advanced and Object-Oriented Programming - Larry Ulman).

 

Basically I've got my classes as the models, intermediate scripts as the controls, and finally a basic template as the view. I'm including the proper view into the controller.

Ex: user.class.php (model) <--- userlogin.php (controller) --> login.form.php (view)

 

This make sense? It probably is a little primitive.

[ignace]

It looks like you understand it pretty well.

I disagree.

 

1) The User object should not be aware of the database.

2) Login is not a User concern, but an application concern so it should be handled by the application or a service.

3) $_SESSION is also an application concern and should not be used inside User.

4) Your User object is nothing more but a simple data container (set/get) so you end up with code like:

 

if ($user->getSomething() == 'somethingelse') $user->setSomethingElse('toSomething');

If the object owns the data, then it should control all actions done to it.

 

Let's take it from the beginning. At the start your client only asks for a quote to do a job for him:

 

$request = $client->askQuote('task description'); // { return new RequestForQuote($this, $description); }
$repo->persist($request);

To which you create a quote and reply your client:

 

$quote = $request->createQuote($dateStart, $workHours, $price); // { return new Quote($this->client, $this->description, $dateStart, $workHours, $price); }

Which your client accepts/rejects:

 

$job = $client->accept($quote); // { return $quote->won(); }
$repo->persist($job);

$client->reject($quote, $reason); // { return $quote->lost($reason); }

I think you get the point.

[KevinM1]

This is probably better off in the application design board... Oops

 

Yup.

[zealer]

I disagree.1) The User object should not be aware of the database.2) Login is not a User concern, but an application concern so it should be handled by the application or a service.3) $_SESSION is also an application concern and should not be used inside User.4) Your User object is nothing more but a simple data container (set/get) so you end up with code like: 

if ($user->getSomething() == 'somethingelse') $user->setSomethingElse('toSomething');

If the object owns the data, then it should control all actions done to it.Let's take it from the beginning. At the start your client only asks for a quote to do a job for him: 

$request = $client->askQuote('task description'); // { return new RequestForQuote($this, $description); }
$repo->persist($request);

To which you create a quote and reply your client: 

$quote = $request->createQuote($dateStart, $workHours, $price); // { return new Quote($this->client, $this->description, $dateStart, $workHours, $price); }

Which your client accepts/rejects: 

$job = $client->accept($quote); // { return $quote->won(); }
$repo->persist($job);

$client->reject($quote, $reason); // { return $quote->lost($reason); }

I think you get the point.

Thanks for the response. I had a suspicion in the back of my head that passing the DB object into the user class is probably not good form. Unfortunately, it is a method used in another book I've been reading for code references (5 PHP Applications for Dummies - Janet Valade). This stuff is tough to learn because there are so many alternative methods to achieve the same effect.

 

Moving on, could you expand on #2 and #3? I don't follow :/

[ignace]

2) Login is not a User concern, but an application concern so it should be handled by the application or a service.

To understand this, you need to understand what authentication is. It is your application acknowledging he knows the User. The mechanism used to verify this fact may differ thus meaning that each time you add a new authentication mechanism (twitter, facebook, restful API, ..) you would need to alter your User object which violates the Single Responsibility Principle which states that an object should have only one reason to change while when you couple it with authentication it needs to change whenever you need to make changes to the User class or when your authentication mechanism changes.

 

3) $_SESSION is also an application concern and should not be used inside User.

This should be really obvious. $_SESSION only exists when using your application over HTTP, as soon as you start using it across other platforms it is no longer available.