5
\$\begingroup\$

I recently did a technical test for a well known Bank in 'The City' and as a result got no further. They were also kind enough not to give me any feedback. The test entailed a TDD approach to a simple and open brief. I am going to post up my solution and the brief.

I would be very grateful to receive any comments and insight into how I might have approached the problem differently, any mistakes I made and what I could have done better.

Here is the brief :

Imagine you're working as a programmer for a company called Silver Bars Marketplace and you have just received a new requirement. In it we would like to display to our users how much demand for silver bars there is on the market.

To do this we would like to have a 'Live Order Board', that could provide us with the following functionality:

  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 'Live Order 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 ‘Live Order 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.

Could you please provide us an implementation of the 'Live Order Board'. No database or UI/WEB is needed for this assignment (we're absolutely fine with in memory solution). The only important thing is that you just write it according to your normal standards.

NOTE: if during your implementation you'll find that something could be designed in multiple different ways, just implement the one which seems most reasonable to you and if you could provide a short (once sentence) reasoning why you choose this way and not another one, it would be great.


LiveOrderBoardTest.java

package silverBars;

import org.junit.After;
import org.junit.Before;
import org.junit.Test;

import static org.junit.Assert.assertEquals;
import static silverBars.OrderType.BUY;
import static silverBars.OrderType.SELL;


public class LiveOrderBoardTest {

    private static final double ARBITRARY_ACCEPTABLE_DELTA = 0.00000001;

    private static final Integer CUSTOMER_1 = 1;
    private static final Integer CUSTOMER_2 = 2;
    private static final Integer CUSTOMER_3 = 3;
    private static final Integer CUSTOMER_4 = 4;

    private static final double PRICE_600 = 600;
    private static final double PRICE_500 = 500;
    private static final double PRICE_450 = 450;
    private static final double PRICE_425 = 425;
    private static final double PRICE_560 = 560;

    private static final double QUANTITY_10 = 10;
    private static final double QUANTITY_12_POINT_5 = 12.5;
    private static final double QUANTITY_27_POINT_5 = 27.5;
    private static final double QUANTITY_40 = 40;


    private LiveOrderBoard liveOrderBoard;

    private final Order Customer1_Order1 = new Order(CUSTOMER_1, BUY, PRICE_500, QUANTITY_10);
    private final Order Customer1_Order2 = new Order(CUSTOMER_1, BUY, PRICE_450, QUANTITY_12_POINT_5);
    private final Order Customer2_Order1 = new Order(CUSTOMER_2, BUY, PRICE_425, QUANTITY_27_POINT_5);
    private final Order Customer2_Order2 = new Order(CUSTOMER_2, BUY, PRICE_450, QUANTITY_10);

    private final Order Customer3_Order3 = new Order(CUSTOMER_3, SELL, PRICE_600, QUANTITY_40);
    private final Order Customer4_Order1 = new Order(CUSTOMER_4, SELL, PRICE_560, QUANTITY_40);
    private final Order Customer4_Order2 = new Order(CUSTOMER_4, SELL, PRICE_600, QUANTITY_10);

    @Before
    public void setup() {
        liveOrderBoard = new LiveOrderBoard();
    }

    @After
    public void summarize(){
        liveOrderBoard.summarize();
    }

    @Test
    public void addOneBuyOrder(){

        liveOrderBoard.add(Customer1_Order1);

        assertEquals(expectedValue(Customer1_Order1), liveOrderBoard.totalBuying(), ARBITRARY_ACCEPTABLE_DELTA);
    }

    @Test
    public void addTwoBuyOrdersForTheSameCustomer(){

        liveOrderBoard.add(Customer1_Order1);
        liveOrderBoard.add(Customer1_Order2);

        assertEquals(expectedValue(Customer1_Order1, Customer1_Order2), liveOrderBoard.totalBuying(), ARBITRARY_ACCEPTABLE_DELTA);
    }

    @Test
    public void addTwoBuyOrdersForTwoDifferentCustomers(){

        liveOrderBoard.add(Customer1_Order1);
        liveOrderBoard.add(Customer2_Order1);

        assertEquals(Customer1_Order1.getValue(), liveOrderBoard.totalBuyingForCustomer(Customer1_Order1.getCustomerId()), ARBITRARY_ACCEPTABLE_DELTA);
        assertEquals(Customer2_Order1.getValue(), liveOrderBoard.totalBuyingForCustomer(Customer2_Order1.getCustomerId()), ARBITRARY_ACCEPTABLE_DELTA);
        assertEquals(expectedValue(Customer1_Order1, Customer2_Order1), liveOrderBoard.totalBuying(), ARBITRARY_ACCEPTABLE_DELTA);
    }

    @Test
    public void addOneSellOrder(){

        liveOrderBoard.add(Customer3_Order3);

        assertEquals(expectedValue(Customer3_Order3), liveOrderBoard.totalSelling(), ARBITRARY_ACCEPTABLE_DELTA);
    }

    @Test
    public void addTwoSellOrdersAndTwoBuyOrdersForDifferentCustomers(){

        liveOrderBoard.add(Customer1_Order1);
        liveOrderBoard.add(Customer2_Order1);
        liveOrderBoard.add(Customer3_Order3);
        liveOrderBoard.add(Customer4_Order1);

        assertEquals(expectedValue(Customer1_Order1, Customer2_Order1), liveOrderBoard.totalBuying(), ARBITRARY_ACCEPTABLE_DELTA);
        assertEquals(expectedValue(Customer3_Order3, Customer4_Order1), liveOrderBoard.totalSelling(), ARBITRARY_ACCEPTABLE_DELTA);

        assertEquals(expectedValue(Customer1_Order1), liveOrderBoard.totalBuyingForCustomer(Customer1_Order1.getCustomerId()), ARBITRARY_ACCEPTABLE_DELTA);
        assertEquals(0.0, liveOrderBoard.totalSelling(Customer1_Order1.getCustomerId()), ARBITRARY_ACCEPTABLE_DELTA);

        assertEquals(expectedValue(Customer3_Order3), liveOrderBoard.totalSelling(Customer3_Order3.getCustomerId()), ARBITRARY_ACCEPTABLE_DELTA);
        assertEquals(0.0, liveOrderBoard.totalBuyingForCustomer(Customer3_Order3.getCustomerId()), ARBITRARY_ACCEPTABLE_DELTA);
    }

    @Test
    public void addAndRemoveAnOrder(){

        liveOrderBoard.add(Customer1_Order1);
        assertEquals(expectedValue(Customer1_Order1), liveOrderBoard.totalBuying(), ARBITRARY_ACCEPTABLE_DELTA);
        liveOrderBoard.remove(Customer1_Order1);
        assertEquals(0.0, liveOrderBoard.totalBuying(), ARBITRARY_ACCEPTABLE_DELTA);
    }

    @Test
    public void anOrderCannotBeAddedTwice(){

        liveOrderBoard.add(Customer1_Order1);
        liveOrderBoard.add(Customer1_Order1);

        assertEquals(expectedValue(Customer1_Order1), liveOrderBoard.totalBuying(), ARBITRARY_ACCEPTABLE_DELTA);
    }

    @Test
    public void addFiveOrdersAndRemoveTwo(){

        liveOrderBoard.add(Customer1_Order1);
        liveOrderBoard.add(Customer1_Order2);
        liveOrderBoard.add(Customer2_Order1);
        liveOrderBoard.add(Customer3_Order3);
        liveOrderBoard.add(Customer4_Order1);

        liveOrderBoard.remove(Customer1_Order1);
        liveOrderBoard.remove(Customer4_Order1);

        assertEquals(expectedValue(Customer1_Order2, Customer2_Order1), liveOrderBoard.totalBuying(), ARBITRARY_ACCEPTABLE_DELTA);
        assertEquals(expectedValue(Customer3_Order3), liveOrderBoard.totalSelling(), ARBITRARY_ACCEPTABLE_DELTA);
    }

    @Test
    public void addTwoBuyOrdersAtTheSamePrice(){

        liveOrderBoard.add(Customer1_Order2);
        liveOrderBoard.add(Customer2_Order2);

        assertEquals(expectedQuantity(Customer1_Order2, Customer2_Order2), liveOrderBoard.quantityBuyingAtPrice(Customer1_Order2.getPriceAsInteger()), ARBITRARY_ACCEPTABLE_DELTA);
    }

    @Test
    public void addTwoSellOrdersAtTheSamePrice(){

        liveOrderBoard.add(Customer3_Order3);
        liveOrderBoard.add(Customer4_Order2);

        assertEquals(expectedQuantity(Customer3_Order3, Customer4_Order2), liveOrderBoard.quantitySellingAtPrice(Customer3_Order3.getPriceAsInteger()), ARBITRARY_ACCEPTABLE_DELTA);
    }

    private double expectedQuantity(Order... orders) {
        double total = 0.0;

        for (Order order : orders) {
            total += order.getQuantity();
        }

        return total;
    }

    private double expectedValue(Order... orders) {
        double total = 0.0;

        for (Order order : orders) {
            total += order.getValue();
        }

        return total;
    }
}

Go.java

package silverBars;


import static silverBars.OrderType.BUY;
import static silverBars.OrderType.SELL;

public class Go {


    public static void main(String[] args) {

        LiveOrderBoard liveOrderBoard = new LiveOrderBoard();

        liveOrderBoard.add(new Order(1, BUY, 300, 25.5));
        liveOrderBoard.add(new Order(2, BUY, 300, 62.5));
        liveOrderBoard.add(new Order(3, BUY, 325, 44.0));
        liveOrderBoard.add(new Order(3, BUY, 325, 15.0));

        liveOrderBoard.add(new Order(4, SELL, 500, 20.0));
        liveOrderBoard.add(new Order(5, SELL, 500, 40.0));
        liveOrderBoard.add(new Order(6, SELL, 700, 40.0));
        liveOrderBoard.add(new Order(7, SELL, 700, 40.0));

        liveOrderBoard.summarize();
    }
}

LiveOrderBoard.java

package silverBars;

import java.util.*;

import static java.text.NumberFormat.getCurrencyInstance;
import static silverBars.OrderType.BUY;
import static silverBars.OrderType.SELL;

/*
 * This solution traverses the Map to calculate totals. A real-world solution would probably need to avoid that
 * by keeping track of the overall-totals and totals-per-customer as orders are added and removed.
 */
class LiveOrderBoard {

    private final Map<Integer, List<Order>> orderBoard; // map key is customerId
    private final Map<Integer, Double> quantityBuyingAtPrice; // map key is price
    private final Map<Integer, Double> quantitySellingAtPrice; // map key is price

    /*
     * The summary in the brief shows a list of order ID 'order (a) + order (d)' etc.. against a particular quantity for a price.
     * My interpretation is that we just require a String representation of an OrderId for each Order.
     *
     * I have not implemented it here but I would choose to do it by changing the Double in the above Map to an Object that encapsulates
     * both the total quantity as a Double and a List<Integer> where Integer is an OrderID and the List comprises all the orders making
     * up that total quantity.
     *
     * In that way they can be listed in the summary. The Order class would need to have some unique Integer ID calculated for it.
     * It seems sensible to track the total quantity as Order instances are added. Re-calculating the total (from the associated orders)
     * might be non-performant for large numbers of Order.
     */

    LiveOrderBoard() {

        orderBoard = new HashMap<Integer, List<Order>>();
        quantityBuyingAtPrice = new HashMap<Integer, Double>();
        quantitySellingAtPrice = new HashMap<Integer, Double>();
    }

    void add(Order order) {
        Integer customerId = order.getCustomerId();

        if(!orderBoard.containsKey(customerId)){
            addNewCustomer(customerId);
        }

        List<Order> customerOrders = orderBoard.get(customerId);
        if(!customerOrders.contains(order)) {
            customerOrders.add(order);
            updateQuantityAtPrice(order, tranlsateOrderTypeToMap(order.getOrderType()));
        }
    }

    private Map<Integer, Double> tranlsateOrderTypeToMap(OrderType type) {
        return type.equals(BUY) ? quantityBuyingAtPrice : quantitySellingAtPrice;
    }

    private void updateQuantityAtPrice(Order order, Map<Integer, Double> quantityAtPrice) {
        double totalQuantityAtPrice;

        if(quantityAtPrice.containsKey(order.getPriceAsInteger())){
            totalQuantityAtPrice = quantityAtPrice.get(order.getPriceAsInteger()) + order.getQuantity();
        } else {
            totalQuantityAtPrice = order.getQuantity();
        }

        quantityAtPrice.put(order.getPriceAsInteger(), totalQuantityAtPrice);
    }

    void remove(Order order) {
        List<Order> customerOrders = orderBoard.get(order.getCustomerId());
        customerOrders.remove(order);
    }

    double totalBuying() {
        return totalForOrderType(BUY);
    }

    double totalSelling() {
        return totalForOrderType(SELL);
    }

    double totalBuyingForCustomer(Integer customerId) {
        return totalOrderTypeForCustomer(customerId, BUY);
    }

    double totalSelling(Integer customerId) {
        return totalOrderTypeForCustomer(customerId, SELL);
    }

    double quantityBuyingAtPrice(Integer price) {
        return quantityBuyingAtPrice.get(price);
    }

    double quantitySellingAtPrice(Integer price) {
        return quantitySellingAtPrice.get(price);
    }

    private double totalForOrderType(OrderType type) {
        double total = 0;

        for (List<Order> customerOrders : orderBoard.values()) {
            for (Order order : customerOrders) {
                if (order.typeIs(type)) {
                    total += order.getValue();
                }
            }
        }

        return total;
    }

    private double totalOrderTypeForCustomer(Integer customer, OrderType type) {
        double total = 0;

        for (Order order : orderBoard.get(customer)) {
            if (order.typeIs(type)) {
                total += order.getValue();
            }
        }

        return total;
    }

    private void addNewCustomer(Integer customerId) {
        orderBoard.put(customerId, new ArrayList<Order>());
    }


    void summarize() {
        System.out.println("LiveOrderBoard Summary ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n");
        System.out.println("Buying >>>>>>>>>>>>>>>>>>>>>>>>>>>>>\n");
        summarizeOrderType(BUY);
        System.out.println("\nSelling >>>>>>>>>>>>>>>>>>>>>>>>>>>>\n");
        summarizeOrderType(SELL);
        System.out.println("END\n\n");
    }

    private void summarizeOrderType(OrderType type) {

        Map<Integer, Double> quantityAtPrice = tranlsateOrderTypeToMap(type);

        for (Integer price : quantityAtPrice.keySet()) {
            System.out.println("- " + quantityAtPrice.get(price) + " kg for " + getCurrencyInstance(Locale.UK).format(price) + " // ...");
        }
    }
}

Order.java

package silverBars;


class Order {

    private final Integer customerId;
    private final OrderType type;
    private final double price;
    private final double quantity;

    Order(Integer customerId, OrderType type, double price, double quantity) {

        this.customerId = customerId;
        this.type = type;
        this.price = price;
        this.quantity = quantity;
    }

    Integer getCustomerId() {
        return customerId;
    }

    double getValue() {
        return price * quantity ;
    }

    boolean typeIs(OrderType type) {
        return this.type.equals(type);
    }

    double getQuantity() {
        return quantity;
    }

    // To be used as HashMap key - A Double would not be very performant and the hashing could produce collisions
    Integer getPriceAsInteger() {
        return new Double(price).intValue();
    }

    OrderType getOrderType() {
        return type;
    }
}

OrderType.java

package silverBars;

public enum OrderType {
    SELL, BUY
}

Here is the output from running the Go class:

LiveOrderBoard Summary ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Buying >>>>>>>>>>>>>>>>>>>>>>>>>>>>>

- 59.0 kg for £325.00 // ...
- 88.0 kg for £300.00 // ...

Selling >>>>>>>>>>>>>>>>>>>>>>>>>>>>

- 60.0 kg for £500.00 // ...
- 80.0 kg for £700.00 // ...
END
\$\endgroup\$
1
\$\begingroup\$

Answering the question

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.

I don't see how that is being handled with your HashMap-backed maps. You need to factor that in...

Also, I don't think your order removal works as intended... Your unit test checks out because you are removing the same object, but 'real-world' hardly works like that. Your code will probably be given a new Order object with the same customer ID, order type, price and quantity, and it is expected to removing the existing object matching those values. You need to implement equals() and hashCode() correctly for this to work.

Traversing a map's entries

Instead of iterating on the Map.keySet() and getting the value by Map.get(K), you should simply iterate on Map.entrySet().

Generic type inference

Since Java 7, you no longer need to re-specify the generic type during instantiation:

private final Map<Integer, List<Order>> orderBoard;
// ...

// orderBoard = new HashMap<Integer, List<Order>>();
orderBoard = new HashMap<>();

Constants in test class

While it's generally a good idea to create constants to aid reusability, I think it's a bit overdone in your unit test class. Do you really need PRICE_600 to tell you that's a price of 600?

Handling double values for maps' keys

// To be used as HashMap key - A Double would not be very performant 
// and the hashing could produce collisions
Integer getPriceAsInteger() {
    return new Double(price).intValue();
}

Since the smallest unit you have to deal with is pounds, you may want to consider representing prices as int already so that you do not need this oddly-named method to 'transform' the price into a usable key. Alternatively, if you have to start handling pences, consider working in that base, such that this method returns you the value in pence, e.g. 27550 to represent £275.50.

Integer vs int for Customer ID

You can use int for the customer ID, and let auto-boxing do its work when you want to use it as a key in your orderBoard. Unboxing potentially null values into an int is bad, but boxing from an int is generally OK.

Comments

You have very verbose comments in your LiveBoard, and I think your solution might have been a bit better if you actually incorporate some of those changes. :)

On a related note:

This solution traverses the Map to calculate totals. A real-world solution would probably need to avoid that by keeping track of the overall-totals and totals-per-customer as orders are added and removed.

I suppose by advocating an OOP approach, you'll want your classes and methods to encapsulate your Map-based inner workings so that it's possible to swap them out for a 'real-world' solution, this will likely require the use of interfaces in addition.

\$\endgroup\$
  • \$\begingroup\$ Really appreciate the detailed feedback. Yes I failed to provide the SELL orders in ascending order of price. I guess a TreeMap would have helped there. I also had the language level set to 1.5 in Intellij. Should have been 1.8 and I would have seen the warnings about the unnecessary type parameters specified in the Map declarations . \$\endgroup\$ – Chris Danson Apr 10 '17 at 22:00
  • \$\begingroup\$ I see your point about implementing hashCode() an equals() now. Although creating an instance of Order that will equate to an existing Order just to facilitate its removal feels slightly counter-intuitive. I was reluctant to do it as I couldn't immediately see the need. But it's making sense to me now. Yes, should have used valueSet() instead of keySet() and I agree the constant naming in the tests was unecessary. \$\endgroup\$ – Chris Danson Apr 10 '17 at 22:22
  • \$\begingroup\$ Fair point about using an int for the price or storing the value in pence and the customerId being an int that gets auto-boxed to become a Map key. And the point about encapsulation and interfaces. \$\endgroup\$ – Chris Danson Apr 10 '17 at 22:29
  • 1
    \$\begingroup\$ Very valuable feedback - I will endeavour to do better. Many thanks! \$\endgroup\$ – Chris Danson Apr 10 '17 at 22:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.