2
\$\begingroup\$

I'm making a very basic user system, and this is what I have so far:

<?php


/* The class for constructing any user's information
 */

    class User {

        protected $userId, $email, $userGroup;

        protected function getEmail() {
            return $this->email;
        }

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


        public function __construct($userId, $email, $emailGroup) {
            $this->userId = $userId;
            $this->email = $email;
            $this->userGroup = $emailGroup;
        }

        // Method for checking whether user can view a specific page
        public function canViewPage($pageId) {
            $userGroup = new UserGroup($this->userGroup);
            if($userGroup->canViewPage($pageId)) {
                return true;
            }else{
                return false;
            }       
        }

        // Method for checking permissions
        public function hasPermissions($alias) {
            /* alias is the name of the permissions. ie can-book
            * create usergroup object based on current user's usergroup */

            $userGroup = new UserGroup($this->userGroup); 
            if($userGroup->hasPermissions($alias)) {
                return true; 
            }else{
                return false;
            }
        }

    }

    class UserMapper {
        // database connection
        private $db;

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

        public function findByUserId ($userId) {
            $userObject = new User();
            $q = $this->db->prepare("SELECT userId, email, userGroup FROM user WHERE userId = :userId");
            $q->bindValue(":userId", $id);
            $q->setFetchMode( PDO::FETCH_INTO, $userObject);
            $q->execute();
            $q->fetch(PDO::FETCH_INTO);
            return $userObject;

        }
    }   
?>

Then on my main script, I include the user.class.php (as shown above) and do this:

<?php 
    $dbh = new PDO('mysql:host=localhost;dbname=test', $user, $pass, array(PDO::ATTR_PERSISTENT => true));
    $getUser = new UserMapper($dbh);
    $user = $getUser->findByUserId(41);
    echo $user->getEmail();
?>

Is this all correct? Or have I misunderstood? Is there a way of simplifying this? I've tried to keep the code as simple as possible. Am I correct for having getters (getEmail, etc.) as well as methods such as canViewPage, within the user class?

Also, should it be:

$userGroup = new UserGroup($this->userGroup); 

or should I do something like this again?

$dbh = new PDO('mysql:host=localhost;dbname=test', $user, $pass, array(PDO::ATTR_PERSISTENT => true));
        $getUserGroup = new UserGroupMapper($dbh);
        $userGroup = $getUserGroupMapper->findByUserId($this->userGroup);

Is there a simpler way?

\$\endgroup\$
2
\$\begingroup\$

I didn't change much. I think you're correct in having getters as well as methods such as 'canViewPage'. The Methods 'canViewPage' and 'hasPermissions' are just wrappers around the 'UserGroup' class, the logic is in 'UserGroup'. It makes sense to create wrappers around 'UserGroup', for example if you want to have also Permission based solely on the User(Example a root user) then you would just have to change these Methods. If you had invoked the Methods directly on the 'UserGroup' - not over the 'User' - you would have to go over all these places. Pherhaps it would be Usefull to have a 'Permissions' class, wich would have a constructor or factory method, where you could give an 'User' as Argument.

<?php
/* The class for constructing any user's information
*/

class User {

    protected $userId, $email, $userGroup;

    protected function getEmail() {
        return $this->email;
    }

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


    public function __construct($userId, $email, $emailGroup) {
        $this->userId = $userId;
        $this->email = $email;
        // You never use userGroup directly, you can just 
        // put the object as membervariable, so you don't 
        // have to instantiate the object twice
        $this->userGroup =  new UserGroup($emailGroup);
    }

    // Method for checking whether user can view a specific page
    public function canViewPage($pageId) {
        /*
        if(condition){
            return true;
        else
            return false;
        is equals
        return (bool) condition
        */
        return (bool) $this->userGroup->canViewPage($pageId);
    }

    // Method for checking permissions
    public function hasPermissions($alias) {
        /* alias is the name of the permissions. ie can-book
        * create usergroup object based on current user's usergroup */
        return (bool) $this->userGroup->hasPermissions($alias);
    }

}

class UserMapper {
    // database connection
    private $db;

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

    public function findByUserId ($userId) {
        $userObject = new User();
        $q = $this->db->prepare("SELECT userId, email, userGroup FROM user WHERE userId = :userId");
        $q->bindValue(":userId", $id);
        $q->setFetchMode( PDO::FETCH_INTO, $userObject);
        $q->execute();
        $q->fetch(PDO::FETCH_INTO);
        return $userObject;

    }
}   
?>

I don't understand your Second Questions, pherhaps you made some typos? You have '$getUserGroup', but use '$getUserGroupMapper' and you give the method 'findByUserId' a UserGroup.

EDIT: Now I understand. If you have a lot of UserGroups, I think it would be good. If you have just around 5 UserGroups, I thinks it would be too much

Your main program is good, for being an example.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for your reply. Sorry I made a few typos. What I mean in my second question, is where you have the line $this->userGroup = new UserGroup($userGroup); - this is calling on the UserGroup class, which needs to retrieve information from the database also - in a similiar way I call on the User class, using a UserMapper class. So do I need two classes also for my UserGroup? (UserGroup class - which stores the information, UserGroupMapper -which retrieves it from MYSQL) Is the code for my "main" script correct also? \$\endgroup\$ – Tom Oct 14 '13 at 19:10
  • \$\begingroup\$ This code doesn't follow SOLID and is hard to unit-test. \$\endgroup\$ – Pinoniq Oct 15 '13 at 15:34

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.