Ian Eure [2006-02-10 19:43 UTC] Needs lots more work. From a quick run-through:
- Needs to follow the PEAR Package Directory Structure: http://pear.php.net/group/docs/20031114-pds.php
- Repeated "if (!isset($conn_params['foo']))" should be cleaned up. You could have default settings in a static class var, then merge with options and store in self::$params.
- The differing backends seem like an ideal place to use a factory pattern.
- The _parseRecord() / Net_CDDB_Disc thing seems silly. Why not have Net_CDDB_Disc parse the record itself? Why not just pass $record instead of each member individually?
- CDDB response codes should probably be constants, i.e. NET_CDDB_RESPONSE_FOUND.
- submitDisc() could use Net_Socket instead of the raw socket functions.
- Unnecessary 'CDDB' prefix on some functions.
- Inconsistent naming of functions. submitDisc() vs. CDDBmotd() vs. CDDBstatistics().
Arnaud Limbourg [2006-03-02 06:22 UTC] I had a quick look and I must say I am impressed with the level of documentation.
Justin Patrin [2006-03-16 07:08 UTC] 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().
|
