Introduced the concept of combinedBalance holding the balance of all accounts #359

2021-03-05T14:47:28-08:00

dazito commented

2021-03-05 14:47:28 -08:00

(Migrated from github.com)

No description provided.

scionaltera (Migrated from github.com) requested changes

2021-03-05 15:37:00 -08:00

src/main/java/com/r307/arbitrader/service/ExchangeService.java

Storing this as a single BigDecimal is going to be difficult to maintain over time. I'd suggest storing a BigDecimal on each Exchange with that exchange's last known balance, then when you need the combined value go through and add them together.

List<BigDecimal> balances = allExchanges.stream().map(exchange -> exchange.getLastKnownBalance()).collect(Collectors.toList());
BigDecimal combinedBalance = BigDecimal.ZERO;

for (BigDecimal balance : balances) {
  combinedBalance = combinedBalance.add(balance);
}

(I'm sure there's a fancy way to add the BigDecimals together as part of the stream operation but it's not coming to me.)

This way each time we query for a balance it simply replaces the old value instead of applying a change to the combined value. It should be less likely to become inaccurate due to rounding errors and stuff like that over time.

And actually it occurs to me as I'm writing this that we already have an ExchangeBalanceCache that would be a perfect home for the "add them all together" method because it should already have all the last known balances for all the exchanges in it.

Storing this as a single `BigDecimal` is going to be difficult to maintain over time. I'd suggest storing a `BigDecimal` on each `Exchange` with that exchange's last known balance, then when you need the combined value go through and add them together. ``` List<BigDecimal> balances = allExchanges.stream().map(exchange -> exchange.getLastKnownBalance()).collect(Collectors.toList()); BigDecimal combinedBalance = BigDecimal.ZERO; for (BigDecimal balance : balances) { combinedBalance = combinedBalance.add(balance); } ``` (I'm sure there's a fancy way to add the BigDecimals together as part of the stream operation but it's not coming to me.) This way each time we query for a balance it simply replaces the old value instead of applying a change to the combined value. It should be less likely to become inaccurate due to rounding errors and stuff like that over time. And actually it occurs to me as I'm writing this that we already have an `ExchangeBalanceCache` that would be a perfect home for the "add them all together" method because it should already have all the last known balances for all the exchanges in it.

src/main/java/com/r307/arbitrader/service/TradingService.java

					
				@ -413,1 +413,4 @@

				        exchangeService.updateCombinedBalance(profit);

				        LOGGER.info("Profit calculation: ${} - ${} = ${} | Current bankroll: ${}",

				            updatedBalance,

If we're going to call the variable combinedBalance the comments and log messages should probably refer to it by that name as well.

If we're going to call the variable `combinedBalance` the comments and log messages should probably refer to it by that name as well.

rubieHess (Migrated from github.com) reviewed

2021-03-21 06:15:35 -07:00

src/main/java/com/r307/arbitrader/service/NotificationServiceImpl.java

					
				@ -102,3 +102,4 @@

				                                          BigDecimal combinedBalance, BigDecimal exitTarget,

				                                          boolean isForceExitPosition, boolean isActivePositionExpired) {

				        final String exitSpreadString = String.format("Exit spread: %s\nExit spread target %s\n", spread.getOut(), exitTarget);

I detect that this code is problematic. According to the Bad practice (BAD_PRACTICE), FS: Format string should use %n rather than n (VA_FORMAT_STRING_USES_NEWLINE).
This format string includes a newline character (\n). In format strings, it is generally preferable to use %n, which will produce the platform-specific line separator.

I detect that this code is problematic. According to the [Bad practice (BAD_PRACTICE)](https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html#bad-practice-bad-practice), [FS: Format string should use %n rather than n (VA_FORMAT_STRING_USES_NEWLINE)](https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html#fs-format-string-should-use-n-rather-than-n-va-format-string-uses-newline). This format string includes a newline character (\n). In format strings, it is generally preferable to use %n, which will produce the platform-specific line separator.