Re: [PEPr] Comment on System::System_WinDrives

Hello Philippe, Thank you very much for your constructive comment. I fixed most of the things, the phps files have been updated. > I suggest you use PEAR::loadExtension() method instead of @dl() calls. I didn't know that it exists.. > The constant names should follow the name of the package > define('SYSTEM_WINDRIVE', ...) Yeah, that's right. I just copied them from the C headers. Now they are "SYSTEM_WINDRIVE_FIXED" and so, so it is not *really* correct as "SYSTEM_WINDRIVES_DRIVE_FIXED" would be - but that's just too long. > getGetName() and setGetName() are odd names, why not using getReadNames() > and setReadNames(). I hadn't a better name, but you're right. > You may also make your $bGetName, $objAPI and $objFFI private members of > the class, and rename them $_readNames, $_objAPI and $_objFFI IMO an @access protected should be enough. > I would also suggest to rename $arTypeTitles to $typeTitle. AFAIK we don't > prefix variable names by their abbreviated types. BTW, you may want to use > the constants you declare above as index of that array. I use hungarian notation in my code all the time; so why stop using it? Especially in php 4 it's necessary to know which type a variable has or should have - to the small indicators are a real help. Additionally, there is no coding standard for prohibiting this. > Also, there may be confusion about adding the trailing backslash to the > drive's name. "C:" would be the drive and "\" the name of the root > folder > on that drive. Drives in Windows are "<letter>:\", that's what people know (or think to know) and so I'd like to keep that. > Also, make sure you use the new docblock header file. Done. > Also for docblock there is only one space between the * and the text > following it. I don't know if the phpDocumentor would choke on that. Doesn't phpdoc strip the lines? But fixed. > Add the @access doc tag information for members and methods. Done. Regards/MfG, Christian Weiske -- XMMS is playing now: Sarah Connor - I canÅœt lie

Attachment: [application/pgp-signature] OpenPGP digital signature signature.asc