III-2464 Return null when geocoder throws no result exception. #3
Conversation
…side the coordinate cache to avoid calling API for 'wrong' addresses.
src/CachedGeocodingService.php
Outdated
| @@ -40,6 +39,12 @@ public function getCoordinates($address) | |||
| if ($encodedCacheData) { | |||
| $cacheData = json_decode($encodedCacheData, true); | |||
|
|
|||
| // Some addresses have no coordinates, to cache these addresses 'NO_COORDINATES_FOUND' is used as value. | |||
| // When the 'NO_COORDINATES_FOUND' cached value is found null is returned as coordinate. | |||
| if ('NO_COORDINATES_FOUND' === $cacheData) { | |||
There was a problem hiding this comment.
Maybe use a constant for NO_COORDINATES_FOUND?
There was a problem hiding this comment.
In the past I did overuse constants, now I use them not enough anymore. Bottomline, changed to constant.
src/CachedGeocodingService.php
Outdated
| ); | ||
| // Some addresses have no coordinates, to cache these addresses 'NO_COORDINATES_FOUND' is used as value. | ||
| // When null is passed in as the coordinates, then 'NO_COORDINATES_FOUND' is stored as cache value. | ||
| $encodedCacheData = json_encode('NO_COORDINATES_FOUND'); |
There was a problem hiding this comment.
I think it would be slightly better to first fill $cacheData with the value to cache (either NO_COORDINATES_FOUND or the array with lat and long), then afterwards perform the json_encode().
There was a problem hiding this comment.
Indeed beter to only call json_encode once.
| { | ||
| public function __construct( | ||
| GeocoderInterface $geocoder, | ||
| LoggerInterface $logger |
There was a problem hiding this comment.
Could $logger be made optional, and use a NullLogger by default?
There was a problem hiding this comment.
I prefered required logger in the constructor.
tests/CachedGeocodingServiceTest.php
Outdated
| /** | ||
| * @test | ||
| */ | ||
| public function it_handles_null_coordinates_as_no_coordinates_found() |
There was a problem hiding this comment.
The name of this test could be improved,. Maybe something in the vein of it_also_caches_when_no_coordinates_were_found() ?
No description provided.