Currency Pair cleanup discussion thread

[timmolter]

Hi all!

Currently there are two XChange API methods one could call to get an exchange's currency pairs:

1. PollingMarketDataService.getExchangeInfo().getPairs()
2. BaseExchangeService.getExchangeSymbols()

This has lead to some confusion. We should probably clean this up and only have one option. To me, it makes more sense that getting currency pairs is general enough to not just be focused in the marketdata service but rather in the BaseExchangeService.

I suggest removing the PollingMarketDataService.getExchangeInfo() and the ExchangeInfo DTO from XChange. Do you agree??

[jpe42]

Agreed!

Also, somewhat related, I never understood the reasoning for BaseExchangeService::verify(CurrencyPair currencyPair). Why not just let the exchange return the error. Is it to make debugging easier? What happens is that it prevents users from using newly added currency pairs to an exchange, forcing them to wait until someone updates XChange and forcing them to always use the latest snapshot release. The good thing is that it is more motivating to keep XChange up to date, but I'm not sure how reasonable that actually is as more exchanges are added.

And how about combining all three polling services into a single service? I think it probably makes the code cleaner to have three different services. But as a user it always felt clunky to me and wish I could just call getPollingService. If you still prefer all three; how about changing to composition instead of extension for the BasePollingService's, this way we could share the same instance (same currency pairs) across all three.

[jheusser]

@timmolter Agreed, that should make it cleaner.

@jamespedwards42 Also agreed, the verify is very unnecessary and is no better than just running in an exception (plus the assertions are very ugly). Combining the services is probably a huge amount of work, not sure if it's justified as the separation is kinda nice and does not cause issues imo.

I have another issue which we could discuss before a bigger refactor, but maybe I'll raise it in a separate issue as this is about CurrencyPair.

[timmolter]

OK, thanks for your feedback, guys. I will get rid of the PollingMarketDataService.getExchangeInfo() and the ExchangeInfo DTO.

I will get rid of the verify calls too. I totally agree with the points you raised and was already thinking along the same lines.

The merging of the services would be lots of work. Whether it's necessary even, I personally don't think so. I do like the separation. I'm open to discussing the pros and cons though.

[timmolter]

Actually, before I go removing stuff, I'll leave this tread open a couple of days to see if anyone has any legitimate objections.

[Achterhoeker]

I didn't know of BaseExchangeService.getExchangeSymbols(). I was using PollingMarketDataService.getExchangeInfo().getPairs(), but on several exchanges this was not implemented and i needed to hardcode it. (bitstamp, bitfinex and cexio) I'm now using the better usable BaseExchangeService.getExchangeSymbols(). :-) Thanks.

[timmolter]

done.

[zholmes1]

The only issue I'm seeing with this is that there is no getBaseService method that I know of. For instance, in my app, some of the exchanges that I support have ExchangeInfo.getPairs() and some do not. The ones that do not I had to manually go in and call for instance KrakenBaseService.getExchangeSymbols().

So removing ExchangeInfo.getPairs() takes away the unified method of getting exchange symbols, and people who are using this library will now need to implement a case for every exchange that their program supports. ie

(if currentExchange == BITSTAMP) { currencyPairs = BitstampBaseService.getExchangeSymbols(); } else if (currentExchange == BTCE) { currencyPairs = BTCEBaseService.getExchangeSymbols(); } else if (currentExchange == KRAKEN) ...

Also: Was BaseService's getExchangeSymbols method getting pairs from the exchange, or was it just returning a hard-coded list of CurrencyPairs? If it's the latter, then aren't we losing the ability to support pairs that the exchange has just begun supporting?

[Achterhoeker]

@shortkeys That was my problem also. But I found a way out. The pollingTradeService can be casted to the BaseExchangeService. ((BaseExchangeService)exchange.getPollingTradeService()).getExchangeSymbols();

[timmolter]

I just refactored the base service classes/interfaces. The casting was very ugly. Actually, everything is much cleaner now, including some other things not related directly to this issue.

[Achterhoeker]

Sorry, this was my fault. A clean start worked ok.

[chorez]

Hey Guys Sorry for pushing this, But I cant find getExchangeSymbols(); in BaseExchangeService

Was that moved? Or is there another method that gives me all currencypairs of an exchange?

[zholmes1]

PollingMarketDataService has it On Oct 11, 2014 12:43 PM, "chorez" notifications@github.com wrote:

Hey Gusy Sorry for pushing this, But I cant find getExchangeSymbols(); in BaseExchangeService

Was that moved? Or is there another method that gives me all currencypairs of an exchange?

— Reply to this email directly or view it on GitHub https://github.com/timmolter/XChange/issues/448#issuecomment-58755834.

[chorez]

thanks