2
\$\begingroup\$

I'm new to PHP OOP and I'm making some tests. I have 2 classes: database and posts and the two interact with each other.

Class Database:

class Database {

    // defining some variables
    private $host;
    private $user;
    private $pass;
    private $bd;

    protected $dbh;


    /*

    method construct
    This method will be used everytime our class is called

    */

    public function __construct() {
        $this->host = HOST;
        $this->user = USER;
        $this->pass = PASS;
        $this->bd = DB;

        // database connection
        $dsn = 'mysql:host=' . HOST . ';dbname=' . DB;
        // Set options
        $options = array(
            PDO::ATTR_PERSISTENT        => true,
            PDO::ATTR_ERRMODE           => PDO::ERRMODE_EXCEPTION,
            PDO::ATTR_EMULATE_PREPARES  => false,
            PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
            PDO::MYSQL_ATTR_INIT_COMMAND => "SET NAMES utf8"
        );
        //Create a new PDO instance
        try {
            $this->dbh = new PDO($dsn, USER, PASS, $options);

            return $this->dbh;
        }
        // Catch any errors
        catch(PDOException $e) {
            die($e->getMessage());
        }
    }
}

Class Posts

class Posts extends Database {

    /* FRONTEND METHODS */
    // method to select all posts
    // uses a query predefined and an array of parameters
    // returns the array of results
    public function selectPosts($query, $params = array()) {
        // prepare the query to bind params
        $stmt = $this->dbh->prepare($query);

        // binds the params
        foreach($params as $key => $val) {
            if(is_string($val)) {
                $stmt->bindParam($key, $val, PDO::PARAM_STR);
            }
            elseif(is_numeric($val)) {
                $stmt->bindParam($key, $val, PDO::PARAM_INT);
            }
        }
        // execute query after parameters are binded
        $stmt->execute();

        // returns the result
        return $stmt->fetchAll();
    }


    // method to select a single posts
    // uses a query predefined and an array of parameters
    // returns a single row
    public function selectSingle($query, $params = array()) {
        // prepare the query to bind params
        $stmt = $this->dbh->prepare($query);

        // binds the params
        foreach($params as $key => $val) {
            if(is_string($val)) {
                $stmt->bindParam($key, $val, PDO::PARAM_STR);
            }
            elseif(is_numeric($val)) {
                $stmt->bindParam($key, $val, PDO::PARAM_INT);
            }
        }

        // execute query after parameters are binded
        $stmt->execute();

        // returns the result - a single row
        return $stmt->fetch();
    }



    /* BACKEND METHODS */

    // insert posts into database
    // passes just the parameters
    public function insertPost($params = array()) {
        $uery = "INSERT INTO posts (...) VALUES (...)";

        // prepare the query to bind params
        $stmt = $this->dbh->prepare($query);

        // binds the params
        foreach($params as $key => $val) {
            if(is_string($val)) {
                $stmt->bindParam($key, $val, PDO::PARAM_STR);
            }
            elseif(is_numeric($val)) {
                $stmt->bindParam($key, $val, PDO::PARAM_INT);
            }
        }

        // execute query after parameters are binded
        return $stmt->exeute() ? true : false;
    }

    // delete posts from the database
    // needs to get the id of the post to delete
    public function deletePost($id) {
        $query = "DELETE FROM posts WHERE id = :id";

        // prepare the query to bind params
        $stmt = $this->dbh->prepare($query);

        // bind the params
        $stmt->bindParam(':id', $id, PDO::PARAM_INT);

        // executes query after params are binded
        return $stmt->exeute() ? true : false;

    }

    // updates a post
    // passes an array of parameters to bind
    public function updatePost($params = array()) {
        $query = "UPDATE posts SET ... WHERE id = :id";

        // prepare the query to bind params
        $stmt = $this->dbh->prepare($query);

        // binds the params
        foreach($params as $key => $val) {
            if(is_string($val)) {
                $stmt->bindParam($key, $val, PDO::PARAM_STR);
            }
            elseif(is_numeric($val)) {
                $stmt->bindParam($key, $val, PDO::PARAM_INT);
            }
        }

        // execute query after parameters are binded
        return $stmt->exeute() ? true : false;
    }
}

Example usage:

$posts = new Posts();

$query = "SELECT * FROM posts WHERE posts.id > :n";
$params = [':n' => 6];
foreach($posts->selectPosts($query, $params) as $post) {
    echo $post['id']. ', ';
}

Is this okay? how can I improve this? Is there a way to make the class Posts simpler (the 2 methods selectPosts() and selectSingle()? I had many more methods but I was able to simplify with these two, but now I have to pass the query and parameters.

\$\endgroup\$
2
  • \$\begingroup\$ Do one thing and do it well - kind of a OOP rule sometimes. Your product class does far to much, it should not be dealing with SQL. Worth reading up on the OOP design patterns, dependency injection will be useful. \$\endgroup\$ Commented Jan 26, 2019 at 19:33
  • \$\begingroup\$ return in constructor? \$\endgroup\$
    – Progrock
    Commented Jan 26, 2019 at 23:03

2 Answers 2

2
\$\begingroup\$

To tell you truth, this is not OOP at all. For the usage example like this, you don't actually need Posts class. You can do it with either raw PDO or a good Database class, to which selectPosts() and selectSingle() actually belong.

With OOP, the usage example would be like

$posts = new Posts();
foreach($posts->selectByIdBiggerThan(6) as $post) {
    echo $post['id']. ', ';
}

so all internal workings being encapsulated in a class method. Which would look like

public function selectByIdBiggerThan($id) {
    $query = "SELECT * FROM posts WHERE id > ?";
    return $this->db->selectAll($query, [$id]));
}

and selectAll(), like it was said above, should belong to a database class, as there is absolutely nothing specific to Posts in this function. It is rather a generic function to run any query.

You may also notice that the code in these functions is almost identical, which is directly opposite to the purpose of OOP. So you should make a generic function to run a query that can be then used to fetch the actual result

public function run($sql, $args = [])
{
    if (!$args)
    {
         return $this->query($sql);
    }
    $stmt = $this->pdo->prepare($sql);
    $stmt->execute($args);
    return $stmt;
}
public function selectAll($sql, $params = []) {
    return $this->run($sql, $params)->fetchAll();
}
public function selectSingle($sql, $params = []) {
    return $this->run($sql, $params)->fetch();
}

There are other issues with your database class, that are pretty common, so I recommend you to read my article, Your first database wrapper's childhood diseases, you can learn a lot from it.

And the most global problem among them is that your Post class extends Database class. You can read in the article why it is so, and then make your Posts class accept a Database class instance as a constructor parameter, that will assign it to a class variable.

\$\endgroup\$
1
  • \$\begingroup\$ @YourCommonSence tks for your answer. My first approach was nothing like this. In posts class i had more methods, each one for a specific task. AS i said, im new to oop and im just trying to understand the concepts and all. So, my question, are you available for some ideas exchange through chat? tks \$\endgroup\$
    – eskimopest
    Commented Jan 27, 2019 at 11:40
0
\$\begingroup\$

In the insertPost() method, there is this line:

$uery = "INSERT INTO posts (...) VALUES (...)";

I haven't seen a spread syntax (i.e. (...)) like that in MySQL/PHP SQL queries, nor do I see any code following that which replaces that spread syntax with a set of fields and values... Does that actually work or did you simplify this for the example? If it does work, I would like to see the documentation for this technique.

That line also declares the variable as $uery, whereas the following line utilizes $query, which would thus be an undefined variable:

$stmt = $this->dbh->prepare($query);

The method PDOStatement::execute() returns a bool, so this last line of deletePost() can be simplified from :

return $stmt->exeute() ? true : false;

to simply:

return $stmt->exeute();

I see the following block of lines occur at least three times, in various methods:

    // binds the params
    foreach($params as $key => $val) {
        if(is_string($val)) {
            $stmt->bindParam($key, $val, PDO::PARAM_STR);
        }
        elseif(is_numeric($val)) {
            $stmt->bindParam($key, $val, PDO::PARAM_INT);
        }
    }

That could easily be abstracted out into a separate method.

What happens if the parameter isn't a string and isn't numeric? should an error be thrown (actually it would likely might happen at the SQL level if that is the case).

\$\endgroup\$

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