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* ?
Thanks so much,
- Keith
P.S.: Thanks for the comments so far Ian, Arnaud, and Justin!