5
\$\begingroup\$

Nothing too special here, just a bunch of stuff. Please tell me if you would've done something different.

<?php /** * SQL_Helper are functions which help with... SQL... :) * * @package SQL_Helper * @author Itai Sagi * @version 1.0 * SQL Helper functions * Assumption: a global PDO $db is active; */ class SQL_Helper{ // this function escapes and trims a string for insertion to SQL Database static public function escapeSQLStatement($string){ return mysql_real_escape_string(trim($string)); } // this function takes a query and returns a result_array if query returned results, false otherwise. static public function result_array($query){ global $db; $result = array(); foreach ($db->query($query) as $row){ $result[] = $row; } if (!empty($result)){ return $result; } return false; } } /** * This is the description for the class below. * * @package User * @author Itai Sagi * @version 1.0 * Basic User class * */ class User{ // class constructor which accepts a user_id as an optional parameter, // so you won't have to use the setUser() method. function __construct($user_id = null){ $this -> userID = $user_id; } /** * function setUser($id); * This function sets the user_id for the class * Parameters: int $user_id, assuming a valid $user_id; * return values: the class itself. **/ function setUser($user_id){ $this -> userID = $user_id; return $this; } } /** * @package User * @subpackage Contacts * @author Itai Sagi * @version 1.0 * The following class handles Contacts of a user. * Usage example: * searching user's contacts: * $contacts = new Contacts(); * $foundContacts = $contacts -> setUser(76) -> searchContactsByName('John'); * * getting contacts whose birthdays are up to 3 days from now: * please note that here, the user is still set to '76'. * $upcomingBirthdays = $contacts -> upcomingBirthdays(3); * **/ class Contacts extends User{ function __construct($user_id = null){ parent::__construct($user_id); } /** * function: searchContactsByName($string); * This function searches the user's contacts for users whose name contain $string; * parameters: String $string - the name to search for * return values: an array containing the user's contacts which were found or FALSE if none were found or $id wasn't set **/ function searchContactByName($string){ if ($this -> userID){ $string = SQL_Helper::escapeSQLStatement($string); $query = "SELECT lname, fname, email FROM contacts c JOIN users u ON u.id = c.contact_id WHERE c.id = '$this->userID' AND (lname LIKE '%$string%' OR fname LIKE '%$string%')"; return $SQL_Helper::result_array($query); } return false; } /** * function: upcomingBirthdays($daysInterval); * This function searches the user's contacts for users whose birthday is in the upcoming $daysInterval; * parameters: int $daysInterval - the amount of days to get the birthdays * return values: an array containing the user's contacts which were found or FALSE if none were found or $userID wasn't set **/ function upcomingBirthdays($daysInterval){ if ($this -> userID){ $query = "SELECT lname, fname, email, birthday FROM contacts c JOIN users u ON u.id = c.contact_id WHERE c.id = $this->userID AND FLOOR( ( UNIX_TIMESTAMP(CONCAT( YEAR(CURDATE()) + (DATE_FORMAT(p.bday, '%m-%d') < DATE_FORMAT(CURDATE(), '%m-%d')), DATE_FORMAT(p.day, '-%m-%d'))) - UNIX_TIMESTAMP(CURDATE())) / 86400) < $daysInterval"; return $SQL_Helper::result_array($query); } return false; } } /** * @package User * @subpackage Messages * @author Itai Sagi * @version 1.0 * The following class handles the messages of a user * Usage examples: * * send a new message: * $message = new Messages(); * if ($message -> setUser(65) -> sendMessage("Hello, how are you?", 12)){ * echo "Message sent successfuly"; * } * else{ * echo "Message delivery failed"; * } * * getting an a list of all the messages: * $messages = new Messages(); * $messageArr = $messages -> setUser(65) -> getMessages(); * foreach ($messageArr as $m){ * // do something. * } * **/ class Messages extends User{ function __construct($user_id = null){ parent::__construct($user_id); } /** * function: getMessages(); * This function returns an array of all the messages a user recieved * return values: an array containing the messages or FALSE on failure / no messages. **/ function getMessages(){ if ($this -> userID){ $query = "SELECT * FROM messages m JOIN users u ON u.id = m.from_id WHERE m.user_id = $this->userID ORDER BY send_time ASC"; return SQL_Helper::result_array($query); } return false; } /** * function: sendMessage($message, $recipientID); * This function "sends" a message from $this->userID to $recipientID * parameters: $message - the message content, $recipientID - the target user id who should get the message * return values: the message_id if successful or FALSE on failure **/ function sendMessage($message, $recipientID){ if ($this -> userID){ $message = SQL_Helper::escapeSQLStatement($message); $query = "INSERT INTO messages (message_text, user_id, from_id, send_time) VALUES ('$message', $recipientID, $this->userID, NOW())"; if ($db -> query ($query)){ return $db -> lastInsertId(); }; } return false; } /** * function: searchInMessages($string) * This function searches all the messages of a user for the text $string * parameters: $string - the text to look for * return values: an array containing the messages or FALSE on failure / no messages found. **/ function searchInMessages($string){ if ($this -> userID){ // Please note that because there's a full text index on the column, we aren't performing a full // table scan by using (LIKE '%$text') $string = SQL_Helper::escapeSQLStatement($string); $query = "SELECT * FROM messages m JOIN users u ON u.id = m.from_id WHERE m.user_id = $this->userID and m.message_text LIKE '$string%' ORDER BY send_time ASC"; return SQL_Helper::result_array($query); } return false; } } ?>
\$\endgroup\$
0

4 Answers 4

Reset to default
11
\$\begingroup\$

Disclaimer: I am not a php developer

Other posts already mentioned good points as far as specifics about SQL and php and some problems with your inheritance structure. I am going to talk more about your craftsmanship in this post.

"SQL_Helper are functions which help with... SQL... :)"

This is like going to an interview with your shirt untucked and instead of properly shaking hands you greet them with 'yo bro'. Simply unprofessional. Won't get you hired.

Anyways... onward...

Documentation

Your documentation is messy. Most great developers care about how their code and documentation looks. They treat writing them as craft. You want the look/layout to be pleasing to the eye and the contents to be pleasing to the brain. This means you need to keep things consistent looking and to the point.

  • Capitalize all your sentences.
  • You should stick with /** */ for method/class level documentation and use // only for one-liners inside methods.
  • Try to avoid using 'this function' when you write about the function. For example, I would rewrite // this function escapes and trims a string for insertion to SQL Database to /** Escapes and trims a string */ or /** Prepares a string to be used in an SQL statement */ (but I do not want to get caught too much in the semantics since I do not know much php...)
  • If you absolutely have to use @package, @author and @version attributes, those should be the last thing in your documentation. They do not help the user whatsoever with using or maintaining your class. The most important things should usually go on the top. Some places you have important information both above and below and sometimes only below.
  • I am unsure about your duplicated method signatures in your documentation (Example: * function setUser($id);) I am positive your documenation tool extracts the signature automatically from the source itself, so this is just redundant.

Code

  • You do not have a userID attribute on your User class. I do not know if this is standard procedure for php but even if it runs in its current form, you should specify the attributes for your classes and those should usually be private. (Standard OOP here, "hide your privates")

  • Minimize your API. You should probably choose to go with the constructor or the setter in the User class. I personally would go with the constructor. So code can be more terse by saying

    $contact= new Contact(72);

    $contact->searchContactByName('John');

  • You should also not allow a null user when you are constructing one, so you do not have to worry about logic like this everywhere:

    if ($this -> userID){ ... } return false;

If your User class enforces the assumption that a userId cannot be null, all that code can be eliminated.

  • Be more creative when choosing method names. Your class name already gives semantics to most of your methods. Fo example, in SQL_Helper::escapeSQLStatement() the 'SQL' is redundant. Why not SQL_Helper::escapeStatement()?
\$\endgroup\$
1
  • 2
    \$\begingroup\$ @package @author @params, etc are very useful when working with an IDE that pickup on these. \$\endgroup\$ Commented Mar 23, 2012 at 17:44
11
\$\begingroup\$

To be honest, if I were an employer I wouldn't read past the first class, which contains static, global and use of the deprecated mysql_*. They should be avoided, especially mysql_* which should be replaced by mysqli or PDO.

All of your classes are tightly coupled to SQL_Helper due to the static functions. If you passed in the SQL_Helper into each class (dependency injection) in the constructor, you could remove this tight coupling.

\$\endgroup\$
5
  • \$\begingroup\$ the $global was an assumption given by the employer. \$\endgroup\$ Commented Mar 14, 2012 at 17:47
  • \$\begingroup\$ @ItaiSagi I believe they're OK in this context, but it's generally a good idea to try avoiding such things. For example, regarding the global, it's easy to use the Singleton pattern which does essentially the same thing but with better encapsulation. \$\endgroup\$ Commented Mar 14, 2012 at 18:55
  • 2
    \$\begingroup\$ @Cygal The singleton pattern unnecessarily limits you to having only 1 database. It is better to just create one and pass it in where it is needed. \$\endgroup\$ Commented Mar 15, 2012 at 3:52
  • \$\begingroup\$ You don't need multiple databases. One database per model is clearly enough. The Singleton pattern was originally designed to manage a pool of connections to a database, but it also works fine for a single connection. And it's not mandatory at all and can be implemented in various ways. A global, Zend_Registry, CodeIgniter's database configuration, and so on. \$\endgroup\$ Commented Mar 15, 2012 at 8:12
  • \$\begingroup\$ I agree with Paul. A classical singleton is almost as bad as a global variable. Both databases and configurations should not be singletons. Just create a single instance and pass it into the class that needs it, instead of having the class ask a global variable for the singleton instance. \$\endgroup\$ Commented Mar 16, 2012 at 11:30
7
\$\begingroup\$

SQL_Helper::result_array() should return an empty array if the database returned an empty result set. In most cases, it is not an error condition when no records, that match your query, exist in a database.

You use inconsistent naming conventions. Rename method result_array() to getResults() or similar, so that it is named using the same naming convention as used by the methods in your other classes.

It seems strange that the classes Contacts and Messages inherit from class User. Inheritance usually signals a relationship of is a. But a contacts is not a user; a user has contacts.

You assume a valid user in User::setUser. As a result, there is the risk of a SQL injection if the developer who uses the User class does not guarantee this precondition.

\$\endgroup\$
2
\$\begingroup\$

Just another suggestion:

function __construct($user_id = null){ $this -> userID = $user_id; }

This is a huge problem to me. This just means that whenever someone will try to do a request like

PDO_xx query ("select * from XX where user_id=%1", array( $user_id ) )

There won't be any error, which is absolutely not acceptable. I always do something like this in all my classes (note that I've removed the comments but you always have to comment):

class User { public function __construct($user_id = false) { if (is_int($user_id)) { $this->setUserId($user_id); } } public function setUserId($user_id) { if (!is_int($user_id)) { throw new Exception("user_id: expected int"); } $this->userID = $user_id; return $this; } public function getUserId() { if (!isset($this->userID)) { throw new Exception("userID not set"); } return $this->userID; } }

And I don't know about the Php version, but if it's recent, you always have to put functions scope: private, protected or public.

Another thing: note my K&R indentation style. This may not be the best in the world, but it's very famous, and used by a lot of people (in C, but in C++, Php and also Java).

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.