Am I using delegation correctly?

Question

PHP

Need advice on whether I'm using delegation correctly. There is a registration on the site where the user's photo is uploaded, it is possible to change these images in the personal account, and you also need to display these images in the user's view.
In general, there is a User model

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

There is an Image model for working with images

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;
        
     }


}

All these operations (adding , updating and displaying) are controlled in the controller

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

Am I doing everything right?

Answer 1

First of all, you are great for moving in this direction. Continue in the same spirit.
The code has the following comments/suggestions for improvement:
- Delegation implies the transfer of some functionality to another object. In fact, when you created an object of a class and executed a method on that object, you have already delegated some functionality. For example, in the controller, you delegated the task of selecting a user to an object of the User class. In the case of the User and Image classes, the Image class is injected into the User (hello Dependency Injection), but then the Image is not used by the User class in any way. Those. Dependency Injection happens, but no delegation. At the stage I now see, I wouldn't inject an Image into a User. Now it looks like a future-proof, but any experienced programmer will confirm that a composition created for the future is an unnecessary composition.
- Looking at the structure of the classes, I don't quite understand what they do. It seems like the User and Image classes should store information about the user and the picture ($id in the constructor tells us about this). But in the same classes there is logic for retrieving data from the database. In an amicable way other classes should be engaged in sampling. It's better to separate fetch and hold tasks into 2 different classes. I highly recommend that you familiarize yourself with the Data Mapper pattern (for example, here designpatternsphp.readthedocs.io/en/latest/Structu... or here codeinthehole.com/projects/domain-model-mapper-ap... In my opinion, Data Mapper - this is what you now need to implement in order to get a clear code structure and separation of responsibility for selection / storage.
About delegation. Just try to keep your classes as small as possible. The criterion of 1-3 methods per class may well be fine (remember that this is not a golden rule). When you have a desire to add another method to class A, ask yourself if this method is even needed there? Maybe it's better to shift all responsibility to a new class B, to which you will pass the object A (or some of its fields), and the class B itself will already carry out the necessary transformations / calculations? This will be the delegation in the form in which the gang of four expects it from us (if you have not read their book yet, then this is a must read for any programmer).
Phew, that's a long answer. I hope it's not in vain.

Answer 2

so User is not a model, but a repository:

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

}

for working with images, I would prefer a service:

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;
        }
    }

}

and now the most interesting controller:

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)
        ]);
    }
}

Answer 3

I think it's worth separating the independent elements of logic.
Is there a class for working with images?
Let him work with them, he should not know that there is some kind of user_id
Is there a user and does he have an avatar? Ok, the user class can pull out the desired img_id and pass it to the Image