Насколько у меня правильный код ООП php?

Question

[Александр]

Осваиваю ООП и перехожу с процедурного стиля, хочу понять правильно ли я усвоил материал, чтобы сразу писать "без костылей"... Вот мой первый класс:

class connectDB {
    protected $dbh;
    protected $error;
    
    protected function connect() {
        try { 
            $this->dbh = new PDO("mysql:host=localhost;dbname=jetjapan_new","jetjapan_jet","GWJfasfw234");
            $this->dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            $this->dbh->exec("set names utf8");
        } catch(PDOException $e) { 
            echo "Произошла системная ошибка. Код ошибки <b>{$e->getCode()}</b>. Уведомление было отправлено администратору!";
        }
    }
}

class SearchOrder extends connectDB {
    public $orderID;
    public $create;
    public $price;
    public $status;
    protected $params;

    public function getOrderInfo($orderID, $params = "*") {
        $this->orderID = (int) strip_tags($orderID);
        $this->params = (string) strip_tags($params);
        parent::connect();
        try { 
            $this->resultDB = $this->dbh->prepare("SELECT {$this->params} FROM `order` WHERE `id` = :orderID");
            $this->resultDB->bindParam(':orderID', $this->orderID);
            $this->resultDB->execute();
            $this->resultDB = $this->resultDB->fetch(PDO::FETCH_OBJ);
            $this->create = $this->resultDB->addtime;
            $this->price = $this->resultDB->price;
            $this->status = $this->resultDB->status;
        } catch(PDOException $e) { 
            echo "Произошла системная ошибка. Код ошибки <b>{$e->getCode()}</b>. Уведомление было отправлено администратору!";
        }
    }
    public function changeOrder($orderID, $column, $value) {
        $this->orderID = (int) strip_tags($orderID);
        $this->column = (string) strip_tags($column);
        $this->value = (string) strip_tags($value);
        parent::connect();
        try { 
            $this->resultDB = $this->dbh->prepare("UPDATE `order` SET {$this->column} = :value WHERE `id` = :orderID LIMIT 1");
            $this->resultDB->bindParam(':value', $this->value);
            $this->resultDB->bindParam(':orderID', $this->orderID);
            $this->resultDB->execute();
            $this->resultDB = $this->resultDB->rowCount();
            return $this->resultDB;
        } catch(PDOException $e) { 
            echo "Произошла системная ошибка. Код ошибки <b>{$e->getCode()}</b>. Уведомление было отправлено администратору!";
        }
    }
}

Насколько правильный, удобный код?! Просьба без шуточек, только конструктив, спасибо :)

Comments

[Максим]

Надо учить шаблоны проектирования. В данном случае подойдет Singleton.

[Frel]

как учишь php?

[Игорь Воротнёв]

Максим, синглтон = антишаблон.

Answer 1

[D3lphi]

Здесь плохо всё, к сожалению.

Начнем с того, что вы неверно наследуете классы. Почему у вас класс, отвечающий за подключение к базе данных является родителем класса, работающим с заказами? Наследование применяется, если можно сказать, что что-то является чем-то. Например, разработчик является работником; компьютер является устройством и тд. Здесь же у вас вообще близко такой логике не получится следовать. Вы должны передавать хотя бы объект для работы с бд через инъекцию, например, в конструктор. В идеале, нужно использовать паттерн репозиторий для работы с базой данных.

Класс SearchOrder у вас не только выполняет запросы, но еще и работает с данными, хранит состояние этих самых данных, фильтрует данные (strip_tags()). Непорядок. Это все нужно разделять. У вас вообще получаются какие-то богообъекты, которые умеют во все.

Вы каждый раз повторяете строки с подготовкой запроса, биндингом параметров, отправкой запроса и тд. Не думали, что неплохо бы было написать какую-нибудь обертку и выполнять запросы как-нибудь так:

$result = $wrapper->select("SELECT * FROM `tablename` WHERE `id` = :id", ['id' => 5]);

?

Вы вызываете connect() в методах. То есть, каждый вызов этого метода будет приводить к установке нового соединения с базой данных, даже если оно уже было установлено. Соединение с базой данных это достаточно дорогостоящая операция.

Зачем вы используете свойства, если можно обойтись обычными локальными переменными:

$this->orderID = (int) strip_tags($orderID);
$this->column = (string) strip_tags($column);
$this->value = (string) strip_tags($value);

?

Почему вы стриппите тэги у идентификатора? вы настолько не уверены в том, что влетает в функцию:
strip_tags($orderID);
?

Если вы не используете php 7 и, как следствие, скалярный тайпхинтинг, то должны делать проверки на тип входящего аргумента. Если что-то не так с типом, бросаем исключение (А не приводим его к нужному)! Например:

if (!is_string($arg)) {
    throw new InvalidArgumentTypeException('string', $arg);
}

Это в идеале. Вы не обязаны это делать, конечно же. Но вот такие проверки делают приложение безопаснее. Хотя, опять же, повторюсь, в 2017 нужно начинать новые проекты на php 7.1+.

Ошибки не нужно выводить в этом классе. Вы должны поймать исключение базы данных, преобразовать ее в исключение предметной области и пробросить его дальше и где-то там, на уровне выше вывести информацию пользователю об ошибке. В mvc системе, например, это делается в контроллере.

Кроме всего прочего, почитайте про стандарты оформления кода. Вы им не следуете.

Вам пока рано писать такие велосипеды. Судя по всему, у вас нет опыта вообще. Посмотрите готовые решения: фреймворки, ORM, изучите их, хотя бы поверхностно разберитесь, как оно работает и уже потом пробуйте что-то сделать, исходя из полученных знаний.

Желаю успехов!

Comments

[Александр]

Мощно! Спасибо )

Answer 2

[Артем Спиридонов]

$props = array(
     'orderID' => 'int',
     'orderID' => 'string',
     'value' => 'string',
);

foreach ($props as $value => $type)
{
     $this->$value = ($type) strip_tags($value);
}

;)

А если серьезно - просто подсмотрите, как у других сделано.

Answer 3

[Вячеслав Успенский]

Основные ошибки уже указали выше. Кроме того пишите классы с большой буквы. И класс лучше называть Connector а не connect. Вы работаете с объектами.

Вам важно понять что ООП вырос не просто так. Одни из основных целей это ускорение разработки и простота поддержки. Ваш код должен быть написано таким образом, чтобы легко манипулировать объектами. Упрощенно $lion->eat($meat); Просто, понятно, коротко 8)

Кроме того по ошибкам выше. Почитайте про DataMapper/ActiveRecord

Answer 4

[MaximMRX]

Почитайте документацию про PSR-4, станет потом более понятно про все именования и тд

Comments

[Кирилл Несмеянов]

С PSR-1, PSR-2 и PSR-12 не попутали часом? =)

Answer 5

[dmitriy]

вообще не правильный, не должно быть наследования, инъекция зависимости должна быть

Answer 6

[Дон Ган]

$this->resultDB = $this->dbh->prepare("SELECT {$this->params} FROM `order` WHERE `id` = :orderID");
$this->resultDB->bindParam(':orderID', $this->orderID);
$this->resultDB->execute();
$this->resultDB = $this->resultDB->fetch(PDO::FETCH_OBJ);

вместо этого треша надо использовать обертки.
есть 2 в сети:
https://github.com/Vasiliy-Makogon/Database - моя
phpfaq.ru/SafeMysql - чужая

Comments

[BoShurik]

У вас отключены issues, поэтому задам тут:
- почему не следуете PSR. Для библиотеки написанной в 2017 это как минимум странно
- зачем приватный конструктор и ссылающийся на него фабричный метод с точно такой же сигнатурой?

и есть еще одна (самая популярная, как мне кажется): https://github.com/doctrine/dbal

[Александр]

Спасибо, взял на вооружение. Отдельное спасибо за документацию на Русском - все понятно :)