Правильно ли я использую делегирование?

Question

[Kolya Vantukh]

Нужен совет правильно ли я использую делегирование . Есть на сайте регистрация , где осуществляется загрузка фото пользователя , есть возможность менять эти изображения в личном кабинете , и еще нужно выводить эти изображения в вюшке пользователю .
В общем есть модель User

class User {
  
    public $image;
    
    public function __construct($image) 
    {
       $this->image = $image;
    }
    
    public function getUser($id) 
    {
        
        $db = Db::getConnection();
       
        $sql = "SELECT * FROM user  WHERE id = :id";
        $stmt = $db->prepare($sql);
        $stmt->bindParam(':id', $id, PDO::PARAM_INT);

        $stmt->execute();
        
        return $stmt->fetch();
    
    }
 
}

Есть модель Image для работы с изображениями

class Image {
    
    private $user_id;
    
    public function __construct($id) 
    {
        $this->user_id = $id;
    }
    public function getImage() 
    {
    
    $db = Db::getConnection();
   
    $sql = "SELECT  IF(image IS NULL or image = '','no_image.png',image) as image  FROM `user` WHERE id = :id";
      
       
        $result = $db->prepare($sql);
        $result->bindParam(':id', $this->user_id, PDO::PARAM_INT);
        $result->execute();
        
        return $result->fetchColumn();
    }
    
    public static function UploadImage($image) 
    {
       
        $usersImage = $image['userfile']['tmp_name'];
        
        $imageName = $image['userfile']['name'];
        
        if(is_uploaded_file($image['userfile']['tmp_name'])){
        
            move_uploaded_file($usersImage, $_SERVER['DOCUMENT_ROOT']."/template/images/users/".$image['userfile']['name']);
             
        
        }
     
        return $imageName;
        
     }
     public function UpdateImage($image) {
       
        $usersImage = $image['userphoto']['tmp_name'];
        
        $imageName = $image['userphoto']['name'];
        
        if(is_uploaded_file($image['userphoto']['tmp_name'])){
        
            move_uploaded_file($usersImage, $_SERVER['DOCUMENT_ROOT']."/template/images/users/".$image['userphoto']['name']);
             
        
        }
        $db = Db::getConnection();
        
        $sql = "UPDATE user SET image = '$imageName' WHERE id = $this->user_id ";
        
        $db->query($sql);
        
        return true;
        
     }


}

Все эти операции(добавление , обновление и показ) контролируются в контроллере

class UserController extends BaseController {
    
   
    public function actionProfile($id) {
        
        $user = new User(new Image($id));
       
        if(isset($_POST['submit_photo'])) {
       
        $user->image->updateImage($_FILES);    
        
        }
       
        return $this->render('user/profile.php',[
            
        'user' => $user->getUser($id),
        'image'=> $user->image->getImage()
      ]);
    }

Правильно ли я все делаю ?

Answer 1

[Demian Smith]

Во-первых, вы молодец, что движетесь в этом направлении. Продолжайте в том же духе.

По коду есть следующие замечания/предложения по улучшению:
- Делегирование подразумевает передачу некоторого функционала другому объекту. По сути, когда вы создали объект класса и выполнили метод этого объекта - вы уже делегировали некую функциональность. Например в контроллере задачу по выборке юзера вы делегировали объекту класса User. В случае же с классами User и Image, происходит внедрение класса Image в User (привет Dependency Injection), но дальше Image никак не используется классом User. Т.е. происходит Dependency Injection, но нет делегирования. На том этапе, который я сейчас вижу, я бы не стал внедрять Image в User. Сейчас это выглядет как задел на будущее, но любой опытный программист подтвердит, что композиция, созданная на будущее - ненужная композиция.
- Глядя на структуру классов, мне не совсем понятно чем они занимаются. Вроде как класс User и Image должны хранить информацию о пользователе и картинке (об этом нам говорит $id в конструкторе). Но в тех же классах есть логика по выборке данных из базы. По-хорошему выборкой должны заниматься другие классы. Лучше разделить задачи выборки и хранения на 2 разных класса. Я очень рекомендую вам ознакомиться с паттерном Data Mapper (наример тут designpatternsphp.readthedocs.io/en/latest/Structu... или тут codeinthehole.com/projects/domain-model-mapper-a-p... На мой взгляд, Data Mapper - это то что вам сейчас нужно реализовать чтоб получить чёткую структуру кода и разделение ответсвенности за выбор/хранени.

По поводу делегирования. Просто старайтесь чтоб ваши классы были как можно меньше. Критерий 1-3 метода на один класс вполне может подойти (помните, что это не золотое правило). Когда у вас появится желание добавить еще один метод в класс A - спросите себя, а нужен ли там вообще этот метод? Может лучше переложить всю ответственность на новый класс B, в который вы будете передавать объект A (или какое-то из его полей), а сам класс B уже проведёт необходимые преобразования/вычисления? Это и будет делегирование в том виде, в котором его ждёт от нас банда четырёх (если вы еще не читали их книгу, то это must read для любого программиста).

Фух, длинный ответ получился. Надеюсь, что не зря.

Comments

[Kolya Vantukh]

не зря . большое спасибо за столь развернутый ответ . буду учится

[Demian Smith]

Kolya Vantukh: я там подправил некоторые неточности в тексте.

[Kolya Vantukh]

Demian Smith: вижу , спасибо . слушайте , а можно с вами как-то связаться ? если вам не трудно ответить на некоторые вопросы по архитектуре . буду очень благодарен

Answer 2

[dmitriy]

так то User у вас не модель, а репозиторий:

namespace Repository;

class User {

    protected $db;

    public function __construct(Adapter $db) {
        $this->db = $db;
    }

    public function getUserById($id) {

        $sql = "SELECT * FROM user  WHERE id = :id";
        $stmt = $this->db->prepare($sql);
        $stmt->bindParam(':id', $id, PDO::PARAM_INT);

        $stmt->execute();

        return $stmt->fetch();
    }

    public function getImageById($id) {


        $sql = "SELECT  IF(image IS NULL or image = '','no_image.png',image) as image  FROM `user` WHERE id = :id";


        $result = $this->db->prepare($sql);
        $result->bindParam(':id', $id, PDO::PARAM_INT);
        $result->execute();

        return $result->fetchColumn();
    }

    public function updateImageById($imageName, $id) {

        $sql = "UPDATE user SET image = '$imageName' WHERE id = $id";

        return $this->db->query($sql);
    }

}

для работы с изображениями я предпочел бы сервис:

namespace Service;

class Image {

    protected $userRepo;

    function __construct(Repository\User $userRepo) {

        $this->userRepo = $userRepo;
    }

    public function UploadImage(array $image) {

        $usersImage = $image['userfile']['tmp_name'];

        $imageName = $image['userfile']['name'];

        if (is_uploaded_file($image['userfile']['tmp_name'])) {

            move_uploaded_file($usersImage, $_SERVER['DOCUMENT_ROOT'] . "/template/images/users/" . $image['userfile']['name']);
        }

        return $imageName;
    }

    public function updateImage(array $image, $id) {
        if($imageName = $this->uploadImage($image, $id)) {
            $this->userRepo->updateImageById($imageName, $id);
            return true;
        }
    }

}

а теперь самое интересное контроллер:

class UserController extends BaseController {

    protected $imageService;
    protected $userRepo;

    function __construct(Service\Image $imageService, Repository\User $userRepo) {
        $this->imageService = $imageService;
        $this->userRepo = $userRepo;
    }

    public function actionProfile($id) {

        if (isset($_POST['submit_photo'])) {

            $this->imageService->updateImage($_FILES, $id);
        }

        return $this->render('user/profile.php', [
                    'user' => $this->userRepo->getUserById($id),
                    'image' => $this->userRepo->getImageById($id)
        ]);
    }
}

Comments

[Kolya Vantukh]

сложно . но спасибо за ответ

[dmitriy]

на самом деле все просто, разбив код на слои контроллер-сервис-репозиторий
репозитрий - работа с бд
сервис - бизнес логика
контроллер - работа с клиентом (http)

Answer 3

[Эдуард]

Думаю, стоит отделить независимые элементы логики.
Есть класс для работы с изображениями?
Пусть с ними и работает, он не должен знать что есть какой-то user_id

Есть пользователь и у него есть аватарка? Ок, класс пользователя может выдернуть нужный img_id и передать его в Image