OHLC chart implementation

I recently failed a Java interview exercise, and I'd like to know where I went wrong. As far as I can tell the code works, the tests pass, and the interviewer didn't give any specific feedback.

The task was to implement the PriceHistorySource interface, to provide data for an OHLC chart. Here is my implementation:

```
import java.math.BigDecimal;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.ConcurrentHashMap;
import java.util.function.Consumer;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class PriceHistorySourceImpl implements PriceHistorySource {

private static class MutableOHLC {

private long _ohlcTimestamp, _openTimestamp, _closeTimestamp;

private BigDecimal _open, _high, _low, _close;

/**
* Must be called before first call to {@link #price(Price)}.
*/
private void reset(long timestamp) {
_ohlcTimestamp = timestamp;
_open = null;
}

private boolean hasValue() {
return null != _open;
}

private void price(Price price) {
BigDecimal mid = price.getMid();
long updateTime = price.getUpdateTime();
if (hasValue()) {
_high = _high.max(mid);
_low = _low.min(mid);
if (updateTime = _closeTimestamp) { // Last one wins.
_close = mid;
_closeTimestamp = updateTime;
}
}
else {
_open = _high = _low = _close = mid;
_openTimestamp = _closeTimestamp = updateTime;
}
}

private void snapshotOrVoid(int instrument, Consumer target) {
if (hasValue()) {
target.accept(new OHLC(instrument, _open, _high, _low, _close, _ohlcTimestamp));
}
}

}

static final String IGNORING_LATE_PRICE_FORMAT = "Ignoring late price: {}";

private class

Here is what I think of your code.


Disclaimer: My thoughts may be completely different to why the potential employer rejected your approach.

Formal weaknesses

Confusing code structure

You put the public method inherited from the interface at the last position in your class file. Usually public methods are placed at the top right behind the constructors or at least right before the methods with lesser visibility the "entry point" uses.

Also you have a "hidden" property (_idToRing) which is defined among the methods instead of before the constructor.
The same applies for IGNORING_LATE_PRICE_FORMAT.

(Unused) Getter

You created a getter for maxIntervalCount which is never used (as far as I see...)

But the major problem with that is that getters violate the information hiding paradigm of OOP and should only be used in DTOs.

Naming conventions (minor)

You prefix class member variables with _ which is explicitly discouraged by the Java naming conventions.

Although I use this naming scheme in my projects too it might not be the best idea to do so in a interview task...

Business solution weaknesses

Complex approach

Your approach is rather complex. Without having deeply thought about the problem it might be solvable with some (nested) sorted collections (e.g. TreeSet).

Separation of concerns

As far as I see there is no requirement to collect the Price objects yourself. It might be a good idea to let the user pass in the list of prices either via constructor or setter.

Bad naming

Classes

I don't know how the interval concept relates to a ring but maybe that's just me...

Methods

-
You added another public method to your (main) class price. It obviously adds another Price object to the statistics. Therefore it should be named addPrice or add.

-
Your method getSnapshot is void. Therefore it should not be named like a getter.

-
Your method ohlc gets a MutableOHLC object from the _ring array. It should have a "getter" name.

On the other hand the interfaces method should also not be named like a getter since it does some effort to calculate the result...

Variables

You use abbreviations in your variable names (relIndex).

Unneeded synchronization

You have some synchronized method which could by simply avoided if you would work on a local copy of the list of prices.

Unit Test

Your unit tests are to complex and do not support the understanding of the code. There are lots of "magic numbers" in them and lots of duplicated code (initializations of price objects).

Most important: good unit tests have method names that tell a story from the business requirements beginning with the one that is easiest to implement getting more and more complex (and should off cause be written right before implementing the tested behavior...).

E.g.:

with_empty_price_list_OHLC_list_is_empty()
single_price_in_list_creates_single_OHLC_list_entry()
two_prices_within_same_interval_result_in_single_OHLC_entry()
two_prices_in_different_intervals_result_in_two_OHLC_entries()
//...
// Yes, test method names may not follow Java nameing conventions, 
// but I do explicitly _not_ say that they have to be written this way...

Also you should have as few asserts in your unit test as possible. So instead of asserting the properties of the result list entries you should rely on the equals implementation.

Maybe there are some more weaknesses but I think there is enough to think about already... ;o)

StackExchange Code Review Q#155343, answer score: 6