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!

• Kar19: 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?
• Dan: 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!
• Kar19: 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.
• Kar19: If returns true, I set sessions with UserSessions is that considered good practice? Please review softwareengineering.stackexchange.com/questions/388870/…