[PEPr] Comment on Web Services::Livejournal

Justin Patrin (http://pear.php.net/user/justinpatrin) has commented on the proposal for Web Services::Livejournal. Comment: I have not checked through all of your code, but I have enough comments for this round. You have a lot to work on. set_time_limit() shouldn't be called in the include file, and especially not just when it's included. You should instead put a warning in the documentation that tells people to make sure their time limit is set high. Please use single quotes instead of double quotes for strings: http://pear.reversefold.com/strings/ Lower case language constants are preferred (true, false). There is some strange indentation in your files. Make sure that you're indenting with four spaces and no tabs. See the PEAR CS section for more. trigger_error is not allowed in PEAR packages. For PHP5 packages use PEAR_Exception for all error handling. Don't check the PHP version on runtime. The installer will check for the required version of PHP when the package is installed. Don't use register_shutdown_function. It is especially useless for disconnecting from a database (or closing any resource) as PHP will do this at the end of the script anyway. If you want a destructor, use __destruct. It would be helpful to allow the user to pass in their own DB object (in which case the destructor above would be harmful). Please review your class and file structure. Class names must map directly to paths e.g. Services_LiveJournal_Common Services/LiveJournal/Common.php. You don't need & when setting an object to a variable, such as with $this->dbh = &DB::connect(). PHP5 uses references automatically for all objects. There should alsways be spaces around operators such as =, +, -, *, =>, ',', etc. Long lines should be wrapped at around 40 chars. Declare arrays before using them: $data = array(); $data['response'] = ''; Declare arrays in one call instead of multiple. E.g.: $data["response"]=""; $data["replyto"]=$replyto; $data["parenttalkid"]=$parenttalkid; Should be: $data = array('response' => '', 'replyto' => $replyto, 'parenttalkid' => $parenttalkid); You don't need to unset() variables when they're no longer in use, especially when it it just at the end of a function. PHP will clean up local variables for you. The many unset() calls in your code are cluttering it up and are simply unneccesary in almost all cases. Use DB's quoteIdentifier() to quote table and field names instead of hard-coding the (only mysql knows ). Use quoteSmart() instead of hard-coding '. e.g.: 'SELECT max('.$db->quoteIdentifier('id'). ') FROM '.$db->quoteIdentifier('lj_comments'). ' WHERE '.$db->quoteIdentifier('journal'). ' = '.$db->quoteSmart($this->_main->settings['username']) When querying for a single row use getRow() instead of query(), fetchRow(), and free(). When querying for a single value use $db->getOne(). $comments_data[(string)intval($row[0])] is far too convoluted. First, (string) does nothing as intval() is creating an integer and PHP treats integer strings as integers when used as an array key. $array = array(); $array['2'] = 1; $array['02'] = 2; foreach ($array as $key => $val) { echo var_dump($key); } Second, forcing the key as an integer here really serves no purpose as your DB table has the id field as an integer already and you will get an integer back. You most certainly do not need to do this: $comments_data[(string)intval($row[0])] = array("parentid"=>$row[1], "subject"=>$row[2], "body"=>$row[3], "date"=>$row[4], "status"=>$row[5], "poster"=>$row[6]); Use DB_FETCHMODE_ASSOC instead. All output of variables into XML needs to be escaped unless it was previously escaped or sanitized. Use htmlentities() with ENT_QUOTES for attributes: '<comment level="'.htmlentities($comment['level'], ENT_QUOTES).'">' And use plain htmlentities() when text is not in an attribute. '<comment>'.htmlentities($text).'</comment>' Private functions need full docblocks too. Debugging would be better done through the Log package instead of always to STDERR IMHO. Proposal information: http://pear.php.net/pepr/pepr-proposal-show.php?id=360 -- Sent by PEPr, the automatic proposal system at http://pear.php.net