Re: [PEPr] Comment on Networking::Net_CDDB

Re: Feedback on proposal: Networking::Net_CDDB Thanks for the feedback. I'm working on fixing the directory structure right now. I like the CONSTANTS idea and cleanup of connection parameters stuff as well, I'll fix those shortly. I'll also fix the method names ( CDDBversion(), etc. ), the initial thought was to differentiate those methods because they pertain more to the server then getting the audio CD data, but now I look at it and its kinda ugly with the inconsistent naming. I'm unclear as to what benefit a factory pattern would offer instead of a driver-based architecture. I'm also unclear as to the benefit of using Net_Socket instead of the raw socket connection as its a pretty simple connect, send data, disconnect process. Maybe you could explain/give me an example of why these things would be a good idea? As far as having _parseRecord() in the CDDB class and not in the Net_CDDB_Disc class: If I was to pass the entire string record to Net_CDDB_Disc, then you could only create Net_CDDB_Disc objects if you already had an entire record. I want to be able to do this so I can submit new discs: $disc = new Net_CDDB_Disc('my disc id', 'The Shins', 'Oh, Inverted World', ... ); $cddb = new Net_CDDB(NET_CDDB_HTTP, NET_CDDB_CDDISCID); $cddb->submitDisc($disc); Would it be better if I made the Net_CDDB_Disc accept either a single array or a single string as a parameter? That way you could pass in a CDDB record string (which would be parsed by Net_CDDB_Disc::_parseRecord()) or an array: $disc = new Net_CDDB_Disc(array('my disc id', 'The Shins', 'Oh, Inverted World' ... )); -OR- $disc = new Net_CDDB_Disc($string_containing_entire_cddb_record); Thanks, Keith > -------- Original Message -------- > Subject: [PEAR-DEV] [PEPr] Comment on Networking::Net_CDDB > Date: 10 Feb 2006 19:43:53 -0000 > From: Ian Eure <ieure@blarg.net> > To: PEAR developer mailinglist <pear-dev@lists.php.net> > CC: Ian Eure <ieure@blarg.net>, Keith Palmer > <keith@uglyslug.com> > > > Ian Eure (http://pear.php.net/user/ieure) has commented on > the proposal for Networking::Net_CDDB. > > Comment: > > 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(). > > Proposal information: > http://pear.php.net/pepr/pepr-proposal-show.php?id=320 > > -- > Sent by PEPr, the automatic proposal system at > http://pear.php.net > > -- > PEAR Development Mailing List (http://pear.php.net/) > To unsubscribe, visit: http://www.php.net/unsub.php > >