0

I'm trying to sort a list of stock orders based on buy or sell direction.

I tried something like:

orders.stream()
      .sorted(o -> "BUY".equals(o.side) ? 
              comparing(Order::getPrice) : 
              comparing(Order::getPrice).reversed());

I'm seeing below error message below which is not clear to me.

Incompatible types. Required int but 'comparing' was inferred to Comparator: no instance(s) of type variable(s) T, U exist so that Comparator conforms to Integer.

Nikolas Charalambidis
  • 35,162
  • 12
  • 84
  • 155

3 Answers3

3

You might want to use this Comparator<Order> adjusted by the o.side parameter.

static Comparator<Order> buyComparator() {
    return (l, r) -> {
        Comparator<Order> comparator = Comparator.comparing(Order::getPrice);
        comparator = "BUY".equals(l.side) && "BUY".equals(r.side) ?
                comparator : comparator.reversed();
        return comparator.compare(l ,r);
    };
}

Then the usage is fairly simple, you can return a new sorted list through the Stream API or mutate the original one:

  1. Stream API:
List<Order> sortedOrders = orders.stream()
         .sorted(buyComparator())
         .collect(Collectors.toList());
  1. Mutate the original one using Collections.sort(List, Comparator):
Collections.sort(orders, buyComparator());

By the way, there are two mistakes in your code:

  1. There is no Stream::sort method but Stream::sorted.
  2. The Stream itself is not terminated so the pipelines never execute. Such terminal operations are collect, reduce, findFirst etc...

...why it doesn't complain if I use only sorted(comparing(Order::getPrice)) instead of ternary operation?

Because the sorted method expects Comparator<Order> but your lambda expression is not compliant with that.

Firstly, your lambda expression starts with o -> ... which is not correct as long as the method in Comparator is int compare(T o1, T o2) hence the lambda expression should look like (o1, o2).

Secondly, the return type must be int (because of int compare(T o1, T o2)). Using only comparing(Order::getPrice) is okey but as long as you choose lambda expression over a method reference, then the return type is clearly seen:

// this is your lambda expression
BiFunction<Order, Order, Comparator<Order>> biFunction = (l, r) -> 
    "BUY".equals(l.side) && "BUY".equals(r.side) ?
            Comparator.comparing(Order::getPrice) :
            Comparator.comparing(Order::getPrice).reversed();

List<Order> sortedOrders = orders.stream()
            .sorted((l, r) -> biFunction.apply(l, r).compare(l ,r))
            .collect(Collectors.toList());

This is obviously not Comparator<Order>, but can be used. The whole thing can be simplified to the solution I have descibed above.

Nikolas Charalambidis
  • 35,162
  • 12
  • 84
  • 155
  • 1
    Thanks! Nikolas. I edited the sorted bit. Ignored the terminal operation purposefully. I was just interested in the ternary operation :) – Vinayak Dhulipudi Oct 22 '20 at 18:17
  • Nikolas, my actual question is still bothering me why a ternary expression didn't compile in the sorted method of the stream? the sorted method is expecting a Comparator which is what I was passing. why it doesn't complain if I use only sorted(comparing(Order::getPrice)) instead of ternary operation – Vinayak Dhulipudi Oct 22 '20 at 18:27
  • @VinayakDhulipudi See my edit. – Nikolas Charalambidis Oct 22 '20 at 18:39
  • Perfect! that clarified my queries and it worked for me. Upvoted. – Vinayak Dhulipudi Oct 22 '20 at 19:08
  • 2
    These comparators all violate the transitivity requirement. Besides being broken, constructing a new comparator for every comparison may lead to horrible performance. – Holger Oct 23 '20 at 10:30
0

I have an alternative idea: how about partitioning your stream based on the type, and then post-fixing the sort order?

Map<Boolean, List<Order>> filtered = orders.stream()
                   .sorted(Order::getPrice)
                   .collect(Collectors.partitioningBy(o -> "BUY".equals(o.side)));
// #partitioningBy guarantees true/false will be in the map
List<Order> bought = filtered.get(true);
List<Order> sold = filtered.get(false);
Collections.reverse(sold); //since you want sold backwards

The disadvantage is for large orders (hundreds+ of items), this could be slow as you are doing a sort of both bought and sold at once (as opposed to two smaller sorts). You could move the sort into the downstream collector (or even the collected object), but it would be more code:

Map<Boolean, List<Order>> filtered = orders.stream()
                   .collect(Collectors.partitioningBy(o -> "BUY".equals(o.side)));
List<Order> bought = filtered.get(true);
List<Order> sold = filtered.get(false);
bought.sort(Comparator.comparingInt(Order::getPrice));
sold.sort(Comparator.comparingInt(Order::getPrice).reversed());

For smaller inputs, the first would be acceptable. Most importantly, it gets rid of a (somewhat) messy lambda, removing the ternary/lambda mixing.

Edit:

If you're only interested in one of the results, then there's no need to have the logic be in the same stream:

//To fetch the items that were not "BUY"
List<Order> sold = orders.stream()
                .filter(o -> !"BUY".equals(o.side))
                .sorted(Comparator.comparingInt(Order::getPrice).reversed())
                .collect(Collectors.toList());

//or if you're looking for the sold items at this point in code
List<Order> bought = orders.stream()
                .filter(o -> "BUY".equals(o.side))
                .sorted(Comparator.comparingInt(Order::getPrice))
                .collect(Collectors.toList());
Rogue
  • 10,402
  • 4
  • 41
  • 68
0

Here is something to sort a list with to comparison rules based on an attribute, while still maintaining the original order of that attribute.

Suppose we have an Order:

class Order {
    private String side;
    private int price;
    // Getters and setters omitted for brevity
}

And also suppose we have a list with orders:

BUY  4
BUY  5
SELL 1
BUY  8
SELL 6
BUY  3
SELL 9
SELL 2
BUY  7

The following code will order the BUY orders ascending and the SELL orders descending, but the order of BUY and SELL in the original list is maintained. In this example, the first entry will be a BUY order, the second one also a BUY order, the third a SELL order, et cetera. This would be the result:

BUY  3
BUY  4
SELL 9
BUY  5
SELL 6
BUY  7
SELL 2
SELL 1
BUY  8

Here's the code:

// We define a comparator here which returns ascending order if the side is BUY,
// or descending if it is SELL.
final Comparator<Order> comparator = (left, right) -> {
    Comparator<Order> c = Comparator.comparing(Order::getPrice);
    if (Objects.equals(left.getSide(), "SELL")) {
        c = c.reversed();
    }
    return c.compare(left, right);
};

// We stream over the orders, and partition them by their side. We sort the
// resulting lists by their own comparison method.
Map<String, List<Order>> map = orders.stream()
    .collect(Collectors.groupingBy(Order::getSide, toSortedList(comparator)));
Iterator<Order> buyIt = map.get("BUY").iterator();
Iterator<Order> sellIt = map.get("SELL").iterator();

// At last, we stream again over the elements, consuming from both iterators
// based on the value of 'side'
orders.stream()
    .map(order -> Objects.equals(order.getSide(), "BUY") ? buyIt.next() : sellIt.next())
    .forEach(System.out::println);
// Almost the same as Collectors.toList(), but sorts the list by the provided
// comparator.
public static <T> Collector<T, List<T>, List<T>> toSortedList(Comparator<T> comparator) {
    return Collector.of(
        ArrayList::new,
        List::add,
        (left, right) -> {
            left.addAll(right); return left;
        },
        list -> list.stream()
            .sorted(comparator)
            .collect(Collectors.toList())
    );
}
MC Emperor
  • 20,870
  • 14
  • 76
  • 119