Voting system for a website

Question

I am currently adding a voting system to my website. Below is my voting class. I feel that it could be simplified and that there is repeated code regarding the functions, although to the best of my knowledge this is the simplest I can make it. I would love to hear other implementation ideas and systems that have been used.

<?php

class Vote{

    var $db;

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

    function vote($user_id, $post_id, $type){
        // check if vote already exists against user and post to prevent duplicates
        if($this->userVoteExists($user_id, $post_id)){
            // retreive vote
            $vote = $this->getVote($user_id, $post_id);
            // check  if the requested vote is the same to prevent needlessly deleting and inserting
            if($vote['type'] != $type){
                if($vote){
                    // remove the vote if it exists
                    $this->removeVote($user_id, $post_id);
                }
                // add the vote
                $this->addVote($user_id, $post_id, $type);
            }
        }
    }

    function userVoteExists($user_id, $post_id){
        $query = $this->db->prepare("SELECT * FROM votes WHERE user_id = :user_id AND post_id = :post_id LIMIT 1");
        $query->execute(array(':user_id' => $user_id, ':post_id' => $post_id));
        return $query->rowCount();
    }

    function getVote($user_id, $post_id){
        $query = $this->db->prepare("SELECT * FROM votes WHERE user_id = :user_id AND post_id = :post_id LIMIT 1");
        $query->execute(array(':user_id' => $user_id, ':post_id' => $post_id));
        return $query->fetch();
    }

    function removeVote($user_id, $post_id){
        $query = $this->db->prepare("DELETE FROM votes WHERE user_id = :user_id AND post_id = :post_id");
        $query->execute(array(':user_id' => $user_id, ':post_id' => $post_id));
    }

    function addVote($user_id, $post_id, $type){
        $timestamp = time();
        $query = $this->db->prepare("
        INSERT INTO votes (
            post_id,
            user_id,
            type,
            timestamp
        ) VALUES (
            :post_id,
            :user_id,
            :type,
            :timestamp
        )");
        $query->execute(array(
            ':post_id' => $post_id,
            ':user_id' => $user_id,
            ':type' => $type,
            ':timestamp' => $timestamp
        ));
    }


}
?>

Answer 1

A Vote should not have access to the database. It should be a dumb 'entity' with setters, getters and a constructor to make the required object a valid one.

You should take a look at the Repository and Data Mapper patterns. The point is, the object you want to save should not know how it is saved - meaning that you can change where you want to save it's data in the future without touching the underlying entity.

For an example implementation of this, check out Doctrine ORM. The Repository is the the object that uses a data mapper to save data to the relevant data source. Change the data source, the repository object-api (how you save / retrieve data) stays the same, but how the data is saved changes. Pretty basic concept but really powerful, and not just limited to ORMs or PHP, either. Imagine entities in JavaScript with a data mapper for local/session storage, for example. The data mapper would have get, update, delete (etc) methods, and the repository would call the underlying data mapper (relying on an interface) to actually perform the saving / retrieving of data, which you can swap out at any time.

Also, our vote needs to have it's own id (unique, primary key) completely separate from every other attribute so it can be uniquely identified. Aside from that, all lowercase, underscores for spaces and prepared statements is a good thing.

I'm not going to write your code for you, but effectively, you need to take a look at doing the following:

• Abstract out the thing that saves, updates and removes data from your persistence. This is your data mapper, it is stand-alone, and just takes data and maps it to your given persistence. This is the object that would have your instance of \PDO dependency injected (yes, typehint for it).
• Have a class that takes an instance of your data mapper that is responsible for saving your vote (a VoteRepository). You don't need an ORM, but you can simulate some of it's useful abstraction to help with SoC. This class contains your object-specific stuff which it forwards to it's data mapper, with your methods like userHasVote(User $user, $voteId).
• You could probably get away with putting addVote(Vote $vote) and removeVote(Vote $vote) in your repository. This will require you to construct a new Vote object before you save it to the database (a Factory will help here, and your repository will give you a Vote object when you request one from the database. Remember, you're trying to use OOP here: everything is an object with a single responsibility.
• The logic to remove / add a vote depending on something I would personally put in a Voting component (name it something better), with this single responsibility within it. Also, read the difference between a component and a service. Responsibility wise, you need to work upwards from the datasource, building a nice object api for each object without having it concern others (so they can work standalone)
• Learn how to document your code. Phpdoc is a standard everyone and their dog's framework are employing to help other developers (and automated documentation tools) understand what code does
• You're not writing php 4 any more. var has no place in your code. Use the correct access modifier, as in protected $db. The same goes for your functions (methods), make the ones you want others to consume (the public api) public function name instead.

So, all in all, a lot of refactoring to be done here. Well done for using \PDO and prepared statements though.

Answer 2

Firstly, lets talk about the coding standard issues which can be found here.

• Have a space between a keyword and a curly brace. It improves readability without making it a clutter. However, you should not open the braces for Classes and Methods in the same line as the keywords. This is how you do it:

  class Foo
  {
      pubic function bar()
      {
  
      }
  }
  

• You should also name your variables in camelCase.

Now, lets talk about basic flaws.

• You haven't used encapsulation which is also known as information hiding.

Other than these, I don't see any other flaws.