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.

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'];
}