Re: [PEPr] Comment on Web Services::Livejournal

Justin, I have just updated the code. New code is linked here. I have changed it according to your comments. http://pear.php.net/pepr/pepr-proposal-show.php?id=360 Can you check it out? Nikita. Justin Patrin wrote: > 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 >