0
\$\begingroup\$

Here is my very first code which fetch data and add it to MySql. Please review my code and let me know if I'm on the right path. Is there anything I can improve?

postsData class = fetch and add data blog posts

categoriesData class = fetch and add data for categories of my blog. Ex: bmw, nissan, opel, ford, etc

connect.php

class dbh{

        private $host = "localhost";
        private $user = "root";
        private $pwd = "";
        private $dbname = "test_blog";

        public $conn;                               

        public function __construct()
        {
            $this->conn = $this->connect();
        }


        private function connect() {
            $dsn = 'mysql:host=' . $this->host . ';dbname=' . $this->dbname;
            $pdo = new PDO ($dsn, $this->user, $this->pwd);
            $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            $pdo->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_ASSOC);
            $pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
            return $pdo;

    }
}  

func.class.php

  class postsData extends dbh{

    public function getPosts() {

        $sql = "SELECT * FROM posts_tbl";
        $stmt = $this->conn->prepare($sql);
        $stmt->execute();
        $result = $stmt->fetchAll((PDO::FETCH_OBJ));

        return $result;

    }

    public function addPost($filter_author, $filter_title, $filter_txt) {

        $sql = "INSERT INTO posts_tbl (post_author, post_title, post_txt) VALUES (?, ?, ?)";
        $stmt = $this->conn->prepare($sql);
        $stmt->execute([$filter_author, $filter_title, $filter_txt]);

    }

}

class categoriesData extends dbh{


    public function getCategories () {

        $sql = "SELECT * FROM categories_tbl";
        $stmt = $this->conn->prepare($sql);
        $stmt->execute();
        $result = $stmt->fetchAll((PDO::FETCH_OBJ));

        return $result;

    }

    public function adCategory ($filter_name, $filter_slug) {

        $sql = "INSERT INTO categories_tbl (category_name, category_slug) VALUES (?, ?)";
        $stmt = $this->conn->prepare($sql);
        $stmt->execute([$filter_name, $filter_slug]);

    }

}

    //test
    $post = new PostsData();
    $posts = $post->getPosts();

    foreach ($posts as $post) { 
        echo $post->post_title . '<br>';
    }
\$\endgroup\$
6
  • 3
    \$\begingroup\$ Could you please explain a little more about what the code does? What kind of categories is it posting to the database? \$\endgroup\$
    – pacmaninbw
    Feb 15, 2020 at 15:22
  • \$\begingroup\$ Blog categories - category 1 : bmw, category 2: nissan etc \$\endgroup\$
    – Sebb
    Feb 15, 2020 at 15:27
  • \$\begingroup\$ Please add the category explanation to the question. Right now there are 2 votes to close your question due to lack of details. \$\endgroup\$
    – pacmaninbw
    Feb 15, 2020 at 15:29
  • \$\begingroup\$ I have recently posted a review on a very similar question. It could be of help to you as well. codereview.stackexchange.com/a/237237/212100 And actually this one too codereview.stackexchange.com/a/237288/212100 It's incredible how you both even extend a dbh class. Where are you getting this idea? Does it come from some tutorial? \$\endgroup\$
    – slepic
    Feb 17, 2020 at 9:00
  • 2
    \$\begingroup\$ Welcome to Code Review! Please edit your question so that the title describes the purpose of the code, rather than its mechanism. We really need to understand the motivational context to give good reviews. Thanks! \$\endgroup\$ Feb 17, 2020 at 9:31

1 Answer 1

1
\$\begingroup\$

Your code is mostly all right, especially given it's your first code attempt.

I would say there is only on critical flaw. A post or a category is not a database connection. Hence it should be never extended from it. It would be like if you extend a human from a car simply because a human is going to ride a car. A car is a service used by a human, that needs to be injected into a human object.

Besides, the way your code is written, it will create a new connection to the database in the every class instance. Which will result in the "Too many connections" error.

To sum it up:

  • a database connection has to be made only once. and then passed as a parameter to all your classes that need it.
  • your dbh class is rather useless. it does't add anything to original PDO. so you can get rid of it and simply create a plain PHP file with connection code and include it in your scripts
  • then you have to rewrite your data classes adding a constructor, providing either vanilla PDO or your own database connection class' instance as a parameter

something like this

class categoriesData {
    protected $conn;
    public function __construct($conn) {
        $this->conn = $conn;
    }
    ...

and then use it like this

require_once 'pdo.php';
$post = new PostsData($pdo);
$posts = $post->getPosts();

foreach ($posts as $post) { 
    echo $post->post_title . '<br>';
}
\$\endgroup\$

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