A
A
Alexander2017-10-01 22:18:17
PHP
Alexander, 2017-10-01 22:18:17

How correct is my OOP php code?

I am mastering OOP and moving from the procedural style, I want to understand if I have learned the material correctly so that I can immediately write "without crutches" ... Here is my first class:

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>. Уведомление было отправлено администратору!";
        }
    }
}

How correct, convenient code?! Please no jokes, only constructive, thanks :)

Answer the question

In order to leave comments, you need to log in

5 answer(s)
D
D3lphi, 2017-10-01
@SmoKE_xD

It's bad here, unfortunately.
Let's start with the fact that you inherit classes incorrectly. Why is your class responsible for connecting to the database a parent of the class that works with orders? Inheritance is applied if it is possible to say that something is something. For example, a developer is an employee; the computer is a device, and so on. Here, you can’t follow such logic at all. You must pass at least an object to work with the database through injection, for example, to a constructor. Ideally, one should use the repository pattern to work with the database.
The SearchOrder class not only executes queries, but also works with data, stores the state of this very data, and filters data (strip_tags()). Disorder. It all needs to be shared. In general, you get some kind of god-objects that can do everything.
Each time you repeat the lines with preparing a request, binding parameters, sending a request, and so on. We didn’t think that it would be nice to write some kind of wrapper and execute queries like this:
?
You are calling connect() in methods. That is, each call to this method will result in a new database connection being established, even if one has already been established. Connecting to a database is quite an expensive operation.
Why are you using properties when you can get by with regular local variables:

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

?
Why are you stripping tags from the id? you are so unsure about what flies into the function:
?
If you are not using php 7 and, as a result, scalar type hinting, then you should do checks for the type of the incoming argument. If something is wrong with the type, throw an exception (rather than cast it to the right one)! For example:
if (!is_string($arg)) {
    throw new InvalidArgumentTypeException('string', $arg);
}

This is ideal. You don't have to, of course. But such checks make the application safer. Although, again, I repeat, in 2017 you need to start new projects on php 7.1+.
Errors do not need to be thrown in this class. You have to catch the database exception, convert it to a domain exception and forward it further and somewhere, at a higher level, display information to the user about the error. In an mvc system, for example, this is done in the controller.
Among other things, read about coding standards. You don't follow them.
It's too early for you to write such bicycles. Apparently, you have no experience at all. Look at ready-made solutions: frameworks, ORM, study them, at least superficially understand how it works and only then try to do something based on the knowledge gained.
Wish you luck!

V
Vyacheslav Uspensky, 2017-10-01
@Kwisatz

The main mistakes have already been pointed out above. Also, capitalize classes. And it's better to call the class Connector and not connect. You are working with objects.
It is important for you to understand that OOP has grown for a reason. One of the main goals is to speed up development and ease of support. Your code should be written in such a way that it is easy to manipulate objects. Simplified $lion->eat($meat); Simple, clear, short 8)
In addition to the errors above. Read about DataMapper/ActiveRecord

M
MaximMRX, 2017-10-01
@GM_pAnda

Read the documentation about PSR-4, then it will become more clear about all the naming and so on

D
dmitriy, 2017-10-01
@dmitriylanets

not correct at all, there should be no inheritance, dependency injection should be

D
Don Gan, 2017-10-02
@PravdorubMSK

$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);
wrappers should be used instead of this trash.
there are 2 on the network:
https://github.com/Vasiliy-Makogon/Database - my
phpfaq.ru/SafeMysql - someone else's

Didn't find what you were looking for?

Ask your question

Ask a Question

731 491 924 answers to any question