Задать вопрос
@georgenov
Пытаюсь программировать на PHP

Кто может дать комментарии по поводу кода PHP ООП (Code review)?

Здравствуйте!
Написал Singleton класс на PHP - необходим человек, который даст свои комментарии по поводу кода. Пытаюсь писать "чистый код". Логика кода работает как надо.
Не совсем уверен в следующих вещах:
  • Реализация соединения с БД
  • Исключения
  • Реализация синглтона
  • ... что-то еще

Сам код:

<?php

/**
 * Class treeData
 * Singleton класс для работы с многомерными пользовательскими массивами
 */

class treeData
{

    protected static $_instance, $data, $pdo, $id;

    /**
     * @param $id
     * @return treeData
     * @throws Exception
     */
    public static function getInstance($id) {
        if (self::$_instance === null) {
            self::$_instance = new self($id);
        } else {
            throw new Exception("Попытка повторного создания экземпляра Singleton класса");
        }
        return self::$_instance;
    }

    /**
     * Соединение с БД
     * @return PDO
     */
    private static function db(){
        $host = '127.0.0.1';
        $db = 'for_test';
        $user = 'root';
        $pass = '';
        $charset = 'utf8';
        $dsn = "mysql:host=$host;dbname=$db;charset=$charset";
        try {
            $pdo = new PDO($dsn, $user, $pass);
        }
        catch (PDOException $e){
            die($e->getMessage());
        }
        return $pdo;
    }

    /**
     * @param $id
     * @return mixed
     */
    private static function getFromDb($id){
        $sql = "SELECT `data` FROM tree2 WHERE id = ? LIMIT 1";
        $stmt = self::$pdo->prepare($sql);
        $stmt->execute(array($id));
        $row = $stmt->fetch(PDO::FETCH_LAZY);

        return $row;
    }

    /**
     * Сохранение в БД.
     */
    private static function saveToDb() {
        $sql = "UPDATE tree2 SET `data` = :data WHERE id = :id";
        $stmt = self::$pdo->prepare($sql);
        $stmt->execute(array(':id' => self::$id, ':data' => self::mySerialize(self::$data)));
    }

    /**
     * treeData constructor.
     * @param $id
     * @throws Exception
     */
    private  function __construct($id) {
        if(!is_int($id)){
            throw new Exception("ИД пользователя должно быть целым числом.");
        }
        self::$pdo = self::db();
        self::$id = $id;
        $row = self::getFromDb($id);
        $data = $row['data'];

        //Подготовка данных
        if(empty($data)){ //Массив пуст, создаем новый
            self::$data = array();
        } else{ //Работаем с данными
            self::$data = self::myDeserialize($data);
        }
    }

    /**
     * Для предотвращения возможного клонирования объекта
     */
    private function __clone() { //запрещаем клонирование объекта модификатором private
    }

    /**
     * Статическая функция для сериализации масива
     * @param $data
     * @return string
     */
    private static function mySerialize($data){
        return json_encode(serialize($data));
    }

    /**
     * Статическая функция для десиарилизации массива
     * @param $data
     * @return mixed
     */
    private static function myDeserialize($data){
        return unserialize(json_decode($data));
    }

    /**
     * Обеспечивает возможность получения конкретной переменной из приватной переменной data
     * @param String $path
     * @return array
     * @throws Exception
     */
    public static function get($path){
        $pathArray = explode('/', $path); //Получаю массив с путем
        $level = self::$data; //Начальный массив
        foreach ($pathArray as $key){
            if(array_key_exists($key, $level)){
                $level = $level[$key];
            } else {
                throw new Exception("Индекса '$key' не существует");
            }
        }
        return $level;
    }

    /**
     * беспечивает возможность установки новой или замещения текущей переменной.
     * @param $path
     * @param $value
     */
    public static function set($path, $value){
        //Получаем путь
        $pathArray = explode('/', $path);
        //Вносим в массив данные
        $level =& self::$data;
        foreach ($pathArray as $key) {
            if (!array_key_exists($key, $level) or !is_array($level[$key])) {
                $level[$key] = [];
            }
            $level =& $level[$key];
        }
        if(is_array($level) && is_array($value)){
            $level = array_merge($level, $value);
        } else {
            $level = $value;
        }
        //Запись в БД
        self::saveToDb();

    }

}
try{
$z = treeData::getInstance(1);
//$z::set("1/3/4", array('test1', 'test2'));
$z::set("1/3/4", array('test3'));
    //$z::set("1", array("main_thread" => "main"));
var_dump($z::get("1"));
}
catch (Exception $e){
    echo $e->getMessage();
}
  • Вопрос задан
  • 754 просмотра
Подписаться 1 Средний 4 комментария
Пригласить эксперта
Ответы на вопрос 6
@D3lphi
Что-то многовато таких вопросов за последние пару дней. Часть замечаний к вашему коду есть в этом ответе. Повторюсь.

Вы же сами написали:
класс для работы с многомерными пользовательскими массивами

но, тем не менее, этот класса делает все, что не лень:
  • Работает с этими самыми массивами
  • Соединяется с базой данных
  • Отправляет запросы к базе данных
  • Занимается обработкой данных

Не многовато ли для одного класса?
В итоге, мы получаем богообъект, который "умеет во всё".

Что у вас за бред написан в методе getInstance()? Зачем бросать исключение, в случае, если инстанс уже был создан.

if (self::$_instance === null) {
    self::$_instance = new self($id);
} else {
    throw new Exception("Попытка повторного создания экземпляра Singleton класса");
}
return self::$_instance;


То есть, у вас теряется весь смысл (анти)паттерна синглотон. Получается, я не смогу сделать так:

$instance1 = treeData::getInstance();
$instance2 = treeData::getInstance(); // Исключение!

Есть логика? Я думаю, что нет.

Почему вы храните данные для соединения с БД внутри метода? Не логично ли было бы передавать их в качестве аргументов к методам?

Вы каждый раз повторяете строки с подготовкой запроса, биндингом параметров, отправкой запроса и тд. Не думали, что неплохо бы было написать какую-нибудь обертку и выполнять запросы как-нибудь так:
$result = $wrapper->select("SELECT * FROM `tablename` WHERE `id` = :id", ['id' => 5]);

?


Абстрактные исключения не бросаем! Создаем свои исключения и наследуемся от них. В своем коде используем только их, дабы можно было легко обработать нужные exception'ы. Текст исключения неплохо бы было писать на английском.

Имена классов пишем с большой буквы! Скобки после методов и классов пишем на новой строке:
function example() {
    // Не так
}

function example()
{
    // А вот так
}


Предлагаю придерживаться общепринятым стандартам оформления кода.

Старайтесь использовать синглтон в таком виде по минимуму (Или вообще не использовать). Тем более, в данном случае, он вообще не нужен.
Ответ написан
@Oblomingo
Добрый день,
я не много понимаю в PHP, но могу попробовать прокомментировать.

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

Класс называется treeData и он соединяется с базой данных, получает данные, сохраняет данные, а также умеет сериализовать/десериализовать данные. На лицо нарушение принципа Single Responsobility (почитайте про SOLID принципы). Ваш класс слишком много умеет и ответвенен за совершенно разные вещи - так писать не надо.

Выделите соединение к базе данных в отдельный класс (singletone pattern).
Выделите методы получения/редактирования данных в отдельный класс (repository & data mapper patterns).
Обьект подключения базы данных добавляйте в класс получения данных с помощью иньекции зависимостей (dependency injection).

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

Итак:
1). Правильно реализовать синглетон.
2). Почитать о SOLID принципах и провести рефакторинг, разделить на разные классы.
3). Почитать о паттернах проектирования singletone, unit of work, repository, data mapper. После этого вам легче будет написать свой класс репозитории, который будет ответвенен за получение/создание/изменение/удаление обьектов из базы данных.

Удачи!
Ответ написан
@Fetur
В карман за ответом не полезу
Внесу свои пять копеек.
Во-первых, все выглядит достаточно дерьмового. А теперь к сути.

1. Хромает именование функций и их действие:
a. Если называешь методы максимально абстрактно, то они и должны выполнять абстрактные вещи, у тебя конкретные действия
b. mySerialize из той же оперы что и mySuperUltraMegaMethodConcat2String
c. getFromDb - вообще нихрена не ясно, что делает функция, название метода должно отражать ее сущность. Этот метод можно переименовать в getById
2. Не вижу ооп, вижу сплошной статик, который равнозначен функциональщине.
У тебя в сингтоне должно получиться что-то типа этого
class Tree
{
private $inst;

__const() {
  return $inst === null ? new self : $inst; 
}

public function getById($id) {
   return TreeModel::find($id);
}

public function TreeSectionExist($id) {
   return !is_null($this->getById($id)) ? true : false;
}

$a = new Tree();
}


3. Не комментируйте очевидное
//Запись в БД
        self::saveToDb();

Картинка с николасом кейджам.jpg
Ответ написан
Комментировать
PravdorubMSK
@PravdorubMSK
я вообще не понимаю зачем этот класс нужен. правда. что вы сделать то хотите? пример использования?
Ответ написан
Комментировать
index0h
@index0h
PHP, Golang. https://github.com/index0h
Если в двух словах, ваш класс - днище. Вы попытались объединить вместе принципиально разные вещи, что не правильно. SOLID - это важно.
Что касается оформления - PSR-1, PSR-2, PSR-4 - учим и используем.
Юзайте скалярный тайпхинтинг, с ним реально проще жить.

Собсно какие сущности у вас объеденены:
1. Система инициализации подключения к БД. У вас это просто хардкод, даже хардкор. Во вне этого класса создаем PDO и передаем аргументом конструктора.
2. Хранилище иерархических данных (ваши get и set) их вполне можно оставить, только добавьте валидацию.
3. Исключения:
- \InvalidArgumentException - "ты впихнул какую-то хрень"
- \DomainException - "ошибка данных", например пользователя с таким логином нет.
- \LogicalException - "я хз как такое произошло", например удаляем не существующий файл
- \RuntimeException - "я хз, что с этим делать", например пытаемся записать в файл, который кто-то удалил
4. Сериализатор, его тоже не помешает вынести и JSON в бд хранить - плохая идея
5. Работу с БД стоит вынести в отдельный класс Repository

<?php

declare(strict_types=1); // !!!! Забыли

namespace MyVendor\MyProject\Path\To; // !!!! Забыли 

/**
 * Class treeData
 * Singleton класс для работы с многомерными пользовательскими массивами
 */

// !!!! Начну с далека: Singleton - антипаттерн, пока что переваривайте эту информацию

class treeData
{

    protected static $_instance, $data, $pdo, $id;

    /**
     * @param $id
     * @return treeData
     * @throws Exception
     */
    public static function getInstance(/* !!!! используйте скалярный тайпхинтинг */$id) { // !!!! Не используйте статические методы
        if (self::$_instance === null) {
            self::$_instance = new self($id);
        } else {
            // !!!! 
            throw new Exception("Попытка повторного создания экземпляра Singleton класса");
        }
        return self::$_instance;
    }

    /**
     * Соединение с БД
     * @return PDO // !!!! может \PDO?
     */
    private static function db(){ // !!!! Не используйте статические методы
        // !!!! Этому методу тут не место. Connection к БД - это вполне отдельная сущность, а у вас это захардкоженное создание PDO
        $host = '127.0.0.1';
        $db = 'for_test';
        $user = 'root';
        $pass = '';
        $charset = 'utf8';
        $dsn = "mysql:host=$host;dbname=$db;charset=$charset";
        try {
            $pdo = new PDO($dsn, $user, $pass);
        }
        catch (PDOException $e){
            die($e->getMessage());
        }
        return $pdo;
    }

    /**
     * @param $id // !!!! А тип где?
     * @return mixed
     */
    private static function getFromDb(/* !!!! используйте скалярный тайпхинтинг */$id){ // !!!! Я понять не могу, вот зачем вам этот метод?
        // !!!! БД - отдельно, претрубации с деревом - отдельно, не мешайте все в кучу!
        $sql = "SELECT `data` FROM tree2 WHERE id = ? LIMIT 1";
        $stmt = self::$pdo->prepare($sql);
        $stmt->execute(array($id));
        $row = $stmt->fetch(PDO::FETCH_LAZY);

        return $row;
    }

    /**
     * Сохранение в БД.
     */
    private static function saveToDb() {// !!!! Я понять не могу, вот зачем вам этот метод?
        // !!!! БД - отдельно, претрубации с деревом - отдельно, не мешайте все в кучу!
        $sql = "UPDATE tree2 SET `data` = :data WHERE id = :id";
        $stmt = self::$pdo->prepare($sql);
        $stmt->execute(array(':id' => self::$id, ':data' => self::mySerialize(self::$data)));
    }

    /**
     * treeData constructor.
     * @param $id // !!!! Указать требуемый тип религия не позволяет?))
     * @throws Exception
     */
    private  function __construct(/* !!!! используйте скалярный тайпхинтинг */$id) {
        if(!is_int($id)){
            throw new Exception("ИД пользователя должно быть целым числом.");
        }// !!!! А что если id = -256?
        self::$pdo = self::db();
        self::$id = $id;
        $row = self::getFromDb($id);
        $data = $row['data'];

        //Подготовка данных
        if(empty($data)){ //Массив пуст, создаем новый
            self::$data = array();
        } else{ //Работаем с данными
            self::$data = self::myDeserialize($data);
        }
    }

    /**
     * Для предотвращения возможного клонирования объекта
     */
    private function __clone() { //запрещаем клонирование объекта модификатором private
    }

    /**
     * Статическая функция для сериализации масива
     * @param $data
     * @return string
     */
    private static function mySerialize(/* !!!! используйте скалярный тайпхинтинг */$data){ // !!!! Зачем сериализация в классе по работе с БД?
        return json_encode(serialize($data));
    }

    /**
     * Статическая функция для десиарилизации массива
     * @param $data
     * @return mixed
     */
    private static function myDeserialize(/* !!!! используйте скалярный тайпхинтинг */$data){ // !!!! Зачем десериализация в классе по работе с БД?
        return unserialize(json_decode($data));
    }

    /**
     * Обеспечивает возможность получения конкретной переменной из приватной переменной data
     * @param String $path // !!!! В php нет типа String, есть string
     * @return array
     * @throws Exception
     */
    public static function get(/* !!!! используйте скалярный тайпхинтинг */$path){ // !!!! get что?
        // !!!! Что если path - исключение?
        $pathArray = explode('/', $path); //Получаю массив с путем
        $level = self::$data; //Начальный массив
        foreach ($pathArray as $key){
            if(array_key_exists($key, $level)){
                $level = $level[$key];
            } else {
                throw new Exception("Индекса '$key' не существует");
            }
        }
        return $level;
    }

    /**
     * беспечивает возможность установки новой или замещения текущей переменной.
     * @param $path // !!!! где типы данных?
     * @param $value
     */
    public static function set(/* !!!! используйте скалярный тайпхинтинг */$path, $value){ // !!!! set что?
        // !!!! Что если path - исключение?
        //Получаем путь
        $pathArray = explode('/', $path);
        //Вносим в массив данные
        $level =& self::$data;
        foreach ($pathArray as $key) {
            if (!array_key_exists($key, $level) or !is_array($level[$key])) {
                $level[$key] = [];
            }
            $level =& $level[$key];
        }
        if(is_array($level) && is_array($value)){
            $level = array_merge($level, $value);
        } else {
            $level = $value;
        }
        //Запись в БД
        self::saveToDb();

    }

}
Ответ написан
Комментировать
@egormmm
Борітеся — поборете!
Это вообще не ООП!
Ответ написан
Комментировать
Ваш ответ на вопрос

Войдите, чтобы написать ответ

Похожие вопросы