0
\$\begingroup\$

I'm a beginner in OOP and PHP frameworks. I used slim skeleton to create a project and designed the project in this way.

Project structure is given by the slim skeleton. Object Mappers are added by my self. I want to know whether this is a good practice. What I'm doing wrong here. Is there any better way to do this?

src
   Controllers
      PostController.php
      CommentController.php
   Models
      Post.php
      Comment.php
   ObjectMappers
      PostMapper.php
      CommentMapper.php
   Dependencies.php
   Middleware.php
   Routes.php
   Settings.php

Model files are used to include queries for relevant class. Mappers are the real classes that used to map query results to necessary class. Controllers are for necessary operations before sending it to the user.

Post.php

namespace App\Models;

class Post {

   public function getPostById($id){

      $sql = "SELECT `post_id`, `post` FROM `posts` WHERE `post_id` = :id";

      try {
         $stmt = $this->db->prepare($sql);
         $stmt->execute(['id' => $id]);
         $post = $stmt->fetchObject('\App\ObjectMappers\PostMapper');
         if($post){
            return $post;
         }
         return false;
      } catch (\PDOException $e){
         return false;
      }
   }

   // Other queries
}

PostMapper.php

namespace App\ObjectMappers;

class PostObject {

    public $post_id;
    public $post;

    public function __construct(){}

    public function getPost_id()
    {
        return $this->post_id;
    }

    public function setPost_id($post_id)
    {
        $this->post_id= $post_id;
        return $this;
    }

    public function getPost()
    {
        return $this->post;
    }

    public function setPost($post)
    {
        $this->post= $post;
        return $this;
    }
}

PostController.php

namespace App\Controllers;

class PostController {

   public function ($request, $response, $args){

      $postId = $request->getAttribute("post_id");

      if(!$postId){
         return $response->withJSON("post id not found");
      }
      $post = $this->Post->getPostById($postId);

      if(!$post ){
         return $response->withJSON("post not found");
      }

      return $response->withJSON($post);
   }
}

I'll be glad if you can help me. I'm trying to avoid using existing ORMs here.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review. From your post, it's not clear what you wwant to get reviewed. Just the general structure? Or the code? Note that example code is off-topic on Code Review. I mention this since your code snippets are prefaced with "For example". If that's actual code that you really want to get reviewed, I suggest you to remove that phrase. Also keep in mind that questions should provide some context, and at the moment, your structure looks very generic. \$\endgroup\$ – Zeta Dec 28 '18 at 10:14
  • \$\begingroup\$ I edited the question. \$\endgroup\$ – tha07 Dec 28 '18 at 11:18
0
\$\begingroup\$

This is good in general. The main point is that you confused a model and a mapper. Your mapper must be a model (or, to avoid a confusion, better to call it an entity) whereas your model is actually a mapper.

People often mistake the term "model", so I had to write an article to explain it.

So

src
   Controllers
      PostController.php
      CommentController.php
   Entities
      Post.php
      Comment.php
   Mappers
      PostMapper.php
      CommentMapper.php
   Dependencies.php
   Middleware.php
   Routes.php
   Settings.php

where Post.php would be

namespace App\Entities;

class Post {

    protected $postId;
    protected $post;

    public function __construct(){}

    public function getPostId()
    {
        return $this->postId;
    }

    public function setPostId($postId)
    {
        $this->postId= $postId;
        return $this;
    }

    public function getPost()
    {
        return $this->post;
    }

    public function setPost($post)
    {
        $this->post= $post;
        return $this;
    }
}

and PostMapper would be

namespace App\Mappers;

class PostMapper {

   public function getPostById($id){

      $sql = "SELECT `post_id`, `post` FROM `posts` WHERE `post_id` = :id";
      $stmt = $this->db->prepare($sql);
      $stmt->execute(['id' => $id]);
      return $stmt->fetchObject('\App\Entity\Post');
   }
}

Please note that a lot of code has been removed from PostMapper class. First, a harmful try .. catch has been removed. A try .. catch that silently returns false is as bad as any other error suppression operator, and shouldn't be used such casually. As a programmer, you crave to know what the error was, so you'll be able to fix it. So always let errors go, unless you have a certain handling scenario. See more in my article, PHP error reporting basics.

Also, rather useless condition is removed too, as fetchObject() already returns false if a row not found, so we can return its result right away.

Also, consider making your PHP code style to conform with the standard

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for the answer. I would like to know how the controller is going to be relevant to your edit. \$\endgroup\$ – tha07 Dec 28 '18 at 11:14
  • \$\begingroup\$ At the moment the controller is a regular controller, does exactly what a controller should. So I have nothing to review in it. \$\endgroup\$ – Your Common Sense Dec 28 '18 at 11:20
  • \$\begingroup\$ in the post mapper, the return should be like this return $stmt->fetchObject('\App\Entities\Post'); am I correct? \$\endgroup\$ – tha07 Dec 28 '18 at 11:27
  • \$\begingroup\$ Can you tell me some resources on learning PHP OOP and MVC further. I'm still new. Your website looks cool. bookmarked it :) \$\endgroup\$ – tha07 Dec 28 '18 at 11:34
  • \$\begingroup\$ I would suggest phptherightway.com as a theory and then symfonycasts.com/screencast/symfony for the best practice you can get \$\endgroup\$ – Your Common Sense Dec 28 '18 at 12:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.