Should I add the "fetch_group" method to my user class or should I create a separate class for group

Question

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.

Answer 1

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!