Michael Gauthier [2010-05-26 16:59 UTC] Great start, here's some feedback:
1.) Package name does not need a PEAR:: prefix in documentation.
2.) "This class acts as interface to generic Realtime Blocking Lists (RBL)" what does that mean in relation to BrowserMob?
3.) In PEAR code, the . operator should be surrounded by spaces like other operators.
4.) Why not pass key, secret to the constructor so they don't have to be specified in all calls?
5.) A link to BrowserMob's available calls in the class docs would be a good addition.
6.) use */ to close docblocks, not **/
7.) use "Signs and Calls" rather than "Sign and Call" in method documentation.
8.) your timestamp could be generated more accurately using microtime. I notice you append three zeros which leads me to believe this should be in microseconds.
9.) I could see this package being called on different servers simultaneously. In that case, you'll want to use the more_entropy flag on uniqid so your nonces have less likelyhood of colliding.
Till Klampaeckel [2010-05-27 00:38 UTC] I second all of Michael's comments. Further more:
1) It wouldn't hurt if you accepted a config/ini file either.
2) The code should probably not be PHP licensed. ;-)
3) I'm not in favor of:
return json_decode(file_get_contents($request_url));
4) Generally, I'd like to see HTTP_Request2 in there.
5) DNSRBL? ;-)
6) One thing I don't understand is why you don't implement the API at all. Like, what good is an API wrapper if I still have to learn the entire API? API wrappers should make it easier.
7) Put your code in a public repo (github, google code, ...). Helps with review.
Ken Guest [2010-05-27 13:40 UTC] It's good and I agree with Michael's and Till's comments, and also:
1) I would like to see better error handling, especially as regards
"return json_decode(file_get_contents($request_url));"
2) There's not much point in providing unit tests that just mark the tests as incomplete
It would also be useful to explain what browsermob is about ;-)
Ken Guest [2010-05-27 13:54 UTC] also, I'd steer clear of using the PHP License and would choose either the LGPL or one of the BSD licenses.
Michael Gauthier [2010-05-27 16:28 UTC] I also agree with the licensing concerns. BSD, MIT or LGPL license should be use instead if possible. The PHP license is really targeted towards PHP's source code and is not great for PHP user-space code.
|
