1
\$\begingroup\$

I have a table of users, where each user has a group (or multiple groups) he/she is part of.

Currently I'm handling the users group via a Users class:

<?php
/**
 *
 *  User (class) Model: Handles users (not system users).
 *
 */
class User{

/*=================================
=            Variables            =
=================================*/

    # Database instance
    private $db;

    # Users ID
    public $user_id;

    # User Realm
    public $realm; 

    # User Name 
    public $user_name;

    # User display name 
    public $display_name;

    # User creation
    public $created; 

    # User last seen
    public $last_seen; 

    # Is user deleted? 
    public $deleted; 

    # Is user ignored? 
    public $ignored; 

    # Is user blacklisted
    public $blacklisted;

    # Is user whitelisted
    public $whitelisted;

    # Is user blocked
    public $blocked;

    # Is user filtered
    public $filtered;

    # User Group 
    public $group = array();


/*===============================
=            Methods            =
================================*/


    /**
     *
     *  Constructor
     *  @param      $user_id    Init        User id. 
     *  @throws                 Object      User object of a single user.
     *
     */
    public function __construct($user_id) {

        # Get database instance
        $this->db           = Database::getInstance();

        # If user_id isn't passed 
        if ( $user_id ) {

            # Get this user by id
            $this->get_user_by_id($user_id);
        }
    }


    /**
     *
     *  Get user by id
     *  @param      $id     Init        User id 
     *  @throws             Object      Returns the object with data applied. 
     *
     */
    private function get_user_by_id($id) 
    {
        if ($id) {

            # Search for the user in the Database 'users' table. 
            $data   = $this->db->row("SELECT user_id, realm, user_name, display_name, created, last_seen, deleted, ignored, blacklisted, whitelisted, blocked, filtered FROM users WHERE user_id = :id", array('id' => $id));

            # If there is a result
            if ( $data ) {

                # Get group/s
                // -- Get Group method  -- 

                # Set data in this user object
                $this->data($data);

                return $this;

            } else { return false; }

        } else {
            return false; 
        }
    }


    /**
     *
     *  Insert Data to this object 
     *  @param      $data       Array       Gets a result array of user data
     *
     */
    private function data($data)
    {
        # Set data for this user object
        $this->user_id      = $data['user_id'];
        $this->realm        = $data['realm'];
        $this->user_name    = $data['user_name'];
        $this->display_name = $data['display_name'];
        $this->created      = $data['created'];
        $this->last_login   = $data['last_seen'];
        $this->deleted      = $data['deleted'];
        $this->ignored      = $data['ignored'];
        $this->blacklisted  = $data['blacklisted'];
        $this->whitelisted  = $data['whitelisted'];
        $this->blocked      = $data['blocked'];
        $this->filtered     = $data['filtered'];
        // $this->group         = $data['group'];
    }

I have just added the group variable, and not sure if it's "good practice" to add a fetch_group_by_user_id() method (separate table in my DB), or create a new group object, and use it inside my User class.

My user class is pasted ^

Please review my User class code and update me if you think it can be written better, and please tell me what is best to fetch the group.

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

Logical inconsistencies

You create a User object expecting a user_id to be passed to setup the object, id object to this design but im sure you've got it covered, but then later on you ask for a user_id to run a query?

I would leave it in the constructor and use type hiniting (php 7.0 +) to make sure it passed on creation

public function __construct(int $userId) {
    $this->userId = $userId;
    $this->get_user_by_id();
}    

But the type hint should make sure an exception is thrown if it isn't provided correctly!

PSR2

The PSR2 is a great coding style guide for php it says you should use camelCase for variable names & function names you should look into it.

Returning early

You should return early where you can this makes your code easier to read and less indentation levels

private function get_user_by_id($id) 
{
    if ($id) {
        return false;
    }

    $data   = $this->db->row("SELECT user_id, realm, user_name, display_name, created, last_seen, deleted, ignored, blacklisted, whitelisted, blocked, filtered FROM users WHERE user_id = :id", array('id' => $id));

    if ( !$data ) {
        return false;
    }

    $this->data($data);

    return $this;    
}

Adding more functions to setup a user

I would create a Service class for your user as your current class is doing both Model work (connecting to the database) and being a "Object".

<?php

namespace SomeNamespace\Services;

use SomeNamespace\Models\Users\GetUserDetails;
use SomeNamespace\Models\Users\GetUserGroupDetails;

class User
{
    // Properties list

    public function __construct(
        GetUserDetails $getUserDetails,
        GetUserGroupDetails $getUserGroupDetails
    ) {
        $this->getUserDetails = $getUserDetails;
        $this->getUserGroupDetails = $getUserGroupDetails;
    }

    public function populateData(int $userId)
    {
        $userDetails = $this->getUserDetails->get($userId);

        if(empty($userDetails)){
            throw new \Exception("Missing user for id $userId", 1);    
        }

        $groupDetails = $this->getUserGroupDetails->get($userId);

        $this->setupUserDetails($userDetails);
        $this->setupUserGroupDetails($groupDetails);

        return $this;
    }

    private function setupUserDetails($userDetails)
    {
        $this->someUserDetails = $userDetails["something"];
    }

    private function setupUserGroupDetails($groupDetails)
    {
        $this->someGroupDetail = $groupDetails["something"];
    }
}

// GetUserDetails.php

namespace SomeNamespace\Models\Users;

class GetUserDetails
{
    public function __construct()
    {
        $this->db = some::db::factory();
    }

    // Lots of sql queries
}

// GetGroupDetails.php

namespace SomeNamespace\Models\Users\Groups;

class GetUserGroupDetails
{
    public function __construct()
    {
        $this->db = some::db::factory();
    }

    // Lots of sql queries
}

I may be wrong as you seem to be using factories to construct objects instead of dependency injection, but im sure you see where im going with it!

\$\endgroup\$
  • \$\begingroup\$ Hi Dan! I'm exactly applying this change on my code (finally!) and I was wondering if you have any idea from where I can find more examples for OOP programming for all sorts of situations. (examples from what I am working on: situations where fetching X type of data and Y type of data, and then returning them both and merged and with some calculations, or if I want to manage Users as in the example above.. Where Can I find well written and structured code to get inspiration of? \$\endgroup\$ – Kar19 Oct 24 '18 at 12:09
  • 1
    \$\begingroup\$ Personally I can't recommend anything to specific but just hanging around on reddit.com/r/php (not a help platform) I picked up quite a bit, I don't use frameworks (you should it was a mistake not to) but if I had to symfony looks good to me! \$\endgroup\$ – Dan Oct 25 '18 at 7:56
  • \$\begingroup\$ A user login to his account, my method login($use, $pas) has to check the database if use exist and if the credentials are correct, set login sessions to true and security tokens - instead of handling all in one method I created a separate class UserAuthentication & UserSessions. When calling login(...) inside that method I authenticate the user with the UserAuthentication class. \$\endgroup\$ – Kar19 Mar 20 at 9:47
  • \$\begingroup\$ If returns true, I set sessions with UserSessions is that considered good practice? Please review softwareengineering.stackexchange.com/questions/388870/… \$\endgroup\$ – Kar19 Mar 20 at 9:47

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.