Fix issue #20 : add getAvailableCurrencies method for ProviderInterface #21
Conversation
| * | ||
| * @return array | ||
| */ | ||
| public function getSupportedCurrencies(); |
There was a problem hiding this comment.
how about creating a different interface like ProvidesSupportedCurrencies for this?
There was a problem hiding this comment.
Hello ojhaujjwal,
I would say it's a bit of over designing but you're the captain on board captain ;)
I'll do this shortly
There was a problem hiding this comment.
Well thinking about it, putting getSupportedCurrencies in another interface may cause an inconsistency back to the CurrencyConverter class ...
let's consider this :
public function setCacheAdapter(Cache\Adapter\CacheAdapterInterface $cacheAdapter)
{
$this->setCachable(true);
$this->cacheAdapter = $cacheAdapter;
return $this;
} if we check $cacheAdapter is Cache\Adapter\CacheAdapterInterface here, then we can't be sure getSupportedCurrencies is implemented in $cacheAdapter if declaration is not included in CacheAdapterInterface ... see ?
|
we also need to check if the currencies are supported in the main class while calculating the exchange rate. |
are you talking about |
|
Thinking about it, supported currencies also require caching ... I implemented it in the former commit. What do you think about this last commit ? |
|
+1 man. I like the overall idea. I will weigh in my comments. |
| */ | ||
| public function getSupportedCurrencies() | ||
| { | ||
| $result = json_decode($this->httpClient->get(self::FIXER_API_BASEPATH)->getBody(), true); |
There was a problem hiding this comment.
I get forbidden at times while accessing this endpoint.
| /** | ||
| * {@inheritDoc} | ||
| */ | ||
| function supportedCurrenciesCacheExists() |
There was a problem hiding this comment.
please don't omit method visibility
|
|
||
| interface ProvidesSupportedCurrenciesCacheInterface | ||
| { | ||
| const SUPPORTED_CURRENCIES_CACHE_KEY_NAME = 'supported-currencies'; |
|
Apologies for delays. I will write all my comments this weekend. |
No description provided.