Can someone code review?

IamtheWalrus

Task: Create a board that shows orders of silver
1) Register an order. Order must contain these fields:
- user id
- order quantity (e.g.: 3.5 kg)
- price per kg (e.g.: £303)
- order type: BUY or SELL

2) Cancel a registered order - this will remove the order from the board
3) Get summary information of live orders (see explanation below)
Imagine we have received the following orders:
- a) SELL: 3.5 kg for £306 [user1]
- b) SELL: 1.2 kg for £310 [user2]
- c) SELL: 1.5 kg for £307 [user3]
- d) SELL: 2.0 kg for £306 [user4]

Our board should provide us the following summary information:
- 5.5 kg for £306 // order a + order d
- 1.5 kg for £307 // order c
- 1.2 kg for £310 // order b

The first thing to note here is that orders for the same price should be merged together (even when they are from different users). In this case it can be seen that order a) and d) were for the same amount (£306) and this is why only their sum (5.5 kg) is displayed (for £306) and not the individual orders (3.5 kg and 2.0 kg).The last thing to note is that for SELL orders the orders with lowest prices are displayed first. Opposite is true for the BUY orders.

public class Order {

    public enum OrderType { BUY, SELL }

    private String userId;
    private Double quantity;
    private BigDecimal pricePerKg;
    private OrderType orderType;
    private boolean isCancelled;

    public Order(String userId, Double quantity, BigDecimal pricePerKg, OrderType orderType) {
        this.userId = userId;
        this.quantity = quantity;
        this.pricePerKg = pricePerKg;
        this.orderType = orderType;
    }

    public String getUserId() {
        return userId;
    }

    public boolean isCancelled() {
        return isCancelled;
    }

    public Double getQuantity() {
        return quantity;
    }

    public BigDecimal getPricePerKg() {
        return pricePerKg;
    }

    public OrderType getOrderType() {
        return orderType;
    }

    public void cancel() {
        isCancelled = true;
    }

    public boolean isValid() {
        return this.getUserId() != null &&
                this.getQuantity() != null &&
                this.getPricePerKg() != null &&
                this.getOrderType() != null;
    }

    public static Order collect(Order order1, Order order2) {
        if(!Objects.equals(order1.getOrderType(), order2.getOrderType())) {
            throw new IllegalStateException("Cannot collect orders of different OrderType");
        }
        if(!Objects.equals(order1.getPricePerKg(), order2.getPricePerKg())) {
            throw new IllegalStateException("Cannot collect order of different price");
        }
        return new Order(order1.getUserId(), order1.getQuantity()+order2.getQuantity(), order1.getPricePerKg(), order1.getOrderType());
    }

    @Override
    public boolean equals(Object o) {
        if (this == o) return true;
        if (o == null || getClass() != o.getClass()) return false;

        Order order = (Order) o;

        if (!userId.equals(order.userId)) return false;
        if (!pricePerKg.equals(order.pricePerKg)) return false;
        if (!quantity.equals(order.quantity)) return false;
        return orderType == order.orderType;
    }

    @Override
    public int hashCode() {
        int result = userId.hashCode();
        result = 31 * result + pricePerKg.hashCode();
        result = 31 * result + quantity.hashCode();
        result = 31 * result + orderType.hashCode();
        return result;
    }
}

public class OrderBoard {

    private final List<Order> orders;

    public OrderBoard() {
        orders = new ArrayList<>();
    }

    public void registerOrder(final Order order) {
        if (order == null || !order.isValid()) {
            throw new IllegalArgumentException("Order must contain userId, pricePerKg, quantity and orderType");
        }
        orders.add(order);
    }

    public void cancelOrder(Order orderToCancel) {
        findOrder(orderToCancel).cancel();
    }

    public int getOrderCount() {
        return orders.size();
    }

    public List<Order> getBuyOrders() {
        return getOrders(BUY, ORDER_BY_PRICE_DESC);
    }

    public List<Order> getSellOrders() {
        return getOrders(SELL, ORDER_BY_PRICE_ASC);
    }

    private List<Order> getOrders(OrderType orderType, Comparator<Order> sortByPricePerKg) {
        return orders.stream()
                .filter(filterByType(orderType))
                .filter(filterOutCancelledOrders())
                .collect(toMap(Order::getPricePerKg, identity(), Order::collect))
                .values()
                .stream()
                .sorted(sortByPricePerKg)
                .collect(toList());
    }

    private Order findOrder(final Order orderToFind) {
        return orders.stream().filter(o -> o.equals(orderToFind)).findFirst().get();
    }
}

public class OrderPredicates {

    public static final Comparator<Order> ORDER_BY_PRICE_ASC = (o1, o2) -> o1.getPricePerKg().compareTo(o2.getPricePerKg());

    public static final Comparator<Order> ORDER_BY_PRICE_DESC = (o1, o2) -> o2.getPricePerKg().compareTo(o1.getPricePerKg());

    public static Predicate<Order> filterByType(Order.OrderType orderType) {
        return (order -> order.getOrderType().equals(orderType));
    }

    public static Predicate<Order> filterOutCancelledOrders() {
        return (o -> !o.isCancelled());
    }
}

donaldtramp

There are much easier ways of going about this...

IamtheWalrus

donaldtramp wrote: »

There are much easier ways of going about this...

It's not a wall I'm building..

donaldtramp

IamtheWalrus wrote: »

It's not a wall I'm building..

By the looks of it, it's a wall of code. Forget about object oriented. You should be looking at a more modern methodology of software mapping.

IamtheWalrus

donaldtramp wrote: »

By the looks of it, it's a wall of code. Forget about object oriented. You should be looking at a more modern methodology of software mapping.

Care to elaborate? I'll accept a tweet.

donaldtramp

IamtheWalrus wrote: »

Care to elaborate? I'll accept a tweet.

Wow, most people wouldn't go as far as to accept a tweet.

Idleater

So, you are looking for comments, I presume the code works, and achieves your assignment goals.

Couple of comments off the top of my head:
* BigDecimal is quite a resource hungry data type -are you sure you need that?
* can you use a simple == for your !Objects.equals checks?
* can you do a if x && x.isValid?
* your checks in the equals method could be condensed into if a && B && c ... like what you have done with the ordertype comparison.
* you have no unit tests
* you have absolutely no comments or documentation - for example your dashcode uses a magic number.

IamtheWalrus

Idleater wrote: »

So, you are looking for comments, I presume the code works, and achieves your assignment goals.

Couple of comments off the top of my head:
* BigDecimal is quite a resource hungry data type -are you sure you need that?
* can you use a simple == for your !Objects.equals checks?
* can you do a if x && x.isValid?
* your checks in the equals method could be condensed into if a && B && c ... like what you have done with the ordertype comparison.
* you have no unit tests
* you have absolutely no comments or documentation - for example your dashcode uses a magic number.

Thanks, and I'll amend.

I'm more interested in the design. I fear it's to rigid and complicated.

Idleater

IamtheWalrus wrote: »

Thanks, and I'll amend.

I'm more interested in the design. I fear it's to rigid and complicated.

It is a bit, but I don't know the context of the exercise - for example is it to specifically leverage stream and predicate. I don't see anything wrong per se

If it is working, and achieves what is required, use it as a baseline (check it in with a v1 tag). What kind of improvements (even theoretical) would you like to do. How easy will it be to achieve them with the current design.
For example new Order types (e.g. Hedge, Future, Short). How would different order attributes be modelled and keep the same display ordering?
Are there other ways to Collect orders? By price instead of quantity?
Is there a way to reduce the business logic in the model? e.g. your two IllegalStateException clauses.

Also, just looking at your two Predicates, are you sure they are working?

IamtheWalrus

Idleater wrote: »

It is a bit, but I don't know the context of the exercise - for example is it to specifically leverage stream and predicate. I don't see anything wrong per se

If it is working, and achieves what is required, use it as a baseline (check it in with a v1 tag). What kind of improvements (even theoretical) would you like to do. How easy will it be to achieve them with the current design.
For example new Order types (e.g. Hedge, Future, Short). How would different order attributes be modelled and keep the same display ordering?
Are there other ways to Collect orders? By price instead of quantity?
Is there a way to reduce the business logic in the model? e.g. your two IllegalStateException clauses.

Also, just looking at your two Predicates, are you sure they are working?

Thought so.

Idleater

IamtheWalrus wrote: »

Thought so.

I haven't played with java in a few years but from https://docs.oracle.com/javase/7/docs/api/java/util/Comparator.html and my understanding, a compare to b is 0 -1 or +1. Both of yours do
o1.getPricePerKg().compareTo(o2.getPricePerKg()); (And vice versa) but are called asc and desc.

donaldtramp

I'd recommend using a library.

Giblet

Your hashCode seems to be encoding a lot of state, as well as being ripe for an overflow issue (unless it supports unchecked, I know C# needs an explicit keyword). Are you comparing an in-memory state or a transactional state? ie: Would you ever be comparing the object stored in a cache with an updated version or could you instead guarantee that what you read is up to date within a transaction? You could just use an id for the comparison in the latter.

Do not use object.equals for this when a method could to the same thing and be more explicit.

Steve

Also, it's bad practice to declare null variables. You should initialise them in the declarations.