Arnaud Limbourg [2005-09-22 14:40 UTC] First off: Nice work !
Now onto comments :)
The link to the PEAR package on the page is broken (i mean this page http://dev.jystewart.net/XML_Feed_Parser/.
You might want to have a custom exception class extending PEAR_Exception or throw a PEAR_Exception.
In XML_Feed_Parser constructor I would use a switch rather than elseif for feed type detection. Having that in another method altogether might even make more sense (just a thought).
In Atom.php there is a require statement at the very bottom of the script, should be at the top.
I get PHP Fatal error: Call to protected method XML_Feed_Parser_Type::getCategory() from context '' example.php line 22
Andrew Morton [2005-09-22 18:43 UTC] First off, the code looks good. Most of my comments relate to CS and documentation.
Overall:
* Not using the required header comment blocks [1].
* In-consistent in naming of member variables. Some private/protected variables begin with an underscore others don't. The PCS advises that PHP5 code not use the leading underscore [2]. Personally, I prefer $_model but I just want to see it see consistent.
* Member variables don't have a @var tag to document their data type.
XML_Feed_Parser
* Why not use a getter/setter pair for $model if it's public? One benefit is that a setter with a type hint will prevent null values.
XML_Feed_Parser_Type
* getDate($method, $arguments) takes two parameters but only one is used. The documentation seems like it was copied and pasted from another function and doesn't make any sense. It also lists a string as the return type when in fact it would be an integer [3].
* getText(), again, what does the $arguments parameter do? Is it there for use by derived classes?
I didn't get too deep into the derived classes but like I said, it looks good.
andrew
[1] http://pear.php.net/manual/en/standards.header.php
[2] http://pear.php.net/manual/en/standards.naming.php
[3] http://us2.php.net/strtotime
Arnaud Limbourg [2005-09-23 07:36 UTC] There is a problem running the example. The XML_Feed_Parser_Type class getCategory gets called and since it is declared protected the call fails. Changing it to public makes the code work.
The phpdoc of getCategory states it can return array or string, looking at the code it can also return false.
I like this package :)
|
