Till Klampaeckel [2009-10-12 23:48 UTC] First off, there are a lot of reasons why you should never race to a 1.0. The most important is that alpha and beta versions allow you to adjust and improve the API, while this is nearly impossible once stable.
Then on to a couple comments on the code:
1) First off, one-class-per-file rule. This means that all your classes should probably be called KML_*. Or if it's acceptable, XML_KML_*, e.g. XML_KML_Place, vs. KMLPlace, this results in a path name, XML/KML/Place.php.
2) It would be much nicer if you use used xmlwriter or simplexml to create your xml.
3) Unit tests would be very much appreciated, and I think they shouldn't be too hard to come up with since the result seems relatively straight forward.
4) You don't want to send headers in PHP code (because of among all the things why this is not a good idea, making it harder to unit test your code is one of the most obvious).
5) KMLStyle (or well, XML_KML_Style, or KML_Style), is not a bad idea, but I'd appreciate set*() and get*() that also validate input when needed, when needed. I'd suggest you implement those in KMLPlace, etc. as well.
6) For addItem(), you could force e.g. XML_KML_Common in the signature:
function addItem(XML_KML_Common $item), then all the different objects extend from XML_KML_Common and you created a simple object hirachey. Could also make sense to add addStyle() (or maybe setStyle()) for the different types of objects.
7) In some cases exceptions wouldn't hurt -- e.g. for invalid arguments. FALSE is too plain, and since the methods can return FALSE for various reasons, it becomes tedious to debug.
8) With a lot of add*()/set*(), a fluent interface would be nice.
9) create() -- personally, I'd implement __toString() which would replace create() completely (or act as a wrapper).
10) Not sure if save() is needed.
11) I can see cloning come in handy -- so I suggest you implement the magic method.
12) Maybe a destructor to clean up?
13) Please implement empty constructors.
14) Last but not least, run PHP_CodeSniffer to fix all the obvious issues and to confirm to our coding standard.
Anyway, as David said, he's setting up a sandbox on Github. You could commit your code and work on it and a couple of us could help you improve this so you can propose this code to PEAR.
Let me know if you have any Qs!
Till
Robert McLeod [2009-10-16 18:24 UTC] OK cool I'm adjusting some things now... Just a few questions:
3) What is a unit test?
4) Its not whether I want to send headers it whether the user of the script wants to send headers. Sometimes it is handy to show XML in the styled view in firefox. Right?
6) How can KML_Place and KML_Style extend from KML_Common when they are comletely different objects?
9) __toString is awesome!
I'll have to look into cloning and exceptions before I implement them because I don't really understand them...
Cheers for the feedback :)
Till Klampaeckel [2009-10-17 17:26 UTC] Your package is broken. I can't untar etc. it when I download it.
Also, would you mind putting all your code on Github? It makes easier looking through it.
Till
Michael Gauthier [2011-01-30 21:56 UTC] Responding to existing comments:
3.) I don't see an issue with a method that sets the appropriate headers as long as it remains optional and doesn't get called by default.
4.) Wikipedia's entry on unit testing is pretty good. You can also check out some examples of packages with unit tests in pear. For example: Services_Amazon_SQS and HTTP_Request2.
My additional feedback:
a.) Use require_once, not require as specified in the PEAR coding standards. This will prevent fatal errors if code is inadvertantly required from two or more places.
b.) There's a parse error in XML_KML_Style and XML_KML_Place, you need to define class properties outside of functions, not in the constructor.
c.) @access in docblocks is not needed since you're specifying the scope of methods with public/private/protected already.
d.) CDATA escaping, etc can all be done automatically and correctly using the DOM API. (http://ca3.php.net/manual/en/class.domcharacterdata.php) What you have now is vulnerable to XML injection. If you're using simplexml, it may also automatically escape data when required. I have less experience with simplexml so I cannot say for sure that it will escape properly.
e.) Consider a fluent interface for set* methods. In such an interface, your setters return $this. Then in calling code you can do:
$place->setFolder('foo')->setCoords(10,200)->setDesc('My House');
Robert McLeod [2011-09-16 17:38 UTC] I have left this project on github at https://github.com/hamstar/Create_KML
I do not have the time to wrap my head around the package file and strict requirements of a PEAR package so I intend to leave it on github instead of PEAR.
I believe it would also benefit from being on a social coding platform where the code won't go stale as fast as it is easily accessible/forkable by anyone in a single click.
The code is completed to the PEAR coding standards however it does not have tests yet.
I'm not sure what to do from here, whether I should delete this proposal or not. If someone else wants to see this plugin in PEAR then by all means they can complete the package file. All the source is on github.
Thanks for all the support and feedback and help with coding, you guys are great :)
Sorry if I have wasted anyones time here.
|
