[PEPr] Comment on XML::XML_Feed_Writer

Justin Patrin (http://pear.php.net/user/justinpatrin) has commented on the proposal for XML::XML_Feed_Writer. Comment: If you're wondering why this comment is coming so "late" it's because you never put up your source on the web for easy viewing. I will vote no on this package unless the following problems are addressed. The structure of this package is not acceptable. Requireing flat code (as bertrand put it) is not ok. Not only does it fly in the face of OOP practice, it's not extendable at all. Please refactor your code to have each of the writers as a seperate backend class. Two possiblities for structure that I see are: 1) Make each of the writers a class which extends a base class (likely the current XML_Feed_Writer) and implement a factory to get the one you need. 2) Make each of the writers a class which acts as a backend. The main class instantiates the backend depending on which you want and accesses it as an internal property. $docs should not be defined as it is. It should be defined by each of the writer classes. Why are you using self:: for calls to the current class? The correct syntax is $this->. e.g. $this->validateValue($item), not seld::validateValue($item). I see $error_message being set in saveFeed but nothing is done with it. A PEAR_Exception should be thrown on an error. docblock for saveFeed is incorrect. DO NOT silence errors on function calls with @. If an error happens the dev needs to see it and fix it. Putting a @ in hides the problem. Some static strings use " instead of '. Please use ' for all static strings without special chars in them. Would it be possible to use the Date package instead of implementing your own parseDate? If not, could your code be added to the Date package? (assuming it's cleaned up...) Your 'a' and 'A' formats only list 'am' and 'AM'. What about 'pm' and 'PM'? Your $dateFormat regex snippets don't need the ( and ). These can be added in the for loop. validateValue() uses the variable $this->skidays which doesn't exist. The function validateValue doesn't seem to do anything. Not only does it seem to be doing 3 different things, the way that it works makes no sense to me. It is highly unlikely that something entered will === skipHours, skipDays, or textInput and thus will "validate" by default. I suggest you break the different types of validation out into their own functions. Please also add a better docblock which describes what these functions are supposed to do. Proposal information: http://pear.php.net/pepr/pepr-proposal-show.php?id=258 -- Sent by PEPr, the automatic proposal system at http://pear.php.net