Christian Weiske [2009-08-09 13:32 UTC] - You package file contains no package.xml and is not installable via PEAR
- Code comments are in french
- You have to define your own exception and throw that one
- Pleasae comply with the PEAR coding standards. Use PHP_CodeSniffer to check that
- Describe what i2c is - I don't know that. Why would I need your package?
Philippe Jausions [2009-08-09 17:41 UTC] - Agreed with Christian, comments and error messages should be in English (unfortunately for some, this is the de-facto standard language)
- Net_IP2Country would be a better self-explanatory name
- I'd suggest to do 2 separate classes for back end SOAP vs. HTTP. Net_IP2Country_SOAP and Net_IP2Country_HTTP, with a Net_IP2Country::factory($backend) method. Your code will be much cleaner instead of having hooks for methods to use, and that would also allow for extensions (interfacing with a MaxMind's GeoIP local database for instance)
- fopen(URL) may not always be allowed on a system, use HTTP_Request2 package instead
Michael Gauthier [2009-08-10 17:44 UTC] Great start. Christian and Philippe have provided some excellent feedback so far. Here are some suggestions:
1.) Rather than both a SOAP and HTTP implementation, I'd suggest just choosing one. The external API shouldn't care what the internal code is using.
2.) Maybe categorize the package in the Web Services category. This seems like a wrapper for a Web service rather than a networking component. Maybe Services_GeoIP
3.) Consider a factory method for other geolocation providers. Something like Services_ShortURL (http://pear.php.net/pepr/pepr-proposal-show.php?id=598). I think Google also provides a geolocation service.
|
