[bisq-network/bisq] Secondary sort order in Market Offer Lists (#4168)

[dmos62]

@dmos62 requested changes on this pull request.

NACK

The routine is very complicated, especially the primary comparator, because we're checking inside the comparator (twice, for some reason) for whether we're comparing sell offers or buy offers and then we're reversing order inside it as well. That shouldn't happen in the comparator. The comparator in both sell and buy cases should be `Comparator.comparing(Offer::getPrice).thenComparing(Offer::getAmount)`, with a `.reversed.()` inserted where needed, for example: `Comparator.comparing(Offer::getPrice).reversed().thenComparing(Offer::getAmount)`

The rest of the functionality should be achieved by filtering the stream on whether it's a buy/sell (which is already being done at https://github.com/bisq-network/bisq/blob/f3dc42e2ed1fc2f2560b541a18cd996a7d23b9ed/desktop/src/main/java/bisq/desktop/main/market/offerbook/OfferBookChartViewModel.java#L282 and https://github.com/bisq-network/bisq/blob/f3dc42e2ed1fc2f2560b541a18cd996a7d23b9ed/desktop/src/main/java/bisq/desktop/main/market/offerbook/OfferBookChartViewModel.java#L311); if nulls have to be handled (can there be offers without prices?), that should happen there as well (presuming we're not showing offers without prices).

> @@ -333,7 +333,14 @@ private void updateChartData() {
         buildChartAndTableEntries(allSellOffers, OfferPayload.Direction.SELL, sellData, topSellOfferList);
     }
 
-    private Comparator<Offer> getComparator(boolean reversePrimarySortOrder) {
+    /**
+     * Returns a comparator to be used for Offers. Sorts primarily for price and
+     * secondarily for offer volume.
+     *
+     * @param reversePrimarySortOrder
+     * @return
+     */
+    private Comparator<Offer> getComparatorWithSecondarySortOrder(boolean reversePrimarySortOrder) {

I think this name is preferable: `getOfferComparator(...)`. The problem with `getComparator` was just that it didn't mention the object of comparison.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/4168#pullrequestreview-411669803
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20200514/971f770d/attachment.html>