[PEPr] Comment on XML::XML_pQuery
| From: | Michael Gauthier | Date: | Wed, 25 Sep 2013 19:58:05 +0000 |
| Subject: | [PEPr] Comment on XML::XML_pQuery | ||
| References: | 1 | Groups: | php.pear.dev |
| Request: | Send a blank email to pear-dev+get-55281@lists.php.net to get a copy of this message | ||
1.) This appears to have the same functionality as
https://github.com/symfony/CssSelector. Your
API is a bit higher level but
functionally they do much the same thing. The symfony package is pear
installable from http://pear.symfony.com/
If you're going ahead, then the package should have "CSS Selector" in the
name as that's what you're doing.
2.) The PHP_CodeStandards tool can help you make the code PEAR-compliant.
3.) constructor1, constructor2 and constructor3 methods should have better
names.
4.) The load() method seems out of the scope of this package. At most, this
package should accept a filename or URL and just use the default PHP
commands to read the file content. Ideally I'd like to see that removed and
you are just required to pass a DOMDocument.
Somewhat related, isURL() won't work for all URLs.
5.) Recreating the DOM API seems crazy. Why not just return DOMNode where
necessary rather than re-implementing add/remove/insert/etc?
6.) I'm not sure if __invoke() provides any benefit here. Code like
$xquery = new XQuery(...);
echo $xquery('my-selector')->text();
isn't easier to read than just calling $xquery->select()
7.) Use type-hinting in method signatures where possible. For example in
CSS classes.
8.) A lot of your member variables are private when they should be
protected to allow subclassing.
--
http://pear.php.net/pepr/pepr-proposal-show.php?id=698