3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – PiZzL3 May 28 '11 at 18:04
5
\$\begingroup\$

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.