Lukas Smith [2006-02-28 14:35 UTC] - it would be nice if you would also support MDB2
- way too many exceptions, explore the possibility of using codes to differentiate highly similar exceptions
Lukas Smith [2006-02-28 14:37 UTC] oh and the name does indeed need to be prefixed with "XML_" ..
Lukas Feiler [2006-02-28 14:55 UTC] - I'll add support for MDB2.
- I'll reduce the number of exceptions
About the name: we are talking about the package name as it is used in PEPr, right? With a package name of "XML_Query2XML" (and the category set to XML) PEPr calls the package "XML::XML_Query2XML". If I'm not mistaken, it should be "XML::Query2XML".
Justin Patrin [2006-02-28 16:04 UTC] There are lots and lots of exceptions in here. It's IMHO OK to have lots of Exceptions instead of codes as long as you your exceptions are extended from more general exceptions. I haven't looked closely at your class tree for Exceptions, but you seem to be doing this, at least somewhat. Make sure that the classes of exceptions that a method may throw is documented well (by classes I mean the parent/more general classes of Exceptions that a method may throw)
throw new XML_Query2XML_SkipElementException();
This is an example of exceptions as control-flow. Don't do this. In the first case that I see you're throwing an exception and the catch just runs "break". This in particular is just silly. Just run break where you're throwing this Exception. I see another place where you simply continue on a "SkipElementException". Don't do that. See the Exception RFC for more:
http://pear.php.net/pepr/pepr-proposal-show.php?id=132
Also see:
http://wiki.ciaweb.net/yawiki/?area=PEAR_Dev&page=RfcExceptionUse
Especially see in the above the ExceptionWrapping section. If you do choose to re-throw your own exceptions upon catching an exception you need to wrap the old exception in the new one. PEAR_Exception already supports this (pass the exception in as the second argument to the constructor).
Your tutorial page is rendering a bit off in Firefox. The XML and SQL in your page is quished together (bad line height or margin I think).
You generally don't need & in PHP5 (such as =&). PHP5 uses references for objects by default.
Please use ' instead of " for strings wherever possible.
http://pear.reversefold.com/strings/
Lukas Feiler [2006-02-28 16:23 UTC] Thanks Justin! I'll work on the code and test some other phpDocumentor converters.
Lukas Feiler [2006-03-13 22:04 UTC] A new release of XML_Query2XML is now available from http://query2xml.sourceforge.net.
It implements all recommendations made after the first proposal:
- full MDB2 support
- new exception handling: just three different
exceptions that all extend XML_Query2XML_Exception.
- every exception that is thrown or bubbles up is
documented in the API-docs; the tutorial has a
new "Exception Handling" section.
- using ' instead of " where-ever possible
- as PHP5 uses references for objects by default & (such as
in =&) is now only used where necessary.
- the tutorial now renders just fine in all common browsers
Additionally the number of PHPUnit2 unit tests was extended to 99.
All comments are greatly appreciated!
Martin Jansen [2006-03-16 21:42 UTC] Usually PEAR code omits the brackets when using require_once/include_once.
Lukas Feiler [2006-03-17 21:47 UTC] The new release v0.5.1 includes the following changes:
- all lines now have a maximum length of 85 characters
- no () with require_once (thanks Martin!)
- always using {} with code blocks
- no silencing of any calls using @
- using /* ... */ for all multi-line comments
- enhanced documentation for private methods
- code enhancements inside XML_Query2XML::getXML()
Please let me know if there is anything else that needs to be corrected.
|
