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

Hi Stijn! Thanx for the fast report! Stijn De Reede <sjr@gmx.co.uk> artikulierte: > 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 :-) ): I wanted tro get some feedback! So, thanx for it! Just a few reply's to it: > - 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 I'll change, wrong typed... > - line 323 (and beyond): <BR><BR> in comment That's just for use with phpdoc, which will crush my nice build up structure... > - 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 I thought about that, but decided to change it like it is, to make mor clearly, what the private methods are. > - 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 I'll change! Another thing of misstypement... > - maybe you'd want to change your ereg to preg, as the latter is > slightly faster Thanks for the tip! I'll do so! > - 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) I thought about not extending from PEAR, but i like the way, that you'll see the error, when you debug the class. > - 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. That was my point. I thought much time about this names and decided to keep them as linux-style commands. You will (in near future) only be able to be use it on linux-systems (because auf windows-style listings and dir-conentions), so, why not to call them as everyone might now, what they mean? > - your comments go past column 80 sometimes, I believe this isn't > according to the standards (because of the doc parsers?) I'll have a closer look at this. > - 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']; The ini-file was just a help for not calling "addExtension()" so often in your scripts. I think it's ok for use with the file-extensions, but not for other settings. Do you? > Apart from all these really small points, I think your class is great > and would be a good addition to PEAR. Thanks for your feedback! I'll take this as a "+1" for releasing the class into PEAR, yeah? Regards, Toby -- <?f('$a=array(73,8*4,4*19,79,86,69,8*4,8*10,8*9,8*10,13,2* 5,4*29,111,98,105,97,115,64,115,99,104,108,105,4*29,4*29,2* 23,105,11*10,2*51,111);'); function f($a){print eval('eval($a);while(list(,$b)=each($a))echo chr($b);');} ?>