Vote Details for "Services_GeoNames" by doconnor

» Details
» Comment
I'd love to see you swap the tests over to make use of HTTP_Request2's MockAdapter.

That would make your tests absolutely zoom; and make it easier to isolate network issues from problems with your code.

A quick howto:
http://clockwerx.blogspot.com/2008/11/pear-and-unit-tests-httprequest2.html


Being able to inject an instance of HTTP_Request2 I've created with the constructor would be good, but that's only a minor thing - Services_Geonames::$request is public, so I can do it straight after.


The only other bits:

protected function sendRequest($url)
{
try {
$this->request->setUrl($url);
$response = $this->request->send();
} catch (Exception $exc) {
throw new Services_GeoNames_Exception(
$exc->getMessage(),
$exc->getCode()
);
}
if ($response->getStatus() != 200) {
throw new Services_GeoNames_Exception(
$response->getReasonPhrase(),
$response->getStatus()
);
}
return $response->getBody();
}


1) ... swallowing all exceptions and re-throwing them might be overkill - You lose trace information by doing what you've done there. Consider shifting this to only catch expected HTTP_Request2 exceptions.

2) There's a bunch of other codes which can occur here.
http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html

By discarding the response, I can only catch your Services_GeoNames_Exception and interrogate it for minimal information.
Consider adding a Services_GeoNames_HTTP_Exception::setResponse(HTTP_Request2_Response $response) or similar; and increasing the amount of test coverage you have here (simulated network errors, 404s, and so forth, see the link from before)

So, +1 if you have a think about some of those for the future; but otherwise great work :)