Vote Details for "Mailman" by gauthierm

» Details
Voter: Michael Gauthier Vote: +1 (conditional) Reviews: Deep source review
» Comment
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.