relax finals, introduce interfaces

[jnorthrup]

certain dto's in the code could be easier to adapt to API's if they were interfaces.

every single instance where there is a public final class could have a more functional interface alternative with no impact to the existing running code given refactoring tools that rename X with XImpl and serve the same interface extracted. now that line endings and formatting are automatic, the delta should be clean and obvious.

I'd like to submit Accountinfo, Wallet, Currency, and CurrencyPair as interfaces with the existing Builders that provide the existing public final classes as they exist now (only where applicable-- some classes don't have builders, and some classes do have interfaces, this is confusing also.)

I'd even like to nominate someone else to perform the intellij refactorings, like @timmolter, who has a mind for making things more consistent.

[jnorthrup]

artist's conception of an adapter that's no more stateful than absolutely necessary:

    public interface AccountInfo {
        /**
         * Gets all wallets in this account
         */
        Map<String, Wallet> getWallets();

        /**
         * Gets wallet for accounts which don't use multiple wallets with ids
         */
        Wallet getWallet();

        /**
         * Gets the wallet with a specific id
         */
        Wallet getWallet(String id);

        /**
         * @return The user name
         */
        String getUsername();

        /**
         * Returns the current trading fee
         *
         * @return The trading fee
         */
        BigDecimal getTradingFee();
    }

    public interface Wallet {
        /** A unique identifier for this wallet 
         * @return The wallet id
         */
        String getId();

        /** A descriptive name for this wallet. Defaults to {@link #id} 
         * @return A descriptive name for the wallet
         */
        String getName();

        /**
         * Returns the balance for the specified currency.
         *
         * @param currency a {@link Currency}.
         * @return the balance of the specified currency, or a zero balance if currency not present
         */
        Balance getBalance(Currency currency);
    }

    public interface Balance extends Comparable<org.knowm.xchange.dto.account.Balance> {
        Currency getCurrency();

        /**
         * Returns the total amount of the <code>currency</code> in this balance.
         *
         * @return the total amount.
         */
        BigDecimal getTotal();

        /**
         * Returns the amount of the <code>currency</code> in this balance that is available to trade.
         *
         * @return the amount that is available to trade.
         */
        BigDecimal getAvailable();

        /**
         * Returns the amount of the <code>currency</code> in this balance that may be withdrawn. Equal to
         * <code>available - borrowed</code>.
         *
         * @return the amount that is available to withdraw.
         */
        default BigDecimal getAvailableForWithdrawal() {
            return getAvailable().subtract(getBorrowed());
        }

        /**
         * Returns the frozen amount of the <code>currency</code> in this balance that is locked in
         * trading.
         *
         * @return the amount that is locked in open orders.
         */
        default BigDecimal getFrozen() {
            return BigDecimal.ZERO;
        } 

        /**
         * Returns the borrowed amount of the available <code>currency</code> in this balance that must be
         * repaid.
         *
         * @return the amount that must be repaid.
         */
        default BigDecimal getBorrowed() {
            return BigDecimal.ZERO;
        }

        /**
         * Returns the loaned amount of the total <code>currency</code> in this balance that will be
         * returned.
         *
         * @return that amount that is loaned out.
         */
        default BigDecimal getLoaned() {
            return BigDecimal.ZERO;
        }

        /**
         * Returns the amount of the <code>currency</code> in this balance that is locked in withdrawal
         *
         * @return the amount in withdrawal.
         */
        default BigDecimal getWithdrawing() {
            return BigDecimal.ZERO;
        }

        ;

        /**
         * Returns the amount of the <code>currency</code> in this balance that is locked in deposit
         *
         * @return the amount in deposit.
         */
        default BigDecimal getDepositing() {
            return BigDecimal.ZERO;
        } 
    }

    @NotNull
    public AccountInfo getAccountInfo() {
        try { 
            return new AccountInfo() {
                String apiKey = idexExchange.getExchangeSpecification().getApiKey();
                String userName = apiKey.substring(0, 6) + "…";

                @Override
                public Map<String, Wallet> getWallets() {
                    return Collections.singletonMap(userName, getWallet());
                }

                @Override
                public Wallet getWallet() {
                    Wallet wallet = null;
                    try {
                        wallet = new Wallet() {
                            Map<Currency, Balance> balances =
                                    RestProxyFactory.createProxy(ReturnCompleteBalancesApi.class, idexExchange.getExchangeSpecification().getSslUri()).completeBalances(new CompleteBalancesReq().address(apiKey)).entrySet().stream().map(entry -> new Balance() {
                                        @Override
                                        public int compareTo(@NotNull org.knowm.xchange.dto.account.Balance o) {
                                            return Comparator.comparing(Balance::getCurrency).compare(this, (Balance) o);
                                        }

                                        @Override
                                        public Currency getCurrency() {
                                            return Currency.getInstance(entry.getKey());
                                        }

                                        @Override
                                        public BigDecimal getTotal() {
                                            return getAvailable().add(getFrozen());
                                        }

                                        @Override
                                        public BigDecimal getAvailable() {
                                            return entry.getValue().getAvailable();
                                        }

                                        @Override
                                        public BigDecimal getAvailableForWithdrawal() {
                                            return getAvailable();;
                                        }

                                        @Override
                                        public BigDecimal getFrozen() {
                                            return entry.getValue().getOnOrders();
                                        }
                                    }).collect(Collectors.toMap(Balance::getCurrency, identity()));

                            public Balance getBalance(Currency currency) {
                                return balances.get(currency);
                            }

                            @Override
                            public String getId() {
                                return userName;
                            }

                            @Override
                            public String getName() {
                                return userName;
                            }

                        };
                    } catch (Exception e) {
                        e.printStackTrace();
                    }

                    return wallet;
                }

                @Override
                public Wallet getWallet(String id) {
                    return getWallets().get(id);
                }

                @Override
                public String getUsername() {
                    return userName;
                }

                @Override
                public BigDecimal getTradingFee() {
                    return BigDecimal.ZERO;
                }
            };

        } catch (Exception e) {
            e.printStackTrace();
        }
        return null;
    }

[jnorthrup]

@npomfret isn't this about as close to having public finals as you want ?

[npomfret]

I think if i were to do this project from scratch, yes, I would use interfaces for domain objects as well as the services.

Personally, I'd wrap the raw json returned by the exchange API in the domain level interfaces which would have the extra benefit of not throwing away data as we currently do in places. The downside is that is that the domain classes would serialise very differently, which might not be to some peoples taste.

[jnorthrup]

The modules require serialization. does XChange core describe any existing exchange directly? The changes would happen in xchange core, but preserve the final classes. If the strategy was to create interfaces with "I" prefix instead of renaming to impl I think serialization changes would be minimal. Imho java serializable has very few actual meaningful tools that add value. It tends to enable harmful tools which clash with bytecode engineering and cdi in downstream. gson and jackson shouldn't require Serializable.

So if we refactor interfaces to "ICurrency" pattern does this fly better? Will not alter roundtrip serialization... But will mix Hungarian naming with bean pattern , it is less conventional java style than the "Impl" suffix but less impact to working code.

using "I" prefix could enable core XChange module to utilize protobuffs, towards high frequency transactions inside or outside of XChange again without altering any current serialization code on the finals already named

[npomfret]

My point about serialisation was simply that if we changed the class structure it might not be popular. But making breaking changes is a thing that's good to do sometimes.

[jnorthrup]

in practice, even with 60+ modules, this is not breaking changes, intellij can extract interfaces in either direction to create Impl renaming or to insert a new interface on top of the existing inheritance hierarchy. the final classes can still stay final, but they will no longer be the only option to use.

this is imho, better than just removing the final modifier. i don't recall seeing any xchange-module objects being serialized other than unit tests, which are inside the refactoring scope

[timmolter]

Hi @jnorthrup Can you please provide a deeper explanation of:

certain dto's in the code could be easier to adapt to API's if they were interfaces...every single instance where there is a public final class could have a more functional interface alternative

I don't have any fundamental issues with your PR, in fact I'm happy to see the major clean up and builders across the board. For someone who doesn't understand what the benefits of the refactor to using interfaces is, what becomes easier and more functional?

[jnorthrup]

the number of "Adapter"'s is large in the last mile of the modules.

end-point currency/balance/wallet/ticker/tickerproperties/orderbook etc. are interpretting textual transforms with varying tuples size and ordering and must impedence-match to an arbitrary final class's arity and nullity.

with Interfaces (as long as ser/deserialization is relatively automatic) and jdk8 we can do a little bit more on the fly with streams and lambdas and interfaces and not make discrete XChange-core dto's and not require a matching xchange-module adapter -- we just instantiate an interface on the fly, which embodies the end contract (the interface) and the adapter workload final state pulled right from the stack objects

with jaxrs we get/create interfaces. the cheapest way to match these interfaces is just wrap one interface's state (from json) into a custom interface to perform the contract on the fly....

[jnorthrup]

right now this code is very entrenched across 60+ module unit tests which sometimes eem to test that the code doesn't change rather than testing the whether the results change.

what would help would be continuous testing --

@timmolter i know how you feel about segregating the integration testsz but there's no trivial means in the IDE to exclude these integration tests from "all tests" to run the unit tests continuously when refactorings are made.

I have made my point that the modules make builds and tests take a long time where the modules are generally not doing anything different than any other module. when im running in my own merged modules branch, with degregated integration tests, i do have the benefit of refactoring and continuous compile/test, since these operations are batched and perform on the filesystem ion an optimized one-shot pass.

anyways, my point is that the unit tests require me to do a manual maven build since there is no continuous testing option that lets me do project-wide changes, and so it consumes more time than it needs to under other crcumstances

[jnorthrup]

I moved Balnce over to interface and it had one constructor for each number of non-computed attributes, but what's worse seems to be that computed attributes are not pure functions, but also attributes. this is ussually a good reason to go with interfaces over the long haul versus keeping a single class that grows expcetional case after exceptional case for every different new usecase (module) added.

Balance had mostly short constructors, but its a complex ledger class with multiple rulesets for multiple sets of modules using it slightly differently. I think the safe apprach is to stick with the builder pattern -- one of the constructors, the one with 3 paramters, was doing something unique compared to the other 4+ arity constructors. luckily, this is an internal object that only exists in a few unit tests and is not json imported for most.

[timmolter]

Your integration test issue is an edge case specific to the way you do things, and I don't want to make a major change for the benefit of one at the cost of affecting everyone else.