WebService to calculate transaction statistics

Question

Recently got rejected in a code challenge that was a very simple WebService with 3 endpoins:

• POST /transactions to add transactions with an ammount and timestamp.
• GET /statistics to query the statistics of the transactions of the last minute.
• DELETE /transactions.

I just pasted below Service object because the API controller is irrelevant:

data class Transaction(val amount: BigDecimal, val timestamp: Instant)
data class Statistics(val sum: String, val min: String, val max: String, val avg: String, val count: Int)
class TransactionInFutureException(message: String) : Exception(message)
class TransactionBeforeThreshold(message: String) : Exception(message)
const val THRESHOLD_SEC = 60L
const val CLEANER_INTERVAL_MS = 5000L
const val AMOUNT_SCALE = 2
class TransactionsService {
 protected val transactions = ArrayList<Transaction>()
 init {
 thread(start = true) {
 Thread.sleep(CLEANER_INTERVAL_MS)
 synchronized(transactions) {
 val threshold = getCurrentThreshold()
 transactions.removeAll { it.timestamp < threshold }
 }
 }
 }
 fun add(amount: BigDecimal, timestamp: Instant) {
 if (timestamp > Instant.now())
 throw TransactionInFutureException("transaction timestamp cannot be in future")
 else if (timestamp < getCurrentThreshold())
 throw TransactionBeforeThreshold("transaction timestamp cannot be before threshold")
 synchronized(transactions) {
 transactions.add(Transaction(amount, timestamp))
 }
 }
 fun clear() {
 synchronized(transactions) {
 transactions.clear()
 }
 }
 fun statistics(): Statistics {
 synchronized(transactions) {
 val threshold = getCurrentThreshold()
 val liveTransactions = transactions.filter { it.timestamp > threshold }
 val count = liveTransactions.count()
 var sum = BigDecimal(0)
 var max = if (count > 0) liveTransactions.first().amount else BigDecimal(0)
 var min = if (count > 0) liveTransactions.first().amount else BigDecimal(0)
 liveTransactions.forEach {
 sum += it.amount
 max = max.max(it.amount)
 min = min.min(it.amount)
 }
 var avg = BigDecimal(0)
 if (count > 0)
 avg = sum.setScale(AMOUNT_SCALE, BigDecimal.ROUND_HALF_UP).
 divide(BigDecimal(count), BigDecimal.ROUND_HALF_UP)
 return Statistics(format(sum), format(min), format(max), format(avg), count)
 }
 }
 protected fun getCurrentThreshold() = Instant.now().minusSeconds(THRESHOLD_SEC)
 protected fun format(value: BigDecimal) = value.setScale(AMOUNT_SCALE, BigDecimal.ROUND_HALF_UP).toString()
}

The two main points because I got rejected were:

There was no separation of concerns; Most of the logic is in just one class.

We felt there was excessive use of Synchronized blocks - which could be replaced with a synchronized or concurrent collection

Looks like they were expecting me to split the functionality of TransactionService into a kind of StatisticsCalculator class that will do what the statistics() method does.

To be honest, for the requirements given, I felt like this is overengineering from here to mars and I will apply the Yagni principle:

• TransactionsService as it is, is a 50L of code class. It fits in one screen and is easy to follow. There is not cognitive overload reading it.

• There is anything to be gained by splitting the class in two right now? I hardly can tell but for sure you will have to add more complexity to the public API of TransactionService to allow to iterate over the stored transactions from StastisticsCalculator.

• As programmers we make trade-offs all the time, between complexity, maintainability, time to build and so forth. I will state that following the Single-responsability-principle is pushing too much the SOLID envelope and I will just adhere to YAGNI and KISS.

• When complexity arises in the future refactoring and splitting will be my first option.

What do you think?

• pragmatic_programmer: Maybe people who is downvoting can explain why? Thanks
• forsvarir: I haven't voted yet. However, your title should reflect what your code does, not what you want for the review. Something like 'statistical transaction analysis' perhaps. You've also tagged both Java and kotlin, even though the code appears to only be kotlin. Both close votes are asking for additional clarity.
• Simon Forsberg: Your question could be improved by giving a bit of context in the beginning. Right now you throw us right into the code and we're left with wondering things like "What is a transaction?" and "What statistics do you want to return?" The more context you can give us before you show the code, the easier it is to understand your code and the easier for us it will be to help you with better answers.
• Simon Forsberg: What is the purpose of transactions.removeAll { it.timestamp < threshold } inside the thread in the init? What requirement did you have that made you implement this code?
• Simon Forsberg: @pragmatic_programmer As I read it, transactions will only be cleaned up once, 5 seconds after the class is instantiated?

Answer 1

I will have to agree that you do have a bit much synchronized blocks. All of them could go away if you would use for example Collections.synchronizedList around your ArrayList or CopyOnWriteArrayList.

Your fun statistics() could be an alternative constructor to Statistics

A few other Kotlin-related tips:

var max = if (count > 0) liveTransactions.first().amount else BigDecimal(0)

Can be written as liveTransactions.firstOrNull()?.amount ?: BigDecimal(0)

Actually:

val amounts = liveTransactions.map {it.amount}
val max = amounts.fold(BigDecimal(0)) { a, b -> a.max(b) }
val sum = amounts.fold(BigDecimal(0)) { a, b -> a.plus(b) }

I would recommend against the forEach. Sure, you need to loop through the array three times, but right now you're doing one loop and three things in every iteration so effectively it has about the same performance.

---

TransactionInFutureException and TransactionBeforeThreshold are really just different versions of IllegalArgumentException, so I would not add my own exceptions here but just use the existing IllegalArgumentException.

Answer 2

Disclaimer... I have very little experience with Kotlin.

Not really the focus of your question, but synchronized blocks should be only as big as they need to be. Looking at your statistics code, you put the whole thing in the block.:

synchronized(transactions) {
 val threshold = getCurrentThreshold()
 val liveTransactions = transactions.filter { it.timestamp > threshold }

I'm not sure if the threshold needs to be in the block or not, it depends what your goal with it is... however I think it's probably arbitrary so could be outside the block. Putting that to the side, filter returns a copy of the collection. Everything in your transactions collections is immutable, which means that liveTransactions is its own copy of data that won't change. I don't see any reason for anything after this line to still be in the synchronised block. Another call to statistics seems perfectly reasonable in this situation.

The way you initialise max and min seems unnecessarily complicated

var max = if (count > 0) liveTransactions.first().amount else BigDecimal(0)

You might as well be always setting it to 0, since you don't then skip the first item in the list.

I think there's a bit more to separation of concerns than cognitive overload. Part of the benefit is that it becomes easier to test the code that you've written. To test your statistics code at the moment, you need to create the array list, via the add method on your TransactionService, making sure your timestamps are all correct for the current time. If instead of your statistics method, you separate the calculation portion out, something like this:

public inline fun <T> Iterable<T>.buildStatistics(selector: (T) -> BigDecimal): Statistics {
 var sum: BigDecimal = BigDecimal(0)
 var count = 0
 var max = BigDecimal(0)
 var min = BigDecimal(0)
 var avg = BigDecimal(0)
 for (element in this) {
 sum += selector(element)
 max = max.max(selector(element))
 min = min.min(selector(element))
 count++
 }
 if (count > 0)
 avg = sum.setScale(AMOUNT_SCALE, BigDecimal.ROUND_HALF_UP).divide(BigDecimal(count), BigDecimal.ROUND_HALF_UP)
 return Statistics(sum, min, max, avg, count)
}

Which can then be called...

val stats = filteredTransactions.buildStatistics { t -> t.amount}

The calculation code then becomes pretty trivial to test, construct an array list of transactions, then call call the buildStatistics method on it, then see if the calculated statistics are correct.