Can anyone give comments about PHP OOP (Code review)?

Hello!
Wrote a Singleton class in PHP - need someone who will give their comments about the code. Try to write "clean code". The code logic works as it should.
Not quite sure of the following things:
  • Implementation of database connection
  • Exceptions
  • The implementation of singleton
  • ... something else

The code itself:

<?php

/**
 * Class treeData
 * Singleton class for multidimensional arrays custom
*/

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("Attempt to re-create the instance of Singleton class");
}
 return self::$_instance;
}

/**
 * DB connection
 * @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;
}

/**
 * Saving to the database.
*/
 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("user ID must be an integer.");
}
 self::$pdo = self::db();
 self::$id = $id;
 $row = self::getFromDb($id);
 $data = $row['data'];

 //Prepare data
 if(empty($data)){ //the Array is empty, create a new
 self::$data = array();
 } else{ //Work with data
 self::$data = self::myDeserialize($data);
}
}

/**
 * To prevent cloning of the object
*/
 private function __clone() { //disallow clone of the object with the private modifier
}

/**
 * Static function to serialize the array
 * @param $data
 * @return string
*/
 private static function mySerialize($data){
 return json_encode(serialize($data));
}

/**
 * Static function for deserilization array
 * @param $data
 * @return mixed
*/
 private static function myDeserialize($data){
 return unserialize(json_decode($data));
}

/**
 * Provides the possibility of obtaining specific variable from the private data variable
 * @param String $path
 * @return array
 * @throws Exception
*/
 public static function get($path){
 $pathArray = explode('/', $path); //Get an array with the path
 $level = self::$data; //Initial array
 foreach ($pathArray as $key){
 if(array_key_exists($key, $level)){
 $level = $level[$key];
 } else {
 throw new Exception("Index '$key' does not exist");
}
}
 return $level;
}

/**
 * bespechivaet the ability to install new or replace the current variable.
 * @param $path
 * @param $value
*/
 public static function set($path, $value){
 //Get the path
 $pathArray = explode('/', $path);
 //Made to the array data
 $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;
}
 //Write to DB
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();
}
June 14th 19 at 20:47
6 answers
June 14th 19 at 20:49
That's a lot of these issues over the last couple of days. Part of comments to your code is in this answer. Again.

You yourself wrote:
class for working with multidimensional arrays custom

but nevertheless, this class does everything that is not lazy:
  • Working with these same arrays
  • Connects to the database
  • Sends queries to the database
  • Processes data

Not much there for one class?
In the end, we get logobject, who "knows all".

What do you have for nonsense is written in the method getInstance()? Why throw an exception in case the instance has already been created.

if (self::$_instance === null) {
 self::$_instance = new self($id);
} else {
 throw new Exception("Attempt to re-create the instance of Singleton class");
}
return self::$_instance;


That is, you have lost all sense (anti)pattern singleton. It turns out, I won't be able to do so:

$instance1 = treeData::getInstance();
$instance2 = treeData::getInstance(); // Exception!

There is a logic? I think not.

Why do you store data for the database connection inside the method? Not logical would be to pass them as arguments to methods?

Every time you repeat the rows with the preparation of the request, bindingon parameters, sending the request, and so on. Didn't think it was a good idea to write some kind of wrapper, and query something like:
$result = $wrapper->select("SELECT * FROM `tablename` WHERE `id` = :id", ['id' => 5]);

?


Abstract no exception throw! Create your exception and inherit from them. In your code only use them so that you can easily handle need exception's. The exception text would be a good idea to write in English.

Write the class names with a capital letter! Parentheses after the methods and classes you write on a new line:
function example() {
 // Not so
}

function example()
{
 // And here
}


I propose to adhere to the generally accepted coding standards.

Try to use singleton like this to a minimum (Or no use). Moreover, in this case, it is not needed.
Thank you very much for Your answer!
Initially the code looked like this:
public static function getInstance($id) {
 if (self::$_instance === null) {
 self::$_instance = new self($id);
}
 return self::$_instance;
 }

but in this case, if a careless programmer will create 2 instance of a class:
$instance1 = treeData::getInstance(1);
$instance2 = treeData::getInstance(2);

and will work with the instance $instance2, thinking that it works with user ID 2, but in fact it will work with user ID 1 (for example: $instance2->get(); will query the user data with ID = 1 and not ID = 2 as the programmer).
In this case, to warn the programmer about what is singleton? - nikita.Stracke commented on June 14th 19 at 20:52
why do you even want to forbid to create more than one instance of the object, which is a maximum of one times will be used in the script for one of his call? So never write, first time I see this. You have the statement of the problem something wrong here. - August80 commented on June 14th 19 at 20:55
You do not understand the essence of the pattern singleton. Singleton should be:
if (self::$_instance === null) {
 self::$_instance = new self($id);
}
return self::$_instance;

That is, in our application there is only 1 instance of the class. No matter how much we called the method getInstance(), it will return the same object. What you do is not a singleton. - Manuela.Ho commented on June 14th 19 at 20:58
The exception text would be a good idea to write in English.

This prejudice, the text needs to be responsible for understanding, not just in English.
Parentheses after the methods and classes you write on a new line:

The card will fall, it is not a dogma. For classes it is better to write the bracket in place ads, it is better to vary the class itself. - Beth_Kuphal commented on June 14th 19 at 21:01
dogma.
Classes - haven't heard that word.
Here's an example of a simple object on the base code in 80 characters.
Vary it's best if bracket as standard, bottom. Actually as it is written in the PSR*
59d2a2fc6c9f7749333399.png - Austin commented on June 14th 19 at 21:04
personally I have is a hostility code, comments, exception messages and so on. written in a language other than English.

The bracket should be written on a separate line, not to use it if, for example, want to implementit interface.
In any case, it's standard. And it should stick. - Manuela.Ho commented on June 14th 19 at 21:07
June 14th 19 at 20:51
Good day,
I don't know much PHP, but can try to comment.

It's a good start, you try to write their own bike - ORM. Alas, you lack knowledge on architecture, principles and patterns of programming.

The treeData class is called and it connects to database, retrieves data, saves data, and also knows how to serialize/deserialize data. On the face of the violation of the principle of Single Responsobility (read about the SOLID principles). Your class too much can and atbetween for completely different things - so no need to write.

Select the database connection in a separate class (singletone pattern).
Will highlight the methods of receiving/editing the data in a separate class (repository & data mapper patterns).
Object database connection, add the class receive data using an injection dependency injection ().

PS I think the singleton is implemented incorrectly. The point is, that there is always only one instance of the connection class. You check for existence and create a new instance (OK), but if the instance exists, throw an exception (why?). In that case, if the instance exists, just take it and use it.

So:
1). To properly implement a singleton.
2). Read about SOLID principles, and refactoring, divided into different classes.
3). To read about the singletone design patterns, unit of work, repository, data mapper. After that you will be easier to write a class repository that will atbetween for retrieve/create/change/delete objects from the database.

Good luck!
it's not ORM, that's some bullshit - nikita.Stracke commented on June 14th 19 at 20:54
June 14th 19 at 20:53
Will contribute my two cents.
First, it looks quite crappy. And now to the point.

1. Lame naming of functions and their effects:
a. If you call methods in the abstract as possible, then they must perform to abstract things, you have specific actions
b. mySerialize from the same Opera that mySuperUltraMegaMethodConcat2String
c. getFromDb - all nichrome not clear what the function does, the method name should reflect its essence. This method can be renamed to getById
2. Don't see the PLO, see a continuum of static, which is equivalent to functionalise.
You have the singleton you should get something like this
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. Do not comment the obvious
//Write to DB
 self::saveToDb();

The picture Nicolas keycard
June 14th 19 at 20:55
I do not understand why this class is needed. true. what you want to do that? an example of use?
June 14th 19 at 20:57
In a nutshell, your class is the bottom. You tried to bring together fundamentally different things, which is not correct. SOLID is important.
As for design - PSR-1, PSR-2, PSR-4 - learn and use.
Yuzayte scalar typehinting, with it really is easier to live.

Sobsno what entity you merged:
1. System initialize the connection to the database. You it's just hardcod, even hardcore. Outside of this class to create a PDO and pass a constructor argument.
2. Store hierarchical data (your get and set) it is possible to write and add validation.
3. Exceptions:
- \InvalidArgumentException - "you shoved some crap"
- \DomainException - "data error", for example, a user with this login, no.
- \LogicalException - "I don't know how that happened", for example, delete not existing file
- \RuntimeException - "I don't know what to do with it", for example trying to write to a file that someone deleted
4. The serializer, it does not hurt to bear, and JSON in the database to store is a bad idea
5. Work with the database is put into a separate Repository

<?php

declare(strict_types=1); // !!!! Forgot

namespace MyVendor\MyProject\Path\To; // !!!! Forgot 

/**
 * Class treeData
 * Singleton class for multidimensional arrays custom
*/

// !!!! Start with distant: Singleton anti-pattern, yet digest this information

class treeData
{

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

/**
 * @param $id
 * @return treeData
 * @throws Exception
*/
 public static function getInstance(/* !!!! use scalar typehinting */$id) { // !!!! Do not use static methods
 if (self::$_instance === null) {
 self::$_instance = new self($id);
 } else {
 // !!!! 
 throw new Exception("Attempt to re-create the instance of Singleton class");
}
 return self::$_instance;
}

/**
 * DB connection
 * @return PDO // !!!! can \PDO?
*/
 private static function db(){ // !!!! Do not use static methods
 // !!!! This method is not the place. Connection to the database is a completely separate entity, and you have it sehardcore create 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 // !!!! And type where?
 * @return mixed
*/
 private static function getFromDb(/* !!!! use scalar typehinting */$id){ // !!!! I can not understand, that's why you need this method?
 // !!!! Database - separately, of Petrovichi tree separately, do not hinder all in a heap!
 $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;
}

/**
 * Saving to the database.
*/
 private static function saveToDb() {// !!!! I can not understand, that's why you need this method?
 // !!!! Database - separately, of Petrovichi tree separately, do not hinder all in a heap!
 $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 // !!!! To specify a type religion does not allow?))
 * @throws Exception
*/
 private function __construct(/* !!!! use scalar typehinting */$id) {
if(!is_int($id)){
 throw new Exception("user ID must be an integer.");
 }// !!!! What if id = -256?
 self::$pdo = self::db();
 self::$id = $id;
 $row = self::getFromDb($id);
 $data = $row['data'];

 //Prepare data
 if(empty($data)){ //the Array is empty, create a new
 self::$data = array();
 } else{ //Work with data
 self::$data = self::myDeserialize($data);
}
}

/**
 * To prevent cloning of the object
*/
 private function __clone() { //disallow clone of the object with the private modifier
}

/**
 * Static function to serialize the array
 * @param $data
 * @return string
*/
 private static function mySerialize(/* !!!! use scalar typehinting */$data){ // !!!! Why serialization in the class work with database?
 return json_encode(serialize($data));
}

/**
 * Static function for deserilization array
 * @param $data
 * @return mixed
*/
 private static function myDeserialize(/* !!!! use scalar typehinting */$data){ // !!!! Why deserialize into a class for working with DB?
 return unserialize(json_decode($data));
}

/**
 * Provides the possibility of obtaining specific variable from the private data variable
 * @param String $path // !!!! In php there is no type String, got string
 * @return array
 * @throws Exception
*/
 public static function get(/* !!!! use scalar typehinting */$path){ // !!!! get what?
 // !!!! What if path is an exception?
 $pathArray = explode('/', $path); //Get an array with the path
 $level = self::$data; //Initial array
 foreach ($pathArray as $key){
 if(array_key_exists($key, $level)){
 $level = $level[$key];
 } else {
 throw new Exception("Index '$key' does not exist");
}
}
 return $level;
}

/**
 * bespechivaet the ability to install new or replace the current variable.
 * @param $path // !!!! where data types?
 * @param $value
*/
 public static function set(/* !!!! use scalar typehinting */$path, $value){ // !!!! set what?
 // !!!! What if path is an exception?
 //Get the path
 $pathArray = explode('/', $path);
 //Made to the array data
 $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;
}
 //Write to DB
self::saveToDb();

}

}
June 14th 19 at 20:59
This is not OOP!

Find more questions by tags PHPOOPCode review