2
\$\begingroup\$

For my current project, I have elected to represent database rows as objects.

So an individual comment can be retrieved as a class like so:

$comment = new Comment(1);

Here is the Comment class code:

<?php
class Comment
{

    private $userid, $postid, $content, $date, $name, $ip, $legacy;

    public function __construct($comment_id)
    {
            $sql = new mysqli(REGISTRY_DBVALUES_SERVER, REGISTRY_DBVALUES_USERNAME, REGISTRY_DBVALUES_PASSWORD, REGISTRY_DBVALUES_DATABASE);        

            $stmt = $sql->prepare("SELECT id, userid, postid, content, date, name, ip, legacy
                                   FROM ".SITE_TABLE_COMMENTS."
                                   WHERE id = ?");

            $stmt->bind_param("i", $comment_id);
            $stmt->bind_result($id, $userid, $postid, $content, $date, $name, $ip, $legacy);

            $stmt->execute();

            while($row = $stmt->fetch())
            {
                    $this->userid = $userid;
                    $this->postid = $postid;
                    $this->content = $content;
                    $this->date = $date;
                    $this->name = $name;
                    $this->ip = $ip;
                    $this->legacy = $legacy;
            }

            $stmt->free_result();   
            $sql->close();
    }


    public function getUserID()
    {
            return $this->userid;
    }

    public function getPostID()
    {
            return $this->postid;
    }

    public function getContent()
    {
            return $this->content;
    }

    public function getDate()
    {
            return $this->date;
    }

    public function getName()
    {
            return $this->name;
    }

    public function getIP()
    {
            return $this->ip;
    }

    public function getLegacy()
    {
            return $this->legacy;
    }
}
?>

This allows you to retrieve the different attribute values for an individual comment using methods.

I am open to criticism on all aspects of the design, but was particularly curious if this is an accepted way of handling retrieval of individual data records?

\$\endgroup\$
  • \$\begingroup\$ is the code complete? There aren't any defined properties. \$\endgroup\$ – 32bitfloat Jul 14 '13 at 19:41
  • \$\begingroup\$ Sorry, don't know where they disappeared to -- added. \$\endgroup\$ – Racktash Jul 15 '13 at 17:47
2
\$\begingroup\$

This is not OOP, your comment is hard coupled with mysqli. What if you would write the exact same program but now with PDO? you will have to change the Comment class.

Then performance wise: you are now opening a mysqli connection for every comment.

You seem to think that using the word 'class' and method names as 'getMyVar()' equals OOP. It doesnt.

Programming is always about problem solving, and a good program is about solving a problem in such a way you can easily reuse that solution for other sort of the same problems.

OOP is very dangerous, because it alows us programmers to write even crapper code as before. That is why some really smart guys came up with loads of rules to live by in OOP land, called SOLID: http://en.wikipedia.org/wiki/SOLID_(object-oriented_design)

Now, lets place your code next to the SOLID rules.

1 Single responsibility principle

The Comment class fails on this account. It does multiple things as far as I can see. It works as a storage object for a Comment with methods to retrieve the parameters of the comment AND it also retrieves the comment from the Database. So we allready have a little problem here.

2 Open/closed principle

Again, not so good. An example: say I have a special sort of comment that has an extra field called 'permissions':

class PermissionComment extends Comment {}

Uh oh, In order to save the permission field into the object, I will have to rewrite the entire __construct while I only added 1 field. Not so good.

Also, all your variables are public variables. I don't need to use the get() methods to access a variable. I can access it directly. We thus violate the above rule in multiple ways. We cannot easily extend the comments functionality and we dont encapsulate it correctly

3 Liskov substitution principle

As I allready wrote somewhere at the top, say we want to change mysqli with PDO. Or we have written an extension of the mysqli object that does some caching of queries, I would now have to rewrite my Comment class because mysqli is hard coded into the object. Not so good again.

4 Interface segregation principle

This one, I will let go since it is not really in order here

5 Dependency inversion principle

Again, could be better. The Comment class i tightly coupled with your database connect. Your comment class 'knows' about the database-connection. It knows how to connect to a database. It knows how comments are stored while in fact it's just a container of comment data.

What now?

Back to the drawing board. You write you want to write a class that represents a Comment. Ok good. As example I omitted a lot of parameters in the code below, in fact, mostly because I'm lazy :p

So lets write a class that represents a basic Comment:

class Comment {

    private $id;

    private $content;

    private $title;

    public function __construct($data) {
        $this->id = $data['id'];
        $this->content = $data['content'];
        $this->title = $data['title'];
    }

    public function getId() {
        return $this->id;
    }

    public function getContent() {
        return $this->content;
    }

    public function getTitle() {
        return $this->title;
    }
}

Good, a Simple class that represents a Comment and only a Comment. But, one problem. What happend if we change the way the data is passed in. Or the key's change. and how can we be sure that that Comment class will only extract the data it needs. How does another programmer know what key's that $data array needs to have without having to read your entire code?

Let's change the constructor into:

public function __construct($content, $title) {
    $this->content = $content;
    $this->userId = $userId;
}

Ooeeh, cheecky. But huh? where did $id go? Well, let's just say I want to create a Comment and later add it to the database, or I want to create a Comment object for testing purposes, or just a default comment to pass to the the frontend, my Comment doesn't have an id. I can now easily create a Content without needing a database id.

And for those Comments who do have an ID, we create an additional class:

class DbComment extends Comment {

    private $id;

    public function __construct($content, $userId, $id) {
        $this->id = $id;
        parent::__construct($content, $userId);
    }

    public function getId() {
        return $this->id;
    }
}

See how little code I had to write to simply add a Field to the Comment object?

But now your thinking: What a load of bull. This code can't even fetch a Comment from the database. True, but a 'Comment' object is a Comment and not a CommentFetcher.

We could now write a CommentFetcher class that fetches and saves Comments and returns a DbComment. Example:

class CommentFetcher {

    private $mysql;

    public function __construct(mysqli $mysql) {
        $this->mysql = $mysql;
    }

    public function fetch($id) {
        $stmt = $this->mysql->prepare("SELECT id, title, content FROM comment_comments WHERE id = ?");
        $stmt->bind_param("i", $id);
        $stmt->bind_result($id, $title, $content);
        $stmt->execute();
        return new DbComment($content, $title, $id);
    }
}

Note how i pass in a mysqli object in the constructor. All the class says is I need a mysqli object. If I now write my ThisClassCachesMysqliQueries extends Mysqli class I can simply pass it in, the CommentFetcher will have no idea I passed in a different class because it's still a mysqli object. But I will have added super duper caching into my program without having to change the CommentFetcher, cool huh?

So now we have a generic Comment class, we have a special case Comment, the DbComment. This is also a Comment but one that exists in a database somewhere and thus has an Id and we have a CommentFetcher that fetches comments from the Database.

Now a note: The code that I have written here is still crap. Don't use it ;) A Class that can only fetch comments isn't really usefull. But you get the point, a CommentController that can update, fetch and insert Comments is something really nice.

Also I would advice you to rething how you are saving comments. Know, a comment has to know about users, ip, legacy,... While in fact all a Comment needs is a Title, data and content. A different table and controller would then look up hte user that postes the comment in the user_posted_comment table etc etc etc.

Once in a while, I take a step back an think, what am I writing? because at the end of the day, a Comment is a Comment and not some exotic objet that knows about users, ip-adresses and legacy.

But hey, atleast you are using bound params and prepared statements! Big up yoself. I hope this answer helps you. else simply flame me in the commentbox below

\$\endgroup\$
  • \$\begingroup\$ Thank you for the thorough review, it is much appreciated. One question -- and it may be very stupid, but this is the place to ask such questions: is there at this point an advantage to still using a Comment object over, say, an associative array being simple returned by our Fetcher? \$\endgroup\$ – Racktash Jul 15 '13 at 17:39
  • \$\begingroup\$ And furthermore, wouldn't many arguments for constructors make code less clean? \$\endgroup\$ – Racktash Jul 15 '13 at 17:54
  • \$\begingroup\$ @Racktash Many arguments will actually in the long run make the code better to read. In a half year you will look back at that comment class and think, huh? only one argument to pass in? but now you see, ah ok, i need these arguments. And to answer your other question. If a Comment doesn't have any extra functionality you could opt for indeed returning a simple associative array. If it doens't help you or solve a problem you are having, don't do it. \$\endgroup\$ – Pinoniq Jul 15 '13 at 22:00
  • \$\begingroup\$ @Racktash your welcome! We've all been there and done that ;) \$\endgroup\$ – Pinoniq Jul 16 '13 at 8:11

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.