Who can comment on PHP OOP code (Code review)?

Question

PHP

Hello!
I wrote a Singleton class in PHP - I need a person who will give his comments about the code. I'm trying to write clean code. The logic of the code works as it should.
Not entirely sure about the following things:

• Implementing a database connection
  
• Exceptions
  
• Singleton Implementation
  
• ... something else
  

The code itself:

<?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();
}

Answer 1

There are a lot of questions like this in the last couple of days. Some of the comments on your code are in this answer . I repeat.
You yourself wrote:
but, nevertheless, this class does everything that is not laziness:
Is it too much for one class?
As a result, we get a god-object that "does everything".
What kind of nonsense do you have written in the getInstance() method? Why throw an exception if the instance has already been created.

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

That is, you lose the whole point of the (anti) singleton pattern. So I can't do this:

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

Is there any logic? I think not.
Why are you storing the data for the database connection inside the method? Wouldn't it be logical to pass them as arguments to methods?
?
Abstract exceptions are not thrown! We create our own exceptions and inherit from them. In our code, we use only them so that we can easily process the necessary exceptions. The text of the exception would be nice to write in English.
Class names are capitalized! Parentheses after methods and classes are written on a new line:

function example() {
    // Не так
}

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

I propose to adhere to generally accepted standards for coding.
Try to use a singleton in this form to a minimum (or not use at all). Moreover, in this case, it is not needed at all.

Answer 2

Good afternoon,
I don't understand much in PHP, but I can try to comment.
For starters, not bad, you are trying to write your bike - ORM. Alas, you lack knowledge of architecture, principles and programming patterns.
The class is called treeData and it connects to the database, receives data, stores data, and also knows how to serialize / deserialize data. There is a violation of the Single Responsibility principle (read about the SOLID principles). Your class knows too much and is responsible for completely different things - you don’t need to write like that.
Separate the database connection into a separate class (singletone pattern).
Separate methods for getting/editing data into a separate class (repository & data mapper patterns).
Add the database connection object to the data retrieval class using dependency injection.
P.S. It seems to me that the singleton is implemented incorrectly. The point is to always have only one instance of the connection class. You check for existence and create a new instance (ok), but if the instance exists you throw an exception (why?). In the event that an instance exists, just take it and use it.
So:
1). Correctly implement singleton.
2). Read about SOLID principles and refactor, divide into different classes.
3). Read about singletone, unit of work, repository, data mapper design patterns. After that, it will be easier for you to write your own repository class, which will be responsible for getting / creating / changing / deleting objects from the database.
Good luck!

Answer 3

I'll put in my five cents.
First, everything looks pretty crappy. And now to the point.
1. Limping the naming of functions and their action:
a. If you name methods as abstractly as possible, then they must perform abstract things, you have specific actions
b. mySerialize from the same opera as mySuperUltraMegaMethodConcat2String
c. getFromDb - in general, it's not clear what the function does, the name of the method should reflect its essence. This method can be renamed to getById
2. I don’t see the oop, I see a solid static, which is equivalent to functionality.
You should have something like this in synthon

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. Don't comment on the obvious

//Запись в БД
        self::saveToDb();

picture with nicolas cage.jpg

Answer 4

I do not understand why this class is needed. truth. what do you want to do? usage example?

Answer 5

In a nutshell, your class is the bottom. You tried to combine fundamentally different things together, which is not correct. SOLID is important.
As for the design - PSR-1, PSR-2, PSR-4 - we teach and use.
Use scalar type hinting, it's really easier to live with it.
Sobsno what entities you have united:
1. The system for initializing the connection to the database. With you it's just hardcode, even hardcore. Outside of this class, we create a PDO and pass it as a constructor argument.
2. Storage of hierarchical data (your get and set) it is quite possible to leave them, just add validation.
3. Exceptions:
- \InvalidArgumentException - "you pushed some crap"
- \DomainException - "data error", for example, there is no user with such login.
- \LogicalException - "I don't know how this happened", for example, we delete a non-existent file
- \RuntimeException - "I don't know what to do with this", for example, we try to write to a file that someone deleted
4. A serializer, it will not hurt either transferring JSON to the database is a bad idea
5. Working with the database should be moved to a separate Repository class

<?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();

    }

}

Answer 6

It's not OOP at all!