Comments for "Net_CDDB"

» Submit Your Comment
Comments are only accepted during the "Proposal" phase. This proposal is currently in the "Finished" phase.
» Comments
  • 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:

    - 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().