Где допущены ошибки и как можно улучшить код?

Question

[S1MY]

Доброго вечера!
На собеседование задали данный вопрос и хотелось бы разобраться в нём, погуглив нашёл этот код 8-ми летней давности, но разбора его не обнаружил, думаю кроме вас мне не подскажет в чём тут явные проблемы.
Заранее спасибо за уделённое время!

<?php
class Document {

    public $user;

    public $name;

    public function init($name, User $user) {
        assert(strlen($name) > 5);
        $this->user = $user;
        $this->name = $name;
    }

    public function getTitle() {
        $db = Database::getInstance();
        $row = $db->query('SELECT * FROM document WHERE name = "' . $this->name . '" LIMIT 1');
        return $row[3]; // third column in a row
    }

    public function getContent() {
        $db = Database::getInstance();
        $row = $db->query('SELECT * FROM document WHERE name = "' . $this->name . '" LIMIT 1');
        return $row[6]; // sixth column in a row
    }

    public static function getAllDocuments() {
        // to be implemented later
    }

}

class User {

    public function makeNewDocument($name) {
        $doc = new Document();
        $doc->init($name, $this);
        return $doc;
    }

    public function getMyDocuments() {
        $list = array();
        foreach (Document::getAllDocuments() as $doc) {
            if ($doc->user == $this)
                $list[] = $doc;
        }
        return $list;
    }  

}

Comments

[Сергей delphinpro]

Улучшить нельзя выкинуть

Answer 1

[ThunderCat]

Штош...

1) Элементарные ошибки на уровне запросов с инжекциями.
2) Класс документ зачем-то получает объект юзера, нигде его не использует, тем не менее даже при использовании нарушает принцип инкапсуляции, объекты не должны по логике знать о других объектах.
3) Класс юзер так же нарушает принципы инкапсуляции и единой ответственности, в частности зачем-то работает с документом, а не занимается обслуживанием сущности юзера, опять же, класс не должен знать о других сущностях, он должен реализовывать только логику собственной сущности.
4) методы getTitle() и getContent(), кроме того что не указывают явно на необходимые поля, надеясь на неизменность сущности в бд, еще и накладывают множественную нагрузку, по сути на каждый чих выполняя один и тот же запрос, вместо 1 запроса инициализации данных и далее обращения к уже существующим атрибутам.
5) Отсутствие неймспейсов и изоляция кода по классам так же отсутствует, соответственно про нормальный автолоад можно забыть.

Вывод: говнокод в чистом виде.

Comments

[Михаил Ливач]

объекты не должны по логике знать о других объектах

Можете пояснить на данном коде? То есть я вижу, что Document не использует User в приведённом коде, но мог бы использовать, и тогда всё по канонам DI , разве нет?

[ThunderCat]

Михаил Ливач,

и тогда всё по канонам DI , разве нет?

Нет. В данном случае у вас объекты-сущности, отражающие какой-то тип реальных объектов (документов, пользователей, деревьев, трусов - не суть), для их взаимодействия не нужно как-то показывать им друг на друга, все взаимодействия между ними могут быть прописаны в коде, использующем их в качестве независимых единиц (хороший пример - код в контроллере, если вы понимаете концепцию MVC). DI же передает [сервисные] объекты, без которых сам объект будет либо не функционален (например та же БД для модели типа Active Record), либо обслуживающий код будет часто повторяться и есть смысл вынести их взаимодействие в метод объекта (если лень), или создать объект взаимодействия (что правильнее), как например при проверке ролей и прав в RBAC/ACL, но тут я не вижу никаких к тому предпосылок.

[Михаил Ливач]

ThunderCat, Вы сторонник анемичных моделей?

Answer 2

[Rsa97]

Добавлю свои пять копеек.
- публичные свойства $user и $name;
- функция init вместо конструктора класса;
- (холивар) использование синглтона Database вместо внедрения зависимости.

Answer 3

[Антон Антон]

Это на собеседовании вопрос? сходу огромная дыра для sql injection, выбор лишних данных, все сломается при изменении структуры таблицы.