OOP PHP for handling upvoting

Question

kinda new to OOP, and I wanna know if i'm heading into the right direction. Also I should mention I'm not using AJAX

AnswerVoteManager.php

<?php
require '../config.php';
interface voting
{
public function voted();
public function redirect();
public function vote();
public function deleteVote();
public function addVote();
public function subtractVote();
public function updateVote();
public function insertVote();
public function getCurrentvote();
}
class AnswerVoteManager
{
private $dbh;
protected $answerID;
protected $title;
protected $voter;
protected $vote_type;

/**
 * @return mixed
 */
public function getAnswerID()
{
    return $this->answerID;
}

/**
 * @param mixed $answerID
 */
public function setAnswerID($answerID)
{
    $this->answerID = $answerID;
}

/**
 * @return mixed
 */
public function getTitle()
{
    return $this->title;
}

/**
 * @param mixed $title
 */
public function setTitle($title)
{
    $this->title = $title;
}

/**
 * @return mixed
 */
public function getVoter()
{
    return $this->voter;
}

/**
 * @param mixed $voter
 */
public function setVoter($voter)
{
    $this->voter = $voter;
}

/**
 * @return mixed
 */
public function getVoteType()
{
    return $this->vote_type;
}

/**
 * @param mixed $vote_type
 */
public function setVoteType($vote_type)
{
    $this->vote_type = $vote_type;
}
public function __construct($dbh)
{
    $this->dbh = $dbh;

}
public function getCurrentvote($answerid,$username)
{
    $sql = "SELECT vote_type from answer_votes WHERE answerid=:answerid and username=:username";
    $stmt = $this->dbh->prepare($sql);
    $stmt->bindValue(':answerid',$answerid);
    $stmt->bindValue(':username',$username);
    $stmt->execute();
    $type = (string) $stmt->fetchColumn();
    if(empty($type))
    {
    return null;
    } else {
    return (string) $type;
    }
}
private function voted()
{
    $sql = "SELECT vote_type from answer_votes where answerID=:answerid and username=:username";
    $stmt = $this->dbh->prepare($sql);
    $stmt->bindValue(':answerid',$this->answerID);
    $stmt->bindValue(':username',$this->voter);
    $stmt->execute();
    $exists = $stmt->rowCount();
    if($exists)
    {
    return true;
    } else {
    return false;
    }
}
private function redirect()
{
    header("Location:" . $_SERVER['REQUEST_URI']);
    exit();
}
private function updateVote()
{
    $sql = "UPDATE answer_votes set vote_type=:vote_type";
    $stmt = $this->dbh->prepare($sql);
    $stmt->bindValue(':vote_type',$this->getVoteType());
    $stmt->execute();
    $this->redirect();
}
private function addVote()
{
    $sql = "UPDATE question_answers set count = question_answers.count + 1 WHERE id=:id";
    $stmt = $this->dbh->prepare($sql);
    $stmt->bindValue(':id',$this->answerID);
    $stmt->execute();
}
private function subtractVote()
{
    $sql = "UPDATE question_answers set count = question_answers.count - 1 WHERE id=:id";
    $stmt = $this->dbh->prepare($sql);
    $stmt->bindValue(':id',$this->answerID);
    $stmt->execute();
}
private function deleteVote()
{
    $sql = "DELETE FROM answer_votes where answerID=:answerid and username=:username LIMIT 1";
    $stmt = $this->dbh->prepare($sql);
    $stmt->bindValue(':answerid',$this->getAnswerID());
    $stmt->bindValue(':username',$this->voter);
    $stmt->execute();
    $this->subtractVote();
    $this->redirect();
}
private function insertVote()
{
    $sql = "INSERT  into answer_votes (vote_type,answerID,username,questiontitle) values (:vote_type,:answerid,:username,:questiontitle)";
    $stmt = $this->dbh->prepare($sql);
    $stmt->bindValue(':vote_type',$this->getVoteType());
    $stmt->bindValue(':answerid',$this->getAnswerID(),PDO::PARAM_INT);
    $stmt->bindValue(':username',$this->getVoter());
    $stmt->bindValue(':questiontitle',$this->getTitle());
    $stmt->execute();
    $this->addVote();
    $this->redirect();
}
public function vote()
{
   if($this->vote_type == 'Upvote' && $this->voted()) {
       $this->deleteVote();

   } else {
       $this->insertVote();
   }
}
}

QuestionVoteManager.php

require '../config.php';
class QuestionVoteManager implements voting
{
 protected $voter;
 protected $vote_type;
/**
 * @var $qtitle (questiontitle)
 */
 protected $qtitle;

/**
 * @return mixed
 */
public function getVoter()
{
    return $this->voter;
}

/**
 * @param mixed $voter most likely a string.
 */
public function setVoter($voter)
{
    $this->voter = $voter;
}

/**
 * @return mixed
 */
public function getVoteType()
{
    return $this->vote_type;
}

/**
 * @param mixed $vote_type
 */
public function setVoteType($vote_type)
{
    $this->vote_type = $vote_type;
}

/**
 * @return mixed
 */
public function getQtitle()
{
    return $this->qtitle;
}

/**
 * @param mixed $qtitle
 */
public function setQtitle($qtitle)
{
    $this->qtitle = $qtitle;
}
public function __construct($dbh)
{
    $this->dbh = $dbh;
}
public function voted()
{
    $sql = "SELECT vote_type from questions_vote WHERE questiontitle=:questiontitle";
    $stmt = $this->dbh->prepare($sql);
    $stmt->bindValue(':questiontitle',$this->getQtitle());
    $stmt->execute();
    $exists = $stmt->rowCount();
    if($exists)
    {
    return true;
    } else {
    return false;
    }
}
public function redirect()
{
    header("Location:" . $_SERVER['REQUEST_URI']);
    exit();
}
public function deleteVote()
{
    $sql = "DELETE FROM questions_vote WHERE username=:username and questiontitle=:questiontitle";
    $stmt = $this->dbh->prepare($sql);
    $stmt->bindValue(':username',$this->getVoter());
    $stmt->bindValue(':questiontitle',$this->getQtitle());
    $stmt->execute();
    $this->subtractVote();
    $this->redirect();
}
public function addVote()
{
    $sql = "UPDATE questions set count = count + 1 WHERE title=:title";
    $stmt = $this->dbh->prepare($sql);
    $stmt->bindValue(':title',$this->getQtitle());
    $stmt->execute();
}
public function subtractVote()
{
    $sql = "UPDATE questions set count = count - 1 WHERE title=:title";
    $stmt = $this->dbh->prepare($sql);
    $stmt->bindValue(':title',$this->getQtitle());
    $stmt->execute();
}
public function updateVote()
{
    // TODO: Implement updateVote() method.
}
public function insertVote()
{
    $sql = "INSERT INTO questions_vote (vote_type,questiontitle,username) values (:vote_type,:questiontitle,:username)";
    $stmt = $this->dbh->prepare($sql);
    $stmt->bindValue(':vote_type',$this->getVoteType());
    $stmt->bindValue(':questiontitle',$this->getQtitle());
    $stmt->bindValue(':username',$this->getVoter());
    $stmt->execute();
    $this->addVote();
    $this->redirect();
}
public function getCurrentvote()
{
    $sql = "SELECT vote_type from questions_vote WHERE questiontitle=:questiontitle and username=:username";
    $stmt = $this->dbh->prepare($sql);
    $stmt->bindValue(':questiontitle',$this->getQtitle());
    $stmt->bindValue(':username',$this->getVoter());
    $stmt->execute();
    $vote_type = (string) $stmt->fetchColumn();
    return $vote_type;
}
public function vote()
{
    if($this->getVoteType() == 'Upvote' && $this->voted())
    {
    $this->deleteVote();
    } else {
    $this->insertVote();
    }
}
final public function getVotes($title)
{
    $sql = "SELECT count from questions where title=:title";
    $stmt = $this->dbh->prepare($sql);
    $stmt->bindValue(':title',$title);
    $stmt->execute();
    $count = $stmt->fetchColumn();
    // MIGHT CHANGE THIS IN THE FUTURE WHEN WE ADD DOWNVOTING..
    if($count < 0)
    {
        exit();
    }
    return (integer) $count;
}
}

Answer 1

I think you might want to reconsider your overall interface and class design here. While I appreciate that you are trying to work with interfaces, I don't think that the interface may be the best way to do what you are wanting to do here. You should always think about the real-world aspects of what you are trying to model. Here, your question and answer classes are not really represent questions and answers at all. Rather, they seemingly are really just two different types of votes you have in your application.

Why is this important?

Because if you just have two different types of the same object, then you might think towards inheritance as part of your solution as opposed to interfaces which are typically used to describe two objects of different types that exhibit similar behavior.

Do you notice that your implementations of the common interface methods across the two classes are almost exactly the same (with DB table you are acting against being the main difference)? This should be a red flag that perhaps this should refactored such that you don't need to re-implement all this functionality in every class that needs voting capability.

There are a couple of ways you might approach refactoring this.

If the questions and answers were truly meaningful objects of their own, (with question/answer text, relations between questions and answers, accepted answers, etc.) rather than just vote objects, you might consider adding a trait to provide voting functionality (including implementation) to each such class that needs this functionality.

Since that is not the case here, I would suggest using a base Vote class (possibly abstract) where you can provide common implementations for most of your methods. This functionality could be inherited by QuestionVote and AnswerVote (or similar) classes and overridden as needed for each type of vote.

---

You are doing absolutely nothing to validate the data being passed to your public methods. That means this code is extremely fragile and likely to be put into unexpected states.

Take your constructors for example. They just blindly accept whatever is passed to them and sets the passed data as the DB connection to be used by the instantiated object.

Since $dbh (a poor variable name by the way) is an object, you should be able to type hint the parameter like this.

// note more meaningful property and variable names
public function __construct(PDO $pdo) {
    $this->pdo = $pdo;
}

This guarantees that these objects are passed a valid dependency upon instantiation. If you try to pass anything else to the constructor, an exception will be thrown and the object will not be created. It fails loudly. This is desired behavior, as the calling code can then do whatever it needs to do to handle the failure condition, which would obviously include not trying to conduct any further operations against that object as would happen now with your current code.

Your setters probably also need validation. For example, perhaps you need to fail loudly if a non-integer (or integer value in string) is set for ID value. Perhaps you need to fail if an invalid vote type is attempted to be set.

---

I think you need to consider either passing all needed dependencies (id's, voter, vote type, etc.) to the constructor, or you need to validate these values are set before performing database queries which rely on them. I do question why caller needs to provide an id for inserting a vote though - is this not autoincrement field in table?

Without understanding how you are working with these objects in your application, it would be hard to determine which approach might be best. If you are treating these dependencies on the objects as immutable during the lifetime of the object, then passing dependencies to constructor probably makes the most sense. If the dependencies can change during lifetime of the object, then perhaps rather than a bunch of individual setters for these dependencies, you provide a single method to set the dependencies on the object where you can perform all validation necessary to make sure the object is set-up properly to work against the database.

---

Why does answer class use ID values as primary key while question class uses title? You mingle qtitle, questiontitle and just title as nomenclature in the question class which seems extremely confusing.

These sorts of inconsistencies probably make it harder for you to see the fact that these classes should probably inherit from a common ancestor with properties like:

$id
$title
$voter
$vote_type

Odd variants in your property names like $answerId and $qTitle don't give you any additional information you need beyond $id and $title as it should already be clear as to whether you are working with an answer object or a question object.

The more I look at this, I wonder whether a single class could actually satisfy all your requirements as question vs. answer vs. [some other type] could just be another column in database in that there is not really any difference in behavior between these separate types.

---

I am having significant struggle understanding how a caller is to operate against these two classes. It is just not clear from reading this code.

Let's look at two use cases - one where you are creating a new vote and one where you want to read out information on an existing vote (i.e for rendering on page).

In the insert use case, it looks to me like you would need to so something like this to work with answer class:

$answerVote = new AnswerVoteManager($pdo);
$answerVote->setVoter($voter);
$answerVote->setVoteType($voteType);
$answerVote->setTitle($title);
$answerVote->setAnswerId($id); // why would caller be setting this? 
$answerVote->insertVote();

This is a really odd usage pattern in that you are turning a transactional operation (inserting a record in the DB) into something that the caller must write six steps/lines of code to complete.

To me a static usage case such as this might be more appropriate:

try {
    $createdVote = AnswerVoteManager::createVote($pdo, $voteType, $voter, $title);
} catch (Exception $e) {
    ....
}

Here you pass all dependencies to a factory-type method which could: validate input (and fail if appropriate), insert record in DB, and return instantiated vote object (with information such as id, count (1), vote type, etc.).

Let's look at your read case now. Let's say you wanted to get the vote count based on unique identifier (title in this case) from question class

Currently, that might have you doing this:

$questionVote = new QuestionVoteManager();
$count = $questionVote->getVotes($title); 

Why instantiate an object at all for this case? Perhaps better usage might be:

try {
    $count = QuestionVoteManager::getVoteCountByTitle($title);
} catch (Exception $e) {
    ...
}

There currently just seems to be no strategy concerning how caller can best interact with these classes. If you couple this with the above thought on inheritance vs. interface, perhaps you end up with class skeletons like this:

abstract class Vote
{
    protected $id;
    protected $title;
    protected $voteType;
    protected $voter;
    // perhaps store DB table here so CRUD operations can be implemented
    // in this abstract class
    protected $dbTable;

    // only let object only be instantiated via class methods
    // standard constructor inherited by implementing classes
    protected function __construct() {
        // not shown - validate input throw exception if unable to set up
        $this->pdo = $pdo;
        $this->title = $title;
        $this->voteType = $voteType;
        $this->voter = $voter;
        $this->insert();
    }

    // provide standard method for instantiating class
    public static function createVote(PDO $pdo, $title, $voteType, $voter) {
        $class = get_called_class();
        return new $class($pdo, $title, $voteType, $voter);
    }

    // common getters and setters
    public function getId() {
        return $this->id;
    }
    public function setTitle($title) {
        // not shown - validate the parameter
        $this->title = $title;
    }
    // etc. for getters and setters

    // vote operation functions
    private function insert() {
        $sql = "
            INSERT INTO `{$this->dbTable}` (title, vote_type, username, count)
            VALUES (:title, :vote_type, :username, 1)";
        // prepare, bind and execute throwing exception on failure
        // update id if successful
        $this->id = $this->pdo->lastInsertId();
    }
    // similar for other CRUD operations    
}

class AnswerVote extends Vote
{
    protected $dbTable = 'answer_votes';
}

class QuestionVote extends Vote
{
    protected $dbTable = 'question_votes';
}

Note here that even the case for inheritance might be weak as mentioned previously, as once you begin to normalize the property and method names across the different vote classes above, you can get to the point as shown above, where the only difference between the two classes is the database table they are pointing at.

---

Now on to some specifics in the code itself:

There are a number of style problems.

• Indentation is inconsistent or even absent in some cases.
• You are mixing camelCase and snake_case in your code. I know PHP is bad about this as a language, but that doesn't mean you shouldn't be consistent within your code (at least within same class/library of classes).
• You are also mixing snake_case and nosnakecase in DB field names. Typically snake_case is a pretty good standard in relational DB's.
• You only partially implement doc blocks. If you are going to use doc blocks (which I suggest), do it consistently, again at least at the class or library level if not for the whole app.
• Your constructor should be the first method listed in your classes.
• Interfaces typically should start with uppercase letter - Voting instead of voting.
• Typically one should put each individual interface, class, or trait in it's own file. Don't group them together in one file without very specific reason. Doing this is like saying that you are designing that class/interface/trait to NOT be reusable on a standalone basis.

Adopt a coding standard (look at PSR standards for PHP as some good suggestions) and enforce it via stylechecker.

---

class AnswerVoteManager

It looks like you are trying to implement voting in this class, so why does the class definition not state this?

---

These classes should have no business in performing client redirection. They should handle voting functionality and that's it. Something further up the stack should handle routing concerns.

---

private function updateVote()
{
    $sql = "UPDATE answer_votes set vote_type=:vote_type";

Do you really want to update every record in the table here?

---

When inserting votes, why make a call (and 2nd query against the DB) to addVote()? You could just set value to 1 on insert and/or have default value for that DB field be 1.

---

public function getCurrentvote($answerid,$username)

Do you really need username for these methods? Shouldn't unique answer id be enough?

Method name seems wrong. You are not getting a "vote", you are getting a "vote type". Also, you are not using the "current" values based on the current object properties at all. A more appropriate name might getVoteTypeById().

Should this be a static method, since it has nothing to do with object context? Perhaps with signature like:

public static function getVoteTypeById($id, PDO $pdo)

---

Many of the method names are confusing and don't really seem to reflect what the methods do. Here's how I might suggest naming them without consideration to any suggestions I have made on refactoring:

getCurrentvote -> getCurrentVoteTypeByIdAndUser [static]
voted          -> hasUserVoted [don't understand this method in instance context. If the object has been instantiated with that user id, doesn't that mean there is a vote? Should this be static?]
redirect       -> [move out of this class]
updateVote     -> updateAllToVoteType [static though this method could just be implemented incorrectly]
addVote        -> incrementCount [concrete]
subtractVote   -> decrementCount [concrete]
deleteVote     -> delete [don't understand this method in instance context - static?]
insertVote     -> insert [static]
vote           -> toggleVote [Not sure I understand this in instance context - static?]