Unify wrapper query code into single method

[Sam Griffiths]

each method that interacts with the api uses the same code block. This can be transferred to a single method to avoid duplication of code.
Allcoin and Bitspark have already been updated. The remaining wrappers need attention

[Desrever Nu]

Careful, need attention here. This is critical and needs review.

While somewhere we have code duplicates that should not be there, this falls into a different category.

Moreover, If we have to do this kind of change, we should always start from TradeInterface.java .

As you can see, there are four query methods defined, with different signatures. I decided for this strict design that can bring some duplication but also allow flexibility without type-safe compromises.

    public String query(String url, HashMap<String, String> args, boolean isGet);
    public String query(String base, String method, HashMap<String, String> args, boolean isGet);
    public String query(String url, TreeMap<String, String> args, boolean isGet);
    public String query(String base, String method, TreeMap<String, String> args, boolean isGet);

This reflect the following needs :

1) Some API entrypoint require that the post parameters are alphabetically ordered, some doesn't matter, other requires a specific order

TreeMap and HashMap are identical, with the only difference that TreeMap automatically orders its elements alphabetically while HashMap maintains the order in which elements are added.

2) Some API entrypoint accept the method in the url , while others declare it as a post parameter

This is why sometimes we need to separate the parameter, sometime we don't.

---

This design choice was not well documented, but its knowledge hidden inside the javadoc and the back of my head :)

Now, knowing the design choice, you can take action and either revert changes or re-design the TradeInterface in a different way .

If you want to go with the latter, a way to do it, is declaring in the interface a single query method that accept any parameters (a list of Object) . That will increase the flexibility but make it chaotic for the developer.

Let me know, this is an important change that will affect the code everywhere.

[Sam Griffiths]

I have deliberately left the tradeInterface alone and it isn't in the number of query() methods that I felt there was duplication. I agree with the design decision to include them all as then, with one interface, we have the flexibility to implement every api we have come across so far.

The effort that I have made has been to create a new method called getQuery() in each wrapper. This method accepts the same parameters that would be passed to the implemented query() method. It handles passing those parameters to query() and then detects three errors in the returned data. 1) Null return - this occurs when there is an exception in the executeQuery() method in the service interface. 2) No connection - as the name suggests 3) ApiError - this allows for api errors to be passed to the calling method

By having this getQuery() method, that error checking code only exists once whereas it was present in each method that interfaces with the api. That meant that the same code was present 5, maybe 6, times in each wrapper.

getQuery() returns an ApiResponse object so in the calling method (getAvailableBalances, getTrades etc.), a successful api call can be measured with the isPositive() method and action taken accordingly.

The upshot is that I don't feel that the TradeInterface needs any alteration. If it did, I would only suggest adding in the getQuery() method. The problem with that is that there are different implementations across the wrappers (HashMap vs. TreeMap etc.) so we would need to implement four versions of getQuery() to match each of the query() methods. (That may be Java best practice? If you feel it necessary, I'll implement it)

[Desrever Nu]

The effort that I have made has been to create a new method called getQuery() in each wrapper.

Oh! That totally changes things! In this case you are correct, no need to change the TradeInterface, as query() method is already exposed.

Thanks for the clarification, I hope I did not wasted your time by doing bla bla about the design of the interface :)

I will take a look at the new method in action soon!

[Sam Griffiths]

no problem at all. It was good to have a chance to fully explain what the changes were. I've found the updated method good so far, especially when combined with the new error handling. I don't know if that is because it is easier or if it's just because I am more familiar with the code now. It would be good to get your perspective.

[Sam Griffiths]

This has been completed and has been transferred to the 0.1.4-RC2 release as it should solve an issue with the BitSpark wrapper

[Sam Griffiths]

There have been issues with the release candidates on 0.1.4 but none have been to do with the refactoring of the getQuery method.
if anything, the debugging has been easier because of it.