3
\$\begingroup\$

I am attempting to do object oriented programming in right way, and appreciate any review to see if my approach is correct.

class_blogpost.php as below:

<?php
class BlogPosts {

function __construct() {
    # code...
}

public function ListAllBlogPosts() {
    $DB = DatabaseFactory::getFactory()->Connect();
    $query = $DB->prepare("SELECT * FROM posts ORDER BY date DESC");
    $query->execute();
    $list_all = $query->fetchAll(PDO::FETCH_ASSOC);
    return $list_all;
}

public function ShowABlogPost($post_id) {

    if (isset((int)$_GET["id"];)) {
        $post_id = (int)$_GET["id"];
        $DB = DatabaseFactory::getFactory()->Connect();
        $query = $DB->prepare("SELECT * FROM posts WHERE id = :post_id");
        $query->bindParam(':post_id', $post_id, PDO::PARAM_INT);
        $query->execute();
        $show_post = $query->fetchAll(PDO::FETCH_ASSOC);
        return $show_post[0];
    } else {
        echo "Post not found.";
    }

}

}

$blogposts = new BlogPosts;

Below is code for home.php which uses ListAllBlogPosts() function:

<?php
include_once 'connect.php';
$title = "Home";
include_once 'header.php';
$list_all = $blogposts->ListAllBlogPosts();
?>
      <!-- col starts -->
      <div>
<?php
foreach ($list_all as $r) {
  $id = $r["id"];
  $title = $r["title"];
  $post = $r["post"];
  $date = $r["date"];
?>

        <h1><?php echo $title; ?></h1>
        <p><?php echo $post; ?></p>
        <a href="post.php?id=<?php echo $id; ?>">more</a>

<?php
} #ends foreach loop
?>
      </div>

And finally post.php below, user reaches as post.php?id=1 like that:

<?php
include_once 'connect.php';
$id = $blogposts->ShowABlogPost($_GET["id"])["id"];
$title = $blogposts->ShowABlogPost($id)["title"];
$date = $blogposts->ShowABlogPost($id)["date"];
$post = $blogposts->ShowABlogPost($id)["post"];
include_once 'header.php';
?>
      <div>
        <h1><?php echo $title; ?></h1>
        <p><?php echo $post; ?></p>
      </div>
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Given your code is not OOP, would you still do things like $blogposts->ShowABlogPost($id)["title"];,$date = $blogposts->ShowABlogPost($id)["date"];? Or you would just fetch a single row and use its contents? What makes you think that fetching the same record from database four times in a row is a good OOP? \$\endgroup\$ Jun 13 '19 at 11:04
  • \$\begingroup\$ Thank you @YourCommonSense, I haven't realized I was fetching 4 times, thanks for pointing out this murder. :) Surely, I would just fetch a single row and use its contents. \$\endgroup\$
    – LetsSeo
    Jun 13 '19 at 11:13
  • \$\begingroup\$ Why would it be important to cast (int)$_GET["id"] before checking if it isset()? \$\endgroup\$ Jun 13 '19 at 11:20
4
\$\begingroup\$

That's a good start but the structure is overall inefficient, that leads to code duplication.

the main problem is the violation of the Single responsibility principle. The ShowABlogPost does four things at a time:

  • connects to the database
  • checks the HTTP data
  • fetches a certain record from the database
  • displays some error

it should be doing only one thing, what it's intended for - fetching a record from a database.

Another issue is multiple calls to ShowABlogPost(). It would be, like, if you have to pay a cab driver 44.50 and you got this exact amount in your pocket, but would fetch the notes and coins one by one instead of taking them all at once.

You already have an array that contains all the data from the first call. So just use this array.

Other issues are:

  • $DB = DatabaseFactory::getFactory()->Connect(); is duplicated in every method. Why not to make a database connection a class variable? So it will be available in all class methods by default.
  • PDO::FETCH_ASSOC could be made the default fetch option so you won't have to call it every time explicitly
  • when no variables are going to be used in the query, there is no use for the prepared statements.
  • when you need to fetch only one row, then you have to use fetch() method, not fetchAll()

So let's rewrite your class

<?php
class BlogPosts {

    protected $db;

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

    public function ListAllBlogPosts() {
        $sql = "SELECT * FROM posts ORDER BY date DESC";
        $return $this->db->query($sql)->fetchAll();
    }

    public function ShowABlogPost($post_id) {
        $query = $this->db->prepare("SELECT * FROM posts WHERE id = :post_id");
        $query->bindParam(':post_id', $post_id, PDO::PARAM_INT);
        $query->execute();
        return $query->fetch();
    }
}

And now we can rewrite post.php

<?php
include_once 'connect.php';
include_once 'class_blogpost.php';

$db = DatabaseFactory::getFactory()->Connect();
$blogposts = new BlogPosts($db);

if (!isset($_GET["id"]) {
    die( "Empty request");
} else {
    $post = $blogposts->ShowABlogPost($_GET["id"]);
}
?>

<?php if ($post) : ?>
      <div>
        <h1><?= $post['title']; ?></h1>
        <p><?= $post['post']; ?></p>
      </div>
<?php else : ?>
    Post not found.
<?php endif ?>
\$\endgroup\$

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.