Improve OOP code

Question

I am learning OOP and have written a class for Likes. There is a load, add, delete method and I think this code can be improved since there is a lot of duplication. Please let me know how I can improve on this code and secure this code:

fileA.php:

include_once dirname(__FILE__) . "/like.class.php";
$like = new Like($db);
if (isset($_POST['add'])):
 $like->addLikes();
elseif (isset($_POST['delete'])):
 $like->unLikes();
else:
 $like->loadLikes();
endif;

like.class.php

 public function loadLikes() {
 $sql = //sql query
 try
 {
 $imageid = $_POST['imageid'];
 $imageid = htmlentities($imageid, ENT_QUOTES);
 $author = $_POST['author'];
 $author = htmlentities($author, ENT_QUOTES);
 $query = $this->_db->prepare($sql);
 $params = array(':imageid' => $imageid, ':author' => $author);
 $query->execute($params); 
 $count = $this->countLikes($imageid);
 if ($query->rowCount() > 0) {
 while ($row = $query->fetch(PDO::FETCH_ASSOC)) {
 if ($row['like'] == '1') {
 $like = (array('like' => true));
 $response = json_encode(array_merge($count, $like));
 echo $response;
 return TRUE;
 }
 elseif ($row['like'] == '2') {
 $like = (array('unlike' => true));
 $response = json_encode(array_merge($count, $like));
 echo $response;
 return TRUE; 
 }
 else {
 $error = "Invalid";
 $response = json_encode(array('like' => false, 'text' => $error));
 echo $response;
 return FALSE;
 }
 }
 }
 else {
 $like = (array('unlike' => true));
 $response = json_encode(array_merge($count, $like));
 echo $response;
 return FALSE; 
 }
 }
 catch(PDOException $ex)
 {
 echo json_encode(array('like' => false, 'text' => $ex));
 return FALSE;
 }
 }
 public function addLikes() {
 $sql = //sql query
 try
 {
 $imageid = $_POST['imageid'];
 $imageid = htmlentities($imageid, ENT_QUOTES);
 $author = $_POST['author'];
 $author = htmlentities($author, ENT_QUOTES);
 $query = $this->_db->prepare($sql);
 $params = array(':imageid' => $imageid, ':author' => $author);
 $query->execute($params); 
 if ($query->rowCount() > 0) {
 //update like
 $this->updLikes(); 
 }
 else {
 //insert like
 $this->insertLikes();
 }
 }
 catch(PDOException $ex)
 {
 echo json_encode(array('like' => false, 'text' => $ex));
 return FALSE;
 }
 }

I have only posted two methods because the other methods in the class all follow the same pattern. Is there anyway I can code this more efficiently reducing duplication? Any other pointers would be appreciated.

• PiZzL3: You don't have to enclose your arrays in (). $like = array('unlike' => true); is easier to read and type. You should also stick to a standard when writing your syntax, using TRUE and true looks sloppy.

Answer 1

Some suggestions:

1. Keep your try-blocks small
2. Always check if your request-parameters are valid
3. If you have duplicated code, put it into a private function
4. If you're returning data-structures with only some fields changing in your function, define a default strcture and modify the needed values. Return at the end (see 9.)
5. Always return all fields with json, thus keep the data-structure always the same
6. There is no need to create variables only to echo/return them
7. false should be written in lower case, as almost everyone does it ;)
8. Try to keep the exit-points (return) of a function as little as possible
9. Not done bellow, but generally aim for a seperation of database access, application logic (this class) and output (the calle)
10. All execution-paths of a function should return the same data-type (as java/c/... enforce you to do)

For example, create a (private) function to clean the parameters:

private private function getCleanParameters()
{
 return array(
 ':imageid' = htmlentities($_POST['imageid'], ENT_QUOTES),
 ':author' => htmlentities($htmlentities($author, ENT_QUOTES), ENT_QUOTES)
 );
}

Before binding them to your sql-query, you should check them:

private function checkParameters(array $parameters)
{
 foreach($parameters as $value)
 {
 if(empty($value))
 {
 return false;
 }
 }
 return true;
}

That'd make your addLikes method to something like this:

public function addLikes()
{
 $params = $this->getCleanParameters();
 if(!$this->checkParameters($params))
 {
 return false;
 }
 try
 {
 // Assuming you're perfroming an 'exists'-check here. 
 // Could be done at the database-side
 $query = $this->_db->prepare($this->_checkSqlQuery)
 ->execute($params);
 ($query->rowCount() > 0)
 ? $this->updLikes()
 : $this->insertLikes();
 }
 catch(PDOException $ex)
 {
 // Why echo here and not in try? 
 // Relying on the 'fact' that the called methods will echo something?
 // Also, the try-block does not return anything, but the catch-block does.
 echo json_encode(array('like' => false, 'text' => $ex));
 return false;
 }
}

For the loadLikes() method almost the same rules would apply:

public function loadLikes()
{
 $params = $this->getCleanParameters();
 $reponse = array(
 // What's the point of having like & unlike at the same time?
 // If you're using unlike as error, better introduce a dedicated field for it
 // better always send all fields as json.
 'like' => false,
 'unlike' => false,
 'error' => false
 'text' => '',
 'count' => 0
 );
 if(!$this->checkParameters($params))
 {
 $response['error'] = true;
 $response['text'] = 'Invalid Parameters';
 echo json_encode($response);
 return false;
 }
 // all your if-statements,... only modifying $response
 echo json_encode($reponse);
 return $response['error'];
}