Хорошо ли написан класс БД?

Question

[Max Ba]

Укажите пожалуйста на грубые ошибки в коде. Планирую использовать этот класс. Не хотелось бы потом внезапно наткнуться на проблемы. Ухожу с синглтона.
PS. всё началось с фразы "никакой работы с БД не должно быть в модели". Также начитался про SOLID)))
В общем верно ли написал код?

class DB
{
  protected $db;

  public function __construct(){
    //require_once 'config_db.php';
    try{
      $this->db = new PDO('mysql:host=localhost;dbname=test','root','');  
    }catch(PDOException $e){
      die('DB ERROR');
    }
  }

  private function query($query, array $params = array()){
    $stmt = $this->db->prepare($query);
    if(!empty($params)){
      foreach($params as $k => $v){
        if(is_int($v)){
          $stmt->bindValue(':'.$k, $v, PDO::PARAM_INT);
        }else{
          $stmt->bindValue(':'.$k, $v, PDO::PARAM_STR);
        }
      }
    }
    $stmt->execute();
    return $stmt;
  }

  public function getRows($query, array $params = array()){
    return $this->query($query, $params)->fetchAll(PDO::FETCH_ASSOC);
  }

  public function getColumn($query, array $params = array()){
    return $this->query($query, $params)->fetchColumn();
  }

  public function lastInsertId(){
    return $this->db->lastInsertId();
  }
}


class Car
{
  protected $db;

  public function __construct(DB $db){
    $this->db = $db;
  }

  public function getAll(){
    $query = "SELECT `id`, `name` FROM `cars`";
    return $this->db->getRows($query);
  }

  public function getById(int $id){
    $query = "SELECT `id`, `name` FROM `cars` WHERE `id` = :id";
    return $this->db->getRows($query, array('id' => $id));
  }
}



$db = new DB();

$car = new Car($db);

var_dump($car->getAll());

var_dump($car->getById(1));

Comments

[Иван Шумов]

У меня есть только один вообще вопрос - зачем вы сами пишете подобный функционал?

[FanatPHP]

Иван Шумов, это очень полезно для саморазвития.

[Иван Шумов]

FanatPHP, я тоже так делал, достаточно давно и когда не были Doctrine, AR и иже с ними. Сейчас я хочу признать что это бесполезная трата времени. Решать стоит практические задачи, а не изобретать велосипеды

[FanatPHP]

Иван Шумов, На PHPRussia было как минимум два доклада, где авторы практически писали фреймворк с нуля. И оба раза залы были полные.
возможно, не все разделяют ваш "практический" подход. Некоторые люди хотят не только пользоваться инструментом, но и понимать, как он работает.

[Иван Шумов]

FanatPHP, для этого достаточно открыть инструмент и почитать - это же опенсорс, черт побери)

[Max Ba]

Иван Шумов, если я не могу сделать быстро хоть какой-либо велик (а я могу), то я умею кодить?)

Я часто замечал, что паттерны, методики, схемы проще всего изучать не на абстрактном варианте, а конкретно на своём сайте. Я долго вникал в ооп посредством "машин", "животных" с их наследованиями. Въехал только на конкретном сайте. Своем. Ибо ты понимаешь ЗАЧЕМ тебе то или иное.
А так да, согласен. Лучше конечно готовое решение) Особенно в коммерческих целях.

[FanatPHP]

Иван Шумов, этот комментарий показывает, что сам-то ты как раз не открывал ни разу. Доктрина - это десятки мегабайт кода. просто понять хотя бы приблизительно ее структуру и понять куда смотреть - уйдет несколько дней. А, главное - без подготовки, без понимания, что там ожидать в принципе - то вообще не разберешься. Именно поэтому и надо сначала наляпать что-то свое.

[Иван Шумов]

Max Ba, готовое решение экономит время, силы, деньги, а так же позволяет делать решение поддерживаемым. Написание велосипедов давно кануло в лету.

Кодить ты умеешь когда ты можешь написать код, соответствующий тз с интерфейсом входа и выхода. А это уже инжиниринг)

[Иван Шумов]

FanatPHP,

Доктрина - это десятки мегабайт кода. просто понять хотя бы приблизительно ее структуру и понять куда смотреть - уйдет несколько дней. А, главное - без подготовки, без понимания, что там ожидать в принципе - то вообще не разберешься.

У меня было это время и более того - я там был, разобрался и успокоился. Так же сделал с AR и еще несколькими инструментами. Более того - когда тебе необходимо использовать опенсорс в коммерческой разработке ты обязан залезть в потроха и проверить несколько фреймворк соответствует требованиям организации

Answer 1

[Lander]

die('DB ERROR');
Сразу нет! Класс не должен останавливать приложение, если произошла ошибка. Он должен бросать исключение, а что делать дальше должен решать клиентский код.

никакой работы с БД не должно быть в модели

В каком смысле это написано? Где это написано?

Ухожу с синглтона

На каждый запрос будете создавать отдельное подключение к БД? Там вообще то есть ограничение на количество открытых.

Я не заметил особой разницы между работой с mysqli и с вашим классом. Инкапсуляция бинда параметров метод query только? Так унаследуйтесь от класса mysqli и напишите удобный метод для этого.

И почему вы не берёте какую то готовую обёртку? Благо их написано тысячи!

Comments

[Lander]

Прошу прощения не mysqli, а PDO. Но сути это не меняет.

[Max Ba]

Lander,

В каком смысле это написано? Где это написано?

Если я решу заменить PDO на другую БД, то конструкция типа

public function getById(int $id){
  $query = "SELECT `id`, `name` FROM `cars` WHERE `id` = :id";
  return $this->db->getRows($query, array('id' => $id));
}

лучше чем

public function getById(int $id){
  $query = "SELECT `id`, `name` FROM `cars` WHERE `id` = ?";
  $stmt = $this->db->prepare($query);
  $stmt->execute(array($id));
  return $stmt->fetchAll();
}

Разве нет?

На каждый запрос будете создавать отдельное подключение к БД? Там вообще то есть ограничение на количество открытых.

А зачем? Раз передал в объект Car и дальше делай что и сколько хочешь. Мне нужны массивы. А не объекты. В данном случае)

[Max Ba]

Lander, ну да, на счет соединений не совсем понял...
Один раз инициализировал бд и начал передавать во все все модели.

$db = new DB();

$car = new Car($db);
$obj2 = new Obj2($db);
$obj3 = new Obj3($db);

Одно ведь соединение

[Lander]

Раз передал в объект Car

Пока вы не сделали объект класса Car где вы будете хранить экземпляр соединения?

Если я решу заменить PDO на другую БД

Тогда ваш класс будет бесполезен. Придётся писать новый с таким же интерфейсом (который, кстати не объявлен).

[Max Ba]

Lander, то есть мне лучше не заморачиваться и не писать класс обертку для БД? А само логику модели вернуть как было?

public function getById(int $id){
  $query = "SELECT `id`, `name` FROM `cars` WHERE `id` = ?";
  $stmt = $this->db->prepare($query);
  $stmt->execute(array($id));
  return $stmt->fetchAll();
}

[Lander]

Max Ba, А если понадобиться использовать объект $db внутри функции, тогда вы должны будете его объявить внутри функции через global. Итак, получается, что соединение с БД у вас глобальное. То есть уходя от синглтона вы приходите к глобальным переменным.

[Lander]

Max Ba,

то есть мне лучше не заморачиваться и не писать класс обертку для БД?

А я не знаю. Дело в том, что мы даже не знаем какую задачу вы решаете! :) Как мы можем понять писать вам обёртку или нет? Но то, что написано сейчас полноценной обёрткой называться не может. Думаю вам лучше изучить пакет, написанный php666. Там всё ОЧЕНЬ подробно с комментариями. Возможно даже начать использовать его в работе.

И в соседнем ответе я вам комментарий написал что иногда всё таки имеет смысл писать обёртку. :)

[FanatPHP]

Lander, вообще-то global - не единственный способ передачи переменной в функцию.

[Lander]

FanatPHP, Ну чего началось? Вы же поняли о чём я! :)))

[FanatPHP]

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

[Max Ba]

Парни большое спасибо. Если будет что неясно, напишу)))
Но думаю не будет.

[Михаил Братеньков]

Тогда ваш класс будет бесполезен. Придётся писать новый с таким же интерфейсом (который, кстати не объявлен).

Ну так в этом и суть ORM. Легче написать один класс с тем-же интерфейсом чем бегать по всей программе и заменять обращения к БД на новые

[Lander]

Михаил Братеньков, Не совсем. Обычно между БД и моделью стоит ещё одни слой абстракции, который фактически реализует обёртку для получения/записи данных. А эта сама обёртка уже использует драйвер конкретной БД. Вот этот драйвер и пишут под каждую СУБД.

[FanatPHP]

Lander, ну так пдо и есть такой драйвер, так что спор в данном случае довольно абстрактный :)

По сути же я согласен c Михаил Братеньков . Даже если бы это был mysqli и пришлось переписывать на pg, то при наличии не имеющей протечек абстракции там дел на 1 день. То есть польза от класса-обертки очевидная. А то что написать грамотную обертку без протечек не так просто - это уже другой вопрос ;)

Answer 2

[Александр]

Параметры бд обычно выносятся в отдельный файл.
Совсем забыл, die, exit внутри класса использовать нельзя.

Comments

[Max Ba]

А про "никакой работы с БД не должно быть в модели" и вообще в SOLID я разобрался?) Или не относится к конкретному примеру?

[Александр]

Max Ba, в модели и должна быть работа с бд. Это в контроллере бд использовать неправильно.

[Max Ba]

Александр, facepalm. Запутался... Я в комменте Lander-у написал метод getByID. Есть смысл было вобще делать класс обертку для PDO?

[Lander]

Max Ba,

Есть смысл было вобще делать класс обертку для PDO?

Возможно и есть. Например, если вы хотите строить запросы с помощью какого то декларативного языка. Что то вроде:

$db->select(
  'from' => 'car',
  'where' => [
    ['wheel_count', '>', 4],
    ['cost', '<', '500000']
  ],
  'sort' => [
    'weight' => 'ASC'
  ]
);

[Max Ba]

Lander, не, это чересчур. Я видел подобное. Нужно иметь гибкую возможность делать запросы. Проанализировав скрипты, я пришел к тому, что все запросы к базе состоят из ЗАПРОСА и ПЕРЕМЕННЫХ.

По сути это prepare и execute.

Сам запрос query может быть сформирован как угодно, руками, конкатенацией, и иметь какой угодно размер. Было и 6-7 JOIN-ов в связке с WHERE `col` NOT IN (SELECT `id`...). UNIONы и тд.
Поэтому ТАКИЕ обертки по моему усложняют работу с БД))

Answer 3

[FanatPHP]

В целом подход правильный.
Именно с точки зрения SOLID и сам класс, и его использование спроектированы верно.

Можно поправить только по мелочам.

- код соединения никуда не годится. Брать отсюда: Как правильно соединяться с mysql в PDO
- при этом конфиграцию (хост, пароль, чарсет и прочее) вынести отдельно и передавать в конструктор в виде массива
- query() переписать так:

private function query($query, array $params = array()){
    $stmt = $this->db->prepare($query);
    $stmt->execute($params);
    return $stmt;
  }

- добавить getRow() и использовать в getById именно его.
- getRows переименовать в getAll чтобы не было путаницы и переписать так

public function getRows($query, array $params = array(), $mode= PDO::FETCH_ASSOC){
    return $this->query($query, $params)->fetchAll($mode);
  }

- либо добавить методы для всех методов PDO, либо сделать $db публичной. потому что когда тебе нужно будет выполнить десяток инсертов обернутых в транзакцию, эта обертка превратится в тыкву.

Comments

[Max Ba]

Спасибо за советы. Мы только вот пришли по моему к тому,что класс обертку делать из PDO было не нужно и я начал сомневаться, есть ли смысл было делать метод getRows, если можно было сразу вывести это внутри метода.

[FanatPHP]

Заметь, тебе все говорят одновременно "откажись от обертки" и "возьми готовую, их миллион". Есть в этом какое-то противоречие, ты не находишь?

По факту, ни один серьезный разработчик не работает с ПДО напрямую. Это следует делать только когда еще только учишься работать с ПХП. И написание своей обортки - это хорошая практика.

[FanatPHP]

В общем, судя по мешашине ответов, у тебя в голове стало куда больше мусора, чем до того как ты задал вопрос. Это печально.

[Max Ba]

FanatPHP, да не, всё четко. Переварю и завтра уже будет всё понятно)
Спасибо за советы!

[FanatPHP]

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

Answer 4

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

1. Мое личное мнение - использовать PDO как движок для своей обертки - очень плохая идея. PDO универсален, тем самым он ограничивает многие возможности. По сути, это весьма усложненная и неторопливая махина.

2. Метод query до неприличия простой. Я не вижу тут никаких плюшек, нет ни одного преимущества обертки над самим PDO. Я писал много времени обертку над MySql, можешь посмотреть, что обычно добавляется к возможностям.

3. У обертки не должно быть методов типа getRows, getColumn и тп. отсебятины. Её функция - обрабатывать запросы, отдавать ответы и какие-то вспомогательные функции, типа lastInsertId и отдавать ошибки. Все остальное должно реализовываться в вышестоящем слое, неважно как ты его назовешь.

4.

всё началось с фразы "никакой работы с БД не должно быть в модели".

это правильно и это ты правильно понял, абстрагировав слой БД от слоя модели. Советую почитать книгу Фаулера "Архитектуру корп. пр. приложений" и особенно главу "Источники данных". Без этой концепции ты дальше своей подделки не уедешь в знаниях.

Comments

[Max Ba]

можешь посмотреть

мощный класс))

это правильно и это ты правильно понял, абстрагировав слой БД от слоя модели. Советую почитать книгу Фаулера "Архитектуру корп. пр. приложений" и особенно главу "Источники данных". Без этой концепции ты дальше своей подделки не уедешь в знаниях.

А на двух словах можешь объяснить? Желательно кодом в три строки))

[FanatPHP]

Max Ba, имей в виду, что и этот ответ целиком, и "мощный" класс - это ад. Лучше всего просто развидеть его и не вступать в дискуссии. Поверь моему опыту.

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

Max Ba, нет конечно - в двух словах не получится. Объяснять что? Как строят приложения? Представь слоеный пирог, он состоит из разных слоев теста и начинки. Также и приложение должно строиться по принципу слоеного пирога, где о существовании соседа знают только ближайшие друг к другу слои.

Антипод этому - буханка хлеба, в которой все вы кучу и является монолитом.

В двух словах у тебя правильный подход - в модели используется НИЖЕСТОЯЩИЙ слой для работы с БД, в котором инкапсулированы механизмы по вытягиванию данных из БД. Вот это - и есть та слоистость приложения, разграничивающая ответственность слоев системы.

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

FanatPHP, ты как был хамло с 2000 года, когда на Xpoint-e говном исходил, так им и остался. У тебя аргументы есть, кроме "сам дурак"?

[DevMan]

php666, нафига тебе аргументы? я приводил тебе их не раз, но ты их игнорируешь.

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

DevMan, я не видел аргументов. Все что я на тостере вижу - раздутое ЧСВ их участников.

[DevMan]

Василий Берестов, пошерсти ответы своим предыдущим акам, увидишь.

Answer 5

[profesor08]

Чтоб получить имена столбцов и данные построчно, надо выполнить два одинаковых запроса. Спрашивается зачем? Можешь удалить весь код, и писать запросы станет легче.

Если же хочешь на классах, то тебе нужен один класс для создания запросов к бд, второй для обработки результата.

class Sql {
  public function query() {
    ....
    return new SqlResult($stmt);
  }
}

class SqlResult {
  public function __constructor($stmt) {
    $this->stmt = $stmt;
  }

  public function getCollumns() {
    return $this->stmt->fetchColumn();
  }

  public function getRows() {
    return $this->stmt->fetchAll(PDO::FETCH_ASSOC);
  }
}

Comments

[FanatPHP]

А зачем два запроса? выборка со строками уже содержит имена колонок. Так что второй одинаковый запрос выполнять по любому нет смысла.

[profesor08]

FanatPHP, это комментарий к коду автора

[FanatPHP]

К какому именно? $car->getAll() уже содержит имена столбцов

[profesor08]

FanatPHP, результат представляет собой ассоциативный массив, где ключи - имена столбцов. Но, что если строк в таблице ноль? Имена столбцов не узнать. Ничего о таблице не узнать. Вот по этому надо разделять создание запросов и работу с результатом запроса, тогда будет минимум непонятностей, минимум обращений к бд и максимум удобства.

[FanatPHP]

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

[profesor08]

FanatPHP, один запрос возвращает все результаты запроса, а так-же техническую информацию. Независимо от наличия результатов, будет техническая информация. Его класс не предоставляет получить техническую информацию, и лишь набор столбцов, для которых надо делать повторный запрос к бд. Вот я хочу вывести содержимое таблицы, при помощи getAll, я получу лишь первый результат - массив данных ключ -> строка. Чтоб узнать что за столбцы, какие у них типы и тд, мне надо делать повторный запрос getColumn.
Это все очевидно из его кода при беглом просмотре.

Это как долбить бд запросами в цикле.