Re: [PEPr] Comment on Networking::Net_CDDB

Keith Palmer Jr. wrote: >> Justin Patrin (http://pear.php.net/user/justinpatrin) has commented on >> the proposal for Networking::Net_CDDB. >> >> Comment: >> >> There seem to be a lot of constants.... I suggest for the drivers just to >> use a straight string instead of a constant. >> >> if ($cdreader == NET_CDDB_READER_USERDEFINED) { >> ; >> } else { >> huh? Just use != >> >> connect() and disconnect() aren't returning anything if they aren't >> connecting ir disconnecting. >> >> Store strpos($this->_buffer, "\n") instead of re-calling it 3 times. >> Something like: if (false !== ($pos = strpos($this->_buffer, "\n"))) { >> Don't use () with return. >> >> Always use {} with blocks, even if the block is one statement. (such as >> with the foreach in calculateDiscId() and elsewhere.) >> >> use Net_Socket instead of using sockets directly. >> >> submitDisc() should be returning a PEAR_Error on error. $err isn't >> used at >> all... >> >> The many parameters to Net_CDDB_Disc's constructor seems to be a bit >> much. >> Why not just use an associative array? >> >> You don't need a seperate factory class. Put those functions in the main >> class. >> >> Don't use dirname(__FILE__)! Include as from the include_path. Always. >> >> connect() ought to return a PEAR_Error if it fails. (PEAR::raiseError()) >> >> use urlencode() or rawurlencode() instead of a direct str_replace(). >> >> Proposal information: >> http://pear.php.net/pepr/pepr-proposal-show.php?id=320 >> > > Alright, I've made several more changes to the Net_CDDB proposal... > things have been significantly cleaned up. > > Does anyone else have any more comments / *if I call for votes, will > anyone vote for it* ? I can't help feeling the _parserecord could be improved. Anyway, if it does the job :) Arnaud. > > > Thanks so much, > - Keith > > P.S.: Thanks for the comments so far Ian, Arnaud, and Justin! >