[PEPr] Comment on File Formats::iCal

Greg Beaver (http://pear.php.net/user/cellog) has commented on the proposal for File Formats::iCal. Comment: Another comment: The constants in File_iCal_File class are not useful. It is far easier and less error-prone to type 'PUBLIC' than it is to type File_iCal_File::ICAL_CLASSIFICATION_PUBLIC. You still have to verify that a string passed in by the user is valid. In addition, drop the ICAL_ - it's implicit in the class name, which is required to use class constants. More useful would be to have shorter constants like CLASS_PUBLIC. In addition, looking around at other classes where you use ICAL_CLASSIFICATION_PUBLIC free of the FIle_iCal_File class (which should be FIle_iCal, no?), when it should be File_iCal_File::ICAL_CLASSIFICATION_PUBLIC. I'm assuming you haven't had time to fully test everything yet :). In addition, this is just unnecessary: public function addAttachment($a) { File_iCal_BaseComponent::addAttachment($a); } The base class is abstract, just make them public. If you are simply trying to hide them from classes that don't use them, perhaps they don't belong in the base class. Make the properties protected and the methods public. More improper stuff: self::processComponent() in iCalendar constructor, but processComponent() is not static, use $this->processComponent(). Also, you're PHP5 - don't use trigger_error() in a constructor, use an exception! The iCalendar class is really a parser, call it File_iCal_Parser, and use it to return a File_iCal class. Perhaps it could even export a Calendar class but I get ahead of myself, that's just a feature request. I would put addEvent() and so on in File_iCal, so you don't have to parse an iCal file just to make a new one. Hope this is helpful. Proposal information: http://pear.php.net/pepr/pepr-proposal-show.php?id=251 -- Sent by PEPr, the automatic proposal system at http://pear.php.net