HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

A simple User class

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
usersimpleclass

Problem

// The DataLayer to MySQL
require_once $_SERVER["DOCUMENT_ROOT"] . "/api/DAL/classes/User.dal.php";

class User
{
    public $id;
    public $name;
    public $age;

    public function __construct($id = null)
    {
        $this->dl = new UserDAL();

        // Get an existing user object
        if (!empty($id)) {
            $user       = $this->getUsers($id);
            $this->id   = $user->id;
            $this->name = $user->name;
            $this->age  = $user->age;
        }
    }

    public function create($user)
    {
        // Create new user
        $this->name = $user->name;
        $this->age  = $user->age;
        return $this->dl->create($user);
    }

    public function update($user)
    {
        // Update existing user
        $this->id   = $user->id;
        $this->name = $user->name;
        $this->age  = $user->age;
        return $this->dl->update($user);
    }

    public function delete($user)
    {
        // Create new user
        $this->id = $user->id;
        return $this->dl->delete($user);
    }

    public function getUsers($id)
    {
        return $this->dl->getUsers($id);
    }

}

// Some tests...

// Get existing user
$existing_user = new User("1");
var_dump($existing_user);

// Create a new user
$new_user = new User();
$new_user->age = 23;
$new_user->name = "Bob";
$new_user->create($new_user);
var_dump($new_user);

// Update existing user
$update_user = new User();
$update_user->id = "1";
$update_user->name = "Henry";
$update_user->age = 100;
$update_user->update($update_user);
var_dump($update_user);

// Delete a user
$delete_user = new User();
$delete_user->delete("1");

Solution

The comment by mferly is very constructive:

Comment your code/methods/properties/everything. Seriously. In your getUsers() method, is the $id argument/value an array or integer I know it's not an array)? I ask because getUsers() implies that you're returning multiple users, not just one. Otherwise it'd be named getUser() See the discrepancy and potential confusion?

The source of the getUsers() method from the class stored at $_SERVER["DOCUMENT_ROOT"] . "/api/DAL/classes/User.dal.php" was not included so we can only guess as to how it functions.

I'd also suggest considering following the singleton pattern with respect to how the dl property is created in the constructor for each instance:

$this->dl = new UserDAL();


Even if instances of that class are not very demanding of resources it may still be beneficial to reduce the number of instances as much as possible for other reasons. Does each instance of the DAL create a connection to the database? If so, that may be an important reason to minimize the number of instances.

Code Snippets

$this->dl = new UserDAL();

Context

StackExchange Code Review Q#130061, answer score: 3

Revisions (0)

No revisions yet.