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:

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 {
$response = $this->request->send();
} catch (Exception $exc) {
throw new Services_GeoNames_Exception(
if ($response->getStatus() != 200) {
throw new Services_GeoNames_Exception(
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.

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 :)