Правильно ли реализованы классы?

Question

[4sadly]

Класс ActiveRecord

abstract class ActiveRecord
{
    protected int $id;
    protected static $db;

    public static function setup($type, $user, $pass, $opt)
    {
        self::$db = new \PDO($type, $user, $pass, $opt);
    }

    abstract protected static function getTableName(): string;

    public static function findAll(): ?array
    {
        $stmt = self::$db->prepare('SELECT * FROM ' . static::getTableName());
        $stmt->execute();
        $r = $stmt->fetchAll();
        $c = sizeof($r);
        for ($i = 0; $i < $c; $i++) {
            $r[$i] = new static($r[$i]);
        }
        return $r ? $r : null;
    }

    public static function findById(int $id): ?object
    {
        $stmt = self::$db->prepare('SELECT * FROM `' . static::getTableName() . '` WHERE id = ?');
        $stmt->execute([$id]);
        $r = $stmt->fetch();

        return $r ? new static($r) : null;
    }

    public static function findByAttribute(array $attributes): ?object
    {
        $keys = array_keys($attributes);
        foreach ($keys as $key => $value) {
            $keys[$key] = '`' . $value . '` = ?';
        }
        $stmt = self::$db->prepare('SELECT * FROM `' . static::getTableName() . '` WHERE ' . implode(' AND ', $keys));
        $stmt->execute(array_values($attributes));
        $r = $stmt->fetch();
        return $r ? new static($r) : null;
    }

    public static function findCollectionByAttribute(array $attributes): ?array
    {
        $keys = array_keys($attributes);
        foreach ($keys as $key => $value) {
            $keys[$key] = '`' . $value . '` = ?';
        }
        $stmt = self::$db->prepare('SELECT * FROM `' . static::getTableName() . '` WHERE ' . implode(' AND ', $keys));
        $stmt->execute(array_values($attributes));
        $r = $stmt->fetchAll();
        $c = sizeof($r);
        if ($c > 0) {
            for ($i = 0; $i < $c; $i++) {
                $r[$i] = new static($r[$i]);
            }
            return $r;
        }
        return null;
    }

    public static function whereOne($query, array $attributes): ?object
    {
        $stmt = self::$db->prepare('SELECT * FROM `' . static::getTableName() . '` WHERE ' . $query);
        $stmt->execute([$attributes]);
        $r = $stmt->fetch();
        return $r ? new static($r) : null;
    }

    public static function whereCollection($query, array $attributes): ?array
    {
        $stmt = self::$db->prepare('SELECT * FROM `' . static::getTableName() . '` WHERE ' . $query);
        $stmt->execute([$attributes]);
        $r = $stmt->fetchAll();
        $c = sizeof($r);
        if ($c > 0) {
            for ($i = 0; $i < $c; $i++) {
                $r[$i] = new static($r[$i]);
            }
            return $r;
        }
        return null;
    }

    public static function deleteById(int $id): void
    {
        self::$db->prepare('DELETE FROM `' . static::getTableName() . '` WHERE id = ?')->execute([$id]);
    }

    public function save(): void
    {
        $properties = get_object_vars($this);
        if (empty($properties['id'])) {
            $questionmarks = array_fill(0, sizeof($properties), '?');
            $stmt = self::$db->prepare('INSERT INTO `' . static::getTableName() . '` (`' . implode('`,`', array_keys($properties)) . '`) VALUES (' . implode(',', $questionmarks) . ')');
            $stmt->execute(array_values($properties));
            $this->id = self::$db->lastInsertId();
        } else {
            $keys = array_keys($properties);
            foreach ($keys as $key => $value) {
                $keys[$key] = $value . ' = ?';
            }
            $stmt = self::$db->prepare('UPDATE `' . static::getTableName() . '` SET ' . implode(',', $keys) . ' WHERE id = ?');
            $stmt->execute(array_merge(array_values($properties), [$this->id]));
        }
    }

    public function delete(): void
    {
        self::$db->prepare('DELETE FROM `' . static::getTableName() . '` WHERE id = ?')->execute([$this->id]);
    }

    public function getId(): int
    {
        return $this->id;
    }

}

Класс пользователя:

class User extends ActiveRecord
{
    protected string $login;
    protected string $password;
    protected string $fullname;
    protected string $type;
    protected int $status;

    protected static function getTableName(): string
    {
        return 'users';
    }

    /**
     * @return string
     */
    public function getLogin(): string
    {
        return $this->login;
    }

    /**
     * @param string $login
     */
    public function setLogin(string $login): void
    {
        $this->login = $login;
    }

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

    /**
     * @return string
     */
    public function getFullname(): string
    {
        return $this->fullname;
    }

    /**
     * @param string $fullname
     */
    public function setFullname(string $fullname): void
    {
        $this->fullname = $fullname;
    }

    /**
     * @return string
     */
    public function getType(): string
    {
        return $this->type;
    }

    /**
     * @param string
     */
    public function setType(string $type): void
    {
        $this->type = $type;
    }

    /**
     * @return int $status
     */
    public function getStatus()
    {
        return $this->status;
    }

    /**
     * @param int $status
     */
    public function setStatus($status): void
    {
        $this->status = $status;
    }

    public function isFree(): bool
    {
        if ($this->status < 2) return true;
        return false;
    }

    public function validatePassword($password): bool
    {
        return password_verify($password, $this->password);
    }

Что-то мне подсказывает, что можно сделать массив data, ActiveRecord создать __construct и использовать его в дочерних классах, или стоит сделать как-нибудь по другому? если можете - дайте совет, буду благодарен
Писать о том, чтобы я не создавал велосипеды - бесполезно

Comments

[Дмитрий]

много дублирующего кода

[4sadly]

Дмитрий, я понимаю, как это можно исправить?

[Максим Федоров]

сеттеры и геттеры в AR модели, скрестили все плохое :)

[Дмитрий]

4sadly, ну вот пример

public static function whereOne($query, array $attributes): ?object
    {
        $stmt = self::$db->prepare('SELECT * FROM `' . static::getTableName() . '` WHERE ' . $query);
        $stmt->execute([$attributes]);
        $r = $stmt->fetch();
        return $r ? new static($r) : null;
    }

    public static function whereCollection($query, array $attributes): ?array
    {
        $stmt = self::$db->prepare('SELECT * FROM `' . static::getTableName() . '` WHERE ' . $query);
        $stmt->execute([$attributes]);
        $r = $stmt->fetchAll();
        $c = sizeof($r);
        if ($c > 0) {
            for ($i = 0; $i < $c; $i++) {
                $r[$i] = new static($r[$i]);
            }
            return $r;
        }
        return null;
    }

Все что одинаковое выносим в метод приватный where, в каждом вызываем этот метод

[di]

public static function findAll(): ?array
    {
        $stmt = self::$db->prepare('SELECT * FROM ' . static::getTableName());
        $stmt->execute();
        $r = $stmt->fetchAll();
        $c = sizeof($r);
        for ($i = 0; $i < $c; $i++) {
            $r[$i] = new static($r[$i]);
        }
        return $r ? $r : null;
    }

Почему не использовать fetchAll(PDO::FETCH_CLASS, self::classname) Зачем какие то циклы?

[4sadly]

Максим Федоров, то есть сеттеры и геттеры в ar не нужны?

[Максим Федоров]

4sadly, они и в нормальных сущностях не нужны, но в AR -- это вообще бред

[4sadly]

Максим Федоров, нормальным ли решением будет сделать массив data и использовать магические методы __set и __get?

[Максим Федоров]

4sadly, плохим

[4sadly]

Максим Федоров, а какое будет нормальным?

Answer 1

[Максим]

Если не хотите использовать готовое - можете создать своё. Ничего плохого в этом нет. Чаще всего готовое имеет зависимости и не всегда это подходит. Ваша модель напоминает модель yii2. Я думаю, вам будет полезно посмотреть на базовый и основной класс Active Record от Yii2. Надеюсь не видели.

По текущем замечаниям у AR для простоты и быстрой разработки свойства являются публичными и они равны прям в базе данных, что позволяет избавиться от геттеров и сеттеров их выполняют роль публичные свойства, как некое DTO. Так что сами свойства у вас не логично что имеют не публичный доступ.

Comments

[4sadly]

А как реализовать метод find, который реализует возврат экземпляра класса и метод where, который работает
$user = User::find()->where();
Каким образом он ставит атрибуты классу, возвращаемому с метода find?

[Максим]

4sadly, Дак вы перейдите и посмотрите. Я же вам выслал код исходник.

Answer 2

[Северное Сияние]

Высокоуровневые методы (например, find****) должны быть обёрткой над кодом, который использует билдер запросов. У тебя дубляж низкоуровневого кода работы с PDO повсеместно.

Когда я свою ORM писал, у меня это выглядело так:

// высокоуровневый метод делающий ТОЛЬКО выборку по id
    public function findModelById($id): Model
    {
            return $this->findModelByParams(
                array('where' => array('`id` = ?i' => array($id)))
            );
    }

    // УНИВЕРСАЛЬНЫЙ низкоуровневый метод, находящий объект модели по любым параметрам
    public final function findModelByParams(array $params=array()): Model
    {
        $res = parent::createQuerySelect($params);

        $object = $this->createModelFromDatabaseResult( is_object($res) && $res->getNumRows() ? $res->fetch_assoc() : array() );

        return $object;
    }

    // построитель любого sql-запроса на SELECT, его исполнение и возвращение объекта типа РЕЗУЛЬТАТ
    protected final function createQuerySelect(array $params)
    {
        $params = self::makeSqlFromParams($params); // это сам билдер!!!!!!!!!!!!!!

        $sql = 'SELECT' . $params['what'] . 'FROM ?f' . $params['join'] . $params['where'] . $params['order'] . $params['limit'];

        array_unshift($params['args'], $this->getTableName());
        array_unshift($params['args'], $sql);

        $result = call_user_func_array(array($this->getDb(), 'query'), $params['args']);

        return $result;
    }

Аналогично был метод на удаление:

protected final function createQueryDelete(array $params)
    {
        $params = self::makeSqlFromParams($params);

        $sql = 'DELETE FROM ?f ' . $params['where'] . $params['limit'];

        array_unshift($params['args'], $this->getTableName());
        array_unshift($params['args'], $sql);

        return call_user_func_array(array($this->getDb(), 'query'), $params['args']);
    }

insert/update в подобном билдере не нуждались, они были частью метода save, в точности как у тебя.

Твоя обертка будет работать, но дубляжа кода много, это прям бросается в глаза. Ты можешь не писать билдер запросов, но тогда ты будешь описывать каждый новый метод и в каждом методе у тебя везде будет написано одно и тоже - prepare/execute/fetch

Это всё будет давить, особенно когда тебе понадобится сделать методы типа findUserByIdOrNameAndSurname($id, $name, $surname)
где нужно будет исполнить запрос вида select * from users where id = ? OR name = ? AND surname = ?

Тут уже не спасет твой метод findByAttribute.

Тебе захочется иметь "красивый" метод высокого уровня, в итоге ты либо напишешь его в привычной стилистике

public static function findUserByIdOrNameAndSurname($id, $name, $surname)
    {
        $stmt = self::$db->prepare('SELECT * FROM `' . static::getTableName() . '` WHERE id = ? or name = ? and surname = ?');
        $stmt->execute([$id, $name, $surname]); // или как тут правильно, не знаю..
        $r = $stmt->fetch();

        return $r ? new static($r) : null;
    }

либо сам придешь к необходимости создать метод findModelBySql($sql).

Но и то и то будет нагромождением повторяющихся кусков кода.

spoiler

PS дзен AR ты познал, крайне советую остановиться и не повторять моих ошибок. Я тоже писал свою ОРМ, только это никому нахер не нужно. И твоя ОРМ тоже не нужна, даже тебе. Есть проверенные решения, тот же eloquent. Не занимайся ерундой.

Answer 3

[Pavel Omelchenko]

public function isFree(): bool
    {
        if ($this->status < 2) return true;
        return false;
    }

Почему нельзя было просто сделать return $this->status < 2 ?