Re: Package-Proposal: Net_FTP, files for review & test

Tobias Schlitt wrote:

Hi all! A view days ago i asked for releasing my FTP-class into PEAR. Here is what i coded until now for review and test: http://www.schlitt-design.de/ftp.zip Regards, Toby

Great class! I took a look at your code, and it seems clearly coded and documented. However, I've got a few remarks (mind you, this is just my opinion, if you don't want it, just ignore it all :-) ): - line 109: typo: setHostename > setHostname maybe this should even just be setHost (as you can set an IP too) - line 222: typo: raiseErrro > raiseError - line 429: typo in comment: maschine > machine - line 323 (and beyond): <BR><BR> in comment - line 784: _list_and_parse > I think this should be _listAndParse (studly caps to differentiate between words), but as this is a internal function it doesn't really matter - private var _passv, should be _pasv I think, because this is a more widely used term in ftp clients, again, only internal used, so not really bad - maybe you'd want to change your ereg to preg, as the latter is slightly faster - I think $this->raiseError should be changed to PEAR::raiseError, this is how it's done in other classes (no need to extend PEAR anymore, maybe only even include PEAR if there's an error) - on method naming: I think some methods should have more explanatory names, like the following: cd > changeDir, or changeDirectory ls > list, or listFiles rm > removeFile, or deleteFile size > fileSize, or getFileSize With a lot of the current method names it isn't clear what they do (yeah, they do if you're familiar with the linux cli). However I'm not sure of this point, because actually I like the cli-like names. I'd have to think more about it. - your comments go past column 80 sometimes, I believe this isn't according to the standards (because of the doc parsers?) - your ini file stuff might need to be changed a bit, to be able to set the options on beforehand like this: $config = parse_ini_file('Net_FTP.ini',true); $options = &PEAR::setStaticProperty('Net_FTP','options'); $options = $config['Net_FTP']; Apart from all these really small points, I think your class is great and would be a good addition to PEAR. regards, Stijn PS: Hmm... re-reading my mail, it seems I'm really bitching a lot on small points, sorry about that :). Just take it as a compliment, I took the time to closely look at your code.