Return to Answer

• 20.3k
• 4
• 70
• 158

• 20.3k
• 4
• 70
• 158

A lot of times people post PHP here and it's often very ugly, this is not one of those times.

Your classes and design are beautiful, and it's very obvious what your intentions are. You've built a good structure, and you've commented clearly. In fact, I saw this comment as was immediately impressed:

//we need to get the data the following way because we have the Content-Type header set to application/json, so $_POST will no longer be populated
$rawPostData = file_get_contents('php://input');
$json = json_decode($rawPostData);

There are people who say comments are "unnecessary" and you should never have them — I disagree and this is the perfect example of why you should have them. Your comment here tells me what I need to know: why you chose this method of $_POST. That's it, that's exactly what I needed.

Now, all the good information aside, there are a couple things that could use improvement, and many of them are 'nit-picky', but if they're corrected then this code goes from very good to amazing:

• Consistent whitespace/indentation. Each file uses mixed indentation/whitespace, if you're going to do thing { // Thing then space then open brace then you should always do that, if you're going to do thing{ // Thing then open brace without space, then you should always do that. Don't mix them, it makes for inconsistent code (and often harder to read for no reason).
• Don't add a bunch of white-space to line parameters up. I'm looking at this array section:

$inputsAreValid = $this->Validator->checkInputsAreValid(array(array("input" => $facebookUserID, "minLength" => 10, "maxLength" => 30), 

 array("input" => $facebookName, "minLength" => 2, "maxLength" => 40),
 array("input" => $profilePicURL, "minLength" => 10, "maxLength" => 250))); 

Instead of writing it like that, break everything to the next line and move one-indentation level in:

 $inputsAreValid = $this->Validator->checkInputsAreValid(
 array(
 array("input" => $facebookUserID, "minLength" => 10, "maxLength" => 30), 
 array("input" => $facebookName, "minLength" => 2, "maxLength" => 40),
 array("input" => $profilePicURL, "minLength" => 10, "maxLength" => 250))); 

You keep the same readability of aligning your array definitions, but you also keep things close to the left side, which is probably where everyone dealing with this code starts reading.

• Don't use magic strings/values. This is a big one, and no one ever agrees on what's "magic", but in the following function I think it's safe to say that the $data indexers are magic:

public function checkFacebookID($facebookUserID){
 /*
 * This method checks to see if a users facebook id exists in our facebook_users table.
 */
 $data = array();
 try{

 $this->query = "SELECT userID FROM facebook_users WHERE facebookUserID = :facebookUserID LIMIT 1";
 $this->statement = $this->pdoConnection->prepare($this->query);
 $this->statement->bindValue(':facebookUserID', $facebookUserID, PDO::PARAM_STR); 
 $this->statement->execute();
 $this->statement->setFetchMode(PDO::FETCH_ASSOC);
 $rowCount = 0; //store the count into a local variable

 while($row = $this->statement->fetch()){
 //store the data from the result of the query into an associative array. 
 $data['userID'] = $row['userID'];
 $rowCount++;
 }
 if($rowCount > 0){
 //The facebookUserID exists in the database.
 $data['facebookUserIDExists'] = true;

 return $data;
 }else{
 $data['facebookUserIDExists'] = false;
 return $data;
 }

 }catch(PDOException $e){
 //Would it be a good idea to log this error in a php log file?
 $data['exceptionOccurred'] = true;
 $data['exceptionMessage'] = $e->getMessage();
 return $data;
 } 
}

I would also throw the SQL queries into a builder of some sort.

• Regarding error file storage: use your own discretion - if it's important, then log it. You have a comment: Would it be a good idea to log this error in a php log file?, and there's no "one size fits all" solution here. Use your discretion, if you think that it would be good to log, then log it. If not, then don't. In this case, I would probably err on the side of logging the error itself, database errors are either very unique, or very general. There's not a lot of "in-between."

• Build abstractions where helpful. You do the following multiple times:

}catch(PDOException $e){
 $data['exceptionOccurred'] = true;
 $data['exceptionMessage'] = $e->getMessage();
 return $data;
}

Instead of having to remember to set $data['exceptionOccurred'] and such, I would build a function that takes the PDOException and fills your $data, then return it:

 } catch (PDOException $e) {
 // The first parameter is whether to log or not - make this general so you can just tell the error handler
 return ErrorHandler::buildError(true, $data);
 }

Overall, wonderful work! This code is very clean, very easy to follow, and very maintainable. Most of the issues I pointed out are nitpicks, they're just things that help improve consistency and usability.