Re: Re: [PEPr] +1 for Web Services::Mailman

As for return and throw – consider this code in your "subscribe" method. You say the function returns a bool. I see a false, true or a void (as far as return values are taken into account). I'd dumb it down (because that's what service wrappers are supposed to do) and make it easier to read for developers. E.g. I'm not sure what "<h5>(.+?)</h5>" means. if (preg_match('#<h5>Successfully subscribed:</h5>#i', $html)) { return true; } elseif (preg_match('#<h5>(.+?)</h5>#i', $html, $m)) { throw new Exception(trim(strip_tags($m[1]), ':')); } On Fri, Jan 20, 2012 at 10:24 AM, HM 2K <nfhm2k@gmail.com> wrote: > 1, Fair comment, applied. > 2, The "i" modifier, means ignore case, so HTML/html/hTmL should all work.. > If the string contains <HMTL> for all intents and purposes, it probably is > HTML. It is just a basic check. > 3, It's HTML, not XML. I have good reason to use regex over domdocument, > but I've yet to explore Xpath. I feel a new version coming on... > 4, Fair point. Fixed. > 5, "Package-specific exception class is required" -- can you show me an > example? > 6, This is getting ridiculous. The PHP manual shows returning values unless > its an error so I'm sticking with that... > 7, It already has a package.xml > 8, It has unit tests, although I should implement a few more... [later] > 9, Until we do wide scale testing, we can't begin to predict which versions > work and which don't. Implementing limitations based on a version number is > illogical, instead it should be feature based. A bit like how modernizr > detects what features your browser has rather than relying on the version > sent. > > Thanks for the tips. > > On Wed, Jan 18, 2012 at 10:30 PM, Michael Gauthier <mike@silverorange.com > >wrote: > > > Michael Gauthier (http://pear.php.net/user/gauthierm) has voted +1 on > the > > proposal for Web Services::Mailman. > > > > Proposal information: > > http://pear.php.net/pepr/pepr-proposal-show.php?id=672 > > Vote information: > > > > http://pear.php.net/pepr/pepr-vote-show.php?id=672&handle=gauthierm > > > > This vote is conditional. The condition is: > > > > 1.) Clean up variable and method names. Don't need to shorten request to > > req, it just makes it harder to read. $adminurl should be $adminURL. > > > > 2.) in fetch(), the check to see if it is HTML could be made more robust. > > What is the service changes to using lower case <HTML>? What if the > > response is not HTML but contains the string <HTML>? > > > > 3.) Use DOM+XPath to parse responses rather than regular expressions. > > Using regular expressions to parse XML is questionable. > > > > 4.) in fetch, you throw an exception and then return false. The return > > false will never execute. This antipattern is repeated in other places in > > the package. > > > > > > 5.) Package-specific exception class is required. Right now it's not > > possible to handle errors from this package. > > > > 6.) Your methods should have no return and always throw exceptions rather > > than having to both catch expected exceptions and check for false as a > > return value. > > > > 7.) needs a package.xml > > > > 8.) Needs unit tests. > > > > 9.) Since the code looks heavily dependent on the output of a particular > > mailman version, perhaps there should be some sort of support for > > versioning and/or mailman version detection. > > >