4
\$\begingroup\$

Recently I did an exercise for a interview pre-screening, but was not selected. They did not give much detailed feedback apart from "the code was not up to what they wanted".

It would be really helpful to understand as to whether the code written is up to production standards and what is it that I could be missing.

Here are the requirements:

Our business wants to move in the bitcoin market and we’ve been asked to implement the first cut of a simple RFQ (request-for-quote) service. The API looks like this and cannot be changed

public interface RfqService {
    Optional<Quote> quoteFor(String currency, int amount);
}

public class Quote {
    public final double bid;
    public final double ask;
    public Quote(double bid, double ask) {
        this.bid = bid;
        this.ask = ask;
    }
}

The job of our engine is to respond with a bid (the price we buy at) and ask (the price we sell at) by finding an exact client offer to match on each side. To do so, our service has access to the company’s live order board which given a single payment currency (e.g. USD) will return a list of clients’ pending orders.

public interface LiveOrderBoard {
    List<Order> ordersFor(String currency);
}

The response from our service should be a single bid/ask quote. If the request cannot be fulfilled on both sides by our internal RFQ service, the response should be empty.

Bid = highest buy order – X
Ask = lowest sell order + X

The difference between the client order and our quote (i.e. the X amount) is how we make money. X is 0.02 irrespective of the currency.

Worked examples

RFQ for 200 bitcoins paying in USD should return:

Bid (we buy) = 232.69
Ask (we sell) = 232.75

RFQ for 500 bitcoins cannot be fulfilled because there’s no matching Sell order.

The solution

Please provide an implementation of the RFQ engine in Java which will allow the business to enter the bitcoin market. No UI or persistence is required but your solution should be able to demonstrate the RFQ business rules given some sample inputs of your choice. We don’t require an implementation of the live orders board. If you find the requirements unclear, you’re free to make any assumptions along the way but please state them explicitly in a readme file within your solution. It should contain everything you would normally check in to your source code management system, and the code should be of production quality.

package com.trade.service;

import java.math.BigDecimal;
import java.util.List;
import java.util.Optional;
import java.util.logging.Level;
import java.util.logging.Logger;

/**
  * Implements the RFQService interface. Provides a mechanism to get a      quotes for bitcoins
*/
public class RFQServiceEngine implements RfQService {

    private static Logger log = Logger.getLogger("RFQServiceEngine");

    private LiveOrderBoard liveOrderBoard; //reference to the live order board

    private static final BigDecimal commission = new BigDecimal(0.02); // Our commission

    public RFQServiceEngine(LiveOrderBoard liveOrderBoard) {
        // We have a depencency on the live order board
        this.liveOrderBoard = liveOrderBoard;
    }

    /**
     *  Returns a quote
     * @param currency - The type of currency to trade the bitcoins in
     * @param amount - The number of bitcoins needed
     * @return -  A quote containing the ask and bid price if we can exactly match the request, else an empty quote
     */
    public Optional<Quote> quoteFor(String currency, int amount) {
        try {
            // No use going any further if we don't have a liveorder board OR the amount needed is not positive
            if (liveOrderBoard != null && amount > 0) {
                // have the whole code in a try catch block , so if the below conversion to currency goes tits up,
                // we log the error and send a empty quote
                final TradeCurrency cur = TradeCurrency.valueOf(currency);
                List<Order> orders = liveOrderBoard.ordersFor(currency);

                Optional<Order> sell = getLowestSellOrder(amount, cur, orders);

                Optional<Order> buy = getHighestBuyOrder(amount, cur, orders);

                if (buy.isPresent() && sell.isPresent()) {
                    // We deal with price as BigDecimal, but the Quote object needs a double
                    double bid = buy.get().getPrice().subtract(commission).doubleValue();
                    double ask = sell.get().getPrice().add(commission).doubleValue();
                    return Optional.of(new Quote(bid, ask));
                }
            }
            // If we don't find any exact matching Buy or Sell orders
            return Optional.empty();

        } catch (IllegalArgumentException e) {
            //Since the signature of quoteFor does not have a throws exception, we return a empty quote.
            // Logs should be able to highlight that the error occurred
            log.log(Level.SEVERE,"Illegal argument passed to quote for",e);
            return Optional.empty();
        } catch (Exception e) {
            log.log(Level.SEVERE,"Exception occurred in quote for",e);
            return Optional.empty();
        }
    }

    /**
     *  Helper method to get the Highest buy order from the order book
     * @param quantity - No of bitcoins requested
     * @param cur - the currency to trade in
     * @param orders - The list of orders from the live order board
     * @return - the highest buy order for the {cur} and {amount}
     */
    private Optional<Order> getHighestBuyOrder(int quantity, TradeCurrency cur, List<Order> orders) {
        return orders.stream().
                            filter(order -> cur.equals(order.getTradeCurrency()) &&
                                    TradeDirection.BUY.equals(order.getTradeDirection()) &&
                                    quantity == order.getQuantity()).
                            max((p1, p2) -> p1.getPrice().compareTo(p2.getPrice()));
    }



    /**
     *  Helper method to get the lowest sell order from the order book
     * @param quantity - No of bitcoins requested
     * @param cur - the currency to trade in
     * @param orders - The list of orders from the live order board
     * @return - the lowest sell order for the {cur} and {amount}
     */
    private Optional<Order> getLowestSellOrder(int quantity, TradeCurrency cur, List<Order> orders) {
        return orders.stream().
                            filter(order -> cur.equals(order.getTradeCurrency()) &&
                                    TradeDirection.SELL.equals(order.getTradeDirection()) &&
                                    quantity == order.getQuantity()).
                            min((p1, p2) -> p1.getPrice().compareTo(p2.getPrice()));

    }
}
\$\endgroup\$
7
\$\begingroup\$

In these screening situations, you only get a few seconds to make a good first impression. Unfortunately, this line already makes a bad impression:

public class RFQServiceEngine implements RfQService {

The inconsistent capitalization shows a lack of attention to detail. If the problem explicitly states that you have to implement the given RfqService interface exactly, you can't write implements RfQService. And the class should be named RfqServiceEngine.


The LiveOrderBoard should be passed in like this:

private final LiveOrderBoard orders;

public RfqServiceEngine(LiveOrderBoard liveOrderBoard) {
    if (liveOrderBoard == null) {
        throw new NullPointerException();
    }
    this.orders = liveOrderBoard;
}

Marking the LiveOrderBoard as a final field makes it clear that it cannot be swapped out after the constructor. I don't think it makes any sense to allow it to be null, and it makes even less sense to decline all future quotes as a result of it being null. (The last thing a financial firm wants is to lose millions of dollars due to a hidden bug. If something is wrong, just make it fail fast.)

Don't write worthless comments like this — it's a dead giveaway that you are a beginner.

//reference to the live order board

It is probably good that you used BigDecimal to represent money, to avoid floating-point imprecision. You didn't have to do it, since the specified interface has doubles. Personally, if I were given this problem for an interview, I would just go with double, but write a big comment like this in protest:

// FIXME: Money amounts should ideally be represented using BigDecimal,
// since doubles can lead to rounding errors.  Unfortunately, we are
// stuck with doubles since that is what the Quote class uses.
private static final double commission = 0.02;

However, having chosen to use BigDecimal, you screwed it up by writing new BigDecimal(0.02) instead of new BigDecimal("0.02"). The JavaDoc warns you that the BigDecimal(double) constructor is unpredictable, but you should be able to figure out yourself that writing 0.02 already risks losing precision.

Here also, // Our commission is a superfluous comment.


The question simply represented currencies as strings. What is this TradeCurrency class?

final TradeCurrency cur = TradeCurrency.valueOf(currency);

Did you include the TradeCurrency class with your submission? Why did you invent something to overcomplicate a simple problem?


You calculated the bid and ask prices, but did you actually check that the spread yields a positive profit? That may be the logic flaw that doomed your chances with the company — imagine how much money the firm could lose by accidentally making unprofitable trades!

\$\endgroup\$
  • \$\begingroup\$ I would go so far as to say that already the comment before the line mentioned in this answer shows lack of attention to detail - why are the "*" and "*/" lines indented inconsistently? What's up with the spaces in a quotes? And is it a quote or multiple quotes? \$\endgroup\$ – mkrieger1 Jun 1 '16 at 16:55
4
\$\begingroup\$

Comparator usage

In Java 8, you can use Comparator.comparing(Function) to give you a Comparator on the Order.getPrice() method, when you use it as a method reference:

Comparator<Order> BY_PRICE = Comparator.comparing(Order::getPrice);

Code deduplication

Both getHighestBuyOrder and getLowestSellOrder are similar except for one of the predicate conditions and the terminal operation (max(Comparator) vs min(Comparator) respectively).

As such, you can consider the following refactoring:

private static Stream<Order> stream(List<Order> orders, TradeCurrency currency,
            TradeDirection direction, int quantity) {
    // assuming TradeCurrency and TradeDirection are enums
    return orders.stream()
                    .filter(o -> currency == o.getTradeCurrency())
                    .filter(o -> direction == o.getTradeDirection())
                    .filter(o -> quantity == o.getQuantity());
}

private static Optional<Order> getHighestBuyOrder(List<Order> orders,
            TradeCurrency currency, int quantity) {
    return stream(orders, currency, TradeDirection.BUY, quantity).max(BY_PRICE);
}

private static Optional<Order> getLowestSellOrder(List<Order> orders,
            TradeCurrency currency, int quantity) {
    return stream(orders, currency, TradeDirection.SELL, quantity).min(BY_PRICE);
}

Exception handling

Avoid catching on a generic Exception as it can cause confusion in the long run. For starters, it adversely affects logic control the moment you encounter any kind of exception - checked or runtime, minor or major.

It also potentially hides newer errors when you change your code: maybe you need to be concerned about a new checked exception that you can safely resume processing the remaining results, but because of this broad catch Exception condition, you'll be left wondering why you sometimes/always end up with an empty Optional.

Also, the throws ... declaration is for checked exceptions, so you are still able throw runtime exceptions, if it's severe enough.

return early

If you invert your if condition check on liveOrderBoard and amount to return early, you can reduce one level of nesting.

Style

public class RFQServiceEngine implements RfQService

Since RfqService is given in proper camelCasing (for some definition of 'proper'), you may want to stick to that convention and call your class RfqServiceEngine.

I noticed that in your getHighestBuyOrder and getLowestSellOrder methods, the dot (.) is separated from the method call when spanning across lines. I think that's slightly unconventional, unless it already is your convention. :)

Also, you should be using {@code cur} instead of {cur} in your Javadoc for the monospaced font to be used and rendered correctly.

Putting it altogether

The refactored code for quoteFor(String, int) can look something like:

public Optional<Quote> quoteFor(String currency, int amount) {
    if (liveOrderBoard == null) {
        // illustrating plausible runtime exceptions
        throw new IllegalStateException("No live order board configured.");
    }
    if (currency <= 0) {
        return Optional.empty();
    }
    TradeCurrency tradeCurrency;
    try {
        tradeCurrency = TradeCurrency.valueOf(currency);
    } catch (IllegalArgumentException | NullPointerException e) {
        // log something first?
        return Optional.empty();
    }
    List<Order> orders = liveOrderBoard.ordersFor(currency);
    Optional<Order> buy = getHighestBuyOrder(order, tradeCurrency, amount);
    Optional<Order> sell = getLowestSellOrder(orders, tradeCurrency, amount);
    if (buy.isPresent() && sell.isPresent()) {
        double bid = buy.get().getPrice().subtract(commission).doubleValue();
        double ask = sell.get().getPrice().add(commission).doubleValue();
        return Optional.of(new Quote(bid, ask));
    }
    return Optional.empty();
}
\$\endgroup\$
0
\$\begingroup\$

Fail Early

Your class has a dependency on liveOrderBoard. Check it in your constructor and throw an exception if it is null, don't defer the check until you use it. It's better to not let your class be constructed in an inconsistent state than to have to check it for every public method.

Tests

One of the requirements was that you submit everything you would normally check into source control. I would expect to at least see tests that covered the example cases provided (you may have written them, but not added them here for review). Testing styles vary and can be the source of many string opinions so it it worth considering them as much as the 'production' code.

Comments

It is very tempting to over comment code when submitting it for an interview. I would try to avoid it, the code should be as much like your real code as possible. Write your code so that what you are doing and why is as obvious as you can and reserve comments for more complex explanations. I know you save the live order board in your constructor because you have a dependency, you don't need to tell me...

\$\endgroup\$

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.