Mark Wiesemann [2007-04-30 11:55 UTC] Here is my short review (in addition to the comments on the mailing list):
- some CS issues:
* s/Pear_Exception/PEAR_Exception/
* comments for class properties are missing
* if/switch are still wrong (no linebreak before "{")
* no space before the opening parenthesis in "throw new
Services_ProjectHoneyPot_Exception (" please, same for
"function ... ("
- sprintf() isn't really fast and I would consider rewriting
a line like
$ip_query = sprintf('%s.%s', $ip, $this->accesskey);
to
$ip_query = $ip . '.' . $this->accesskey;
- comment for parseReponse() contains "Pear_Error"
itrebal [2007-04-30 13:57 UTC] Minor discrepancy, but the variable $blacklist is poorly named. At first glance I thought it was a literal blacklisted host, which is confusing. I recommend changing it to honeypotHost or something that explains what it actually is. Mark's comment about class properties would also fix this, but properly named variables goes a long way to being easily maintainable.
Martin Jansen [2007-05-06 09:23 UTC] I share Mark's concerns with regard to the Coding Standards. Other than it would probably be handy if query() not only accepted IP addresses but also host names. In the latter case you could call gethostbyname() to figure out the IP address.
Other than that, the package looks nice.
|
