patternphpMinor
Voting system for a website
Viewed 0 times
websitesystemvotingfor
Problem
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.
```
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_i
```
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_i
Solution
A
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
I'm not going to write your code for you, but effectively, you need to take a look at doing the following:
So, all in all, a lot of refactoring to be done here. Well done for using
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
\PDOdependency 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 likeuserHasVote(User $user, $voteId).
- You could probably get away with putting
addVote(Vote $vote)andremoveVote(Vote $vote)in your repository. This will require you to construct a newVoteobject before you save it to the database (a Factory will help here, and your repository will give you aVoteobject 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
Votingcomponent (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.
varhas no place in your code. Use the correct access modifier, as inprotected $db. The same goes for your functions (methods), make the ones you want others to consume (the public api)public function nameinstead.
So, all in all, a lot of refactoring to be done here. Well done for using
\PDO and prepared statements though.Context
StackExchange Code Review Q#82271, answer score: 7
Revisions (0)
No revisions yet.