0
\$\begingroup\$

I need some constructive feedback for the working OOPs project which I created to introduce to Object Oriented Programming in a class. Hence is for beginners, I did not include interfaces or abstract functions nor singleton or factories. Can you give me some feedback on the code style, some possible anti patterns? Thanks

<?php

declare(strict_types=1);

namespace MidoriKocak;

use function array_key_exists;

/**
 *
 */
final class User
{

    /**
     * @var Database
     */
    private Database $db;
    /**
     * @var string
     */
    private ?string $id = null;

    /**
     * @var string
     */
    private string $email;
    /**
     * @var string
     */
    private string $username;
    /**
     * @var string
     */
    private string $password;

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

    /**
     * @param  string  $password
     *
     * @return void
     */
    public function setPassword(string $password): void
    {
        $this->password = password_hash($password, PASSWORD_DEFAULT);
    }

    public function getPassword(): string
    {
        return $this->password;
    }

    /**
     * @param  string  $email
     *
     * @return void
     */
    public function setEmail(string $email): void
    {
        $this->email = $email;
    }

    /**
     * @param  string  $username
     *
     * @return void
     */
    public function setUsername(string $username): void
    {
        $this->username = $username;
    }

    /**
     * @return void
     */
    public function save()
    {
        if ($this->id) {
            $this->db->update($this->id, $this->toArray(), 'users');
        } else {
            $this->db->insert($this->toArray(), 'users');
            $this->id = $this->db->lastInsertId();
        }
    }

    /**
     * @return array
     */
    public function toArray(): array
    {
        return [
            'username' => $this->username,
            'email' => $this->email,
            'password' => $this->password
        ];
    }

    /**
     * @param  array  $data
     *
     * @return void
     */
    public function fromArray(array $data): void
    {
        if (array_key_exists('id', $data)) {
            $this->id = (string)$data['id'];
        }

        $this->setEmail($data['email']);
        $this->setUsername($data['username']);
        $this->setPassword($data['password']);
    }
}

Thank you for your feedback.

(P.S. Please be harsh as possible.)

\$\endgroup\$
8
  • 1
    \$\begingroup\$ Please be advised that your question may be closed under the formal pretext that it is not an excerpt from a working code but an educational example. I would remove that beginners stuff from the question, make sure you have this very code up and running as a part of some application and then just ask to review it. this is a very good code for the review and don't want to lose the opportunity to post it \$\endgroup\$ Commented Jan 13, 2020 at 9:30
  • \$\begingroup\$ thank you, where should I post it then? There is a working example project but I did not want to overwhelm the question with details. \$\endgroup\$ Commented Jan 13, 2020 at 9:34
  • \$\begingroup\$ Keep it here, just change the description. It should say this is just a working code from your project. Make sure it is indeed so. For example, make sure there is no copy-paste error in the login() method. \$\endgroup\$ Commented Jan 13, 2020 at 9:37
  • \$\begingroup\$ Thank you, let me update it. \$\endgroup\$ Commented Jan 13, 2020 at 9:59
  • 1
    \$\begingroup\$ Thank you. What is the relation between User and Users classes? \$\endgroup\$ Commented Jan 13, 2020 at 10:07

1 Answer 1

Reset to default
2
\$\begingroup\$

Inconsistent Function Imports

Import all used functions or none. If I see this:

use function array_key_exists;

I assume that it is the only global function being used in that file. To my surprise, it is not true as password_hash is used inside the same file too.

Either import them all, or none. I myself prefer no imports and instead prefix global function calls with the backslash to denote global scope (ie return \array_key_exists($k, $a);).

Useless Docblocks

In PHP 7.4 you get the benefit of typehinting properties. I dont see a reason to repeat that type in a docblock anymore in that case. Furthermore it may cause confusion of what the original intent was, if docblock and typehint mismatch. Like here:

/**
 * @var string
 */
private ?string $id = null;

So can it be null or not? I suppose it can, but you never know if you made it this confusing...

Active Record Anti-pattern

You asked to identify possible anti patterns. Active record is one of them. It combines two responsibilities. The entity should not know anything about where it is going to be stored and how. It may eventualy get stored on multiple places or be stored in various ways. The entity should only know that it has some structured data. Then another class(es) should know how to store it and reconstruct it from its permanent representation (ie a db row).

Also notice that save() is either going to have to be repeated in every entity. Or all entities must inherit the same parent or use the same trait (inheritance is oftne not a good idea, and multiple inheritance (traits) even worse).

Also notice that only one of User class's methods (namely the save) uses the database object. That only confirms that it should not be there. You should pass the user entity/structure to another object asking him to save the user into its persistent storage for later retrieval.

Check Your Arrays' Keys

In the fromArray() you are copying the values from array data without making sure that those data is there.

$this->setEmail($data['email']);
$this->setUsername($data['username']);
$this->setPassword($data['password']);

if (isset($data['email']) && \is_string($data['email'])) $this->setEmail($data['email']);

or set it to some default

$this->setEmail($data['email'] ?? '');

Handling Login

Although you have removed that part from your post, I will address it nevertheless.

isLoggedIn should not be persistent property of user entity (table). It should be stored (or inferred) from session. Otherwise how you make sure that it is turend to false after some time of inactivity?

Anyway User::login(string $email, string $pass): bool again breaks SRP. You should have it more like this: Authenticator::login(string $email, string $pass): ?User. You may be tempted to make it UsersRepository::login() but repository usualy does not need access to session while the login method does, therefore it should be in its own class, not in the repository.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.