HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Improve OOP code

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
improvecodeoop

Problem

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));

Solution

Some suggestions:

  • Keep your try-blocks small



  • Always check if your request-parameters are valid



  • If you have duplicated code, put it into a private function



  • 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.)



  • Always return all fields with json, thus keep the data-structure always the same



  • There is no need to create variables only to echo/return them



  • false should be written in lower case, as almost everyone does it ;)



  • Try to keep the exit-points (return) of a function as little as possible



  • Not done bellow, but generally aim for a seperation of database access, application logic (this class) and output (the calle)



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

Code Snippets

private private function getCleanParameters()
{
     return array(
         ':imageid' = htmlentities($_POST['imageid'], ENT_QUOTES),
         ':author' => htmlentities($htmlentities($author, ENT_QUOTES), ENT_QUOTES)
     );
}
private function checkParameters(array $parameters)
{
    foreach($parameters as $value)
    {
         if(empty($value))
         {
             return false;
         }
    }
    return true;
}
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;
     }

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

Context

StackExchange Code Review Q#2664, answer score: 5

Revisions (0)

No revisions yet.