orderId?.takeIf { it.isNotBlank() }
?.toIntOrNull()
?.let { httpClient.getPartialOrder(it) }
?.toDomainObject()
?.let { dbClient.findFullOrder(it) }
?.takeIf { it.status == Status.PENDING }
?.apply { logger.info("Processing order $id") }
?.let { dbClient.updateStatus(it.id, Status.PROCESSING) }
?.apply { this.total = this.price + billingService.calculateTax(this) }
?.also { order -> emailService.sendConfirmation(order) }
?.let { dbClient.finalizeOrder(it) }
?: logger.error("Order processing skipped or invalid ID")
}
This is a problem. Yes, the code works, it's arguably kotlinic, and it's brief. A strong Kotlin programmer will understand what is occurring at each line. However, it's unintuitive, especially to someone new to Kotlin. Even a senior Kotlin coder might require context to understand each line, especially if the variables and methods aren't named well.
Without Googling, try to remember which scope function returns its lambda result and which returns the original context object? At what point is the variable referred to as it a String, an Int, a FullOrder, or a database-friendly FullOrderDomain? What is even the purpose of cramming all of this logic into just one expression?
Just because you can doesn't mean you should. Let's look at each problem, line by line.
First is the null/empty check at the beginning of the method. Google's AI Overview suggests a manual null check and early termination here:
fun processOrder(orderId: String?) {
if (orderId.isNullOrBlank()) {
logger.error("Order processing skipped: Invalid ID")
return
}
I would shorten that block into something more Kotlinic:
require(!orderId.isNullOrBlank()) { "Order processing skipped: Invalid ID" }
This throws an IllegalArgumentException, appropriate if the null or blank ID represents invalid input. We will assume all exceptions are caught and logged somewhere higher up the call stack.
Since we control the signature, we could even reject nullable input by changing String? to a simple String. In that case we can shorten this validation to:
require(orderId.isNotBlank()) { "Order processing skipped: Invalid ID" }
That works well, but is it okay to return early? We might prefer to put our error message at the bottom of the method, so that other null-returning operations can reuse this validation and log message. That was one advantage of cramming everything into one expression ending in our Elvis operator (:?).
However, I think we can do better. "Invalid ID" isn't a very helpful message for all of the operations in our chain. For example, if the order status isn't PENDING, we probably don't have an invalid ID at all. We probably just don't need to process the order and can instead silently return.
We'll fix these error messages as we go along.
Next, we notice that we transform the String into an Int. Since we never use the String until after it's already converted, we don't need to check whether it's blank. We can simply attempt the conversion:
val orderIdInt = orderId.toIntOrNull()
requireNotNull(orderIdInt) { "Order processing skipped: Invalid ID" }
We could have just as easily used checkNotNull() instead of requireNotNull(), but the latter more accurately communicates that we're validating method input.
Next, we perform a lot of transformations and expensive operations on the resulting objects. That is, we send a network request and a database read. This is a good place to split up the method. It's also a good place to add more specific error messages to make debugging easier. And it's a good time to consider whether all these operations return null upon failure. If they do not, the null-safe calls aren't protecting us from anything. We can remove the null checks.
After some revisions, we could end up with something like this:
val orderIdInt = orderId.toIntOrNull()
requireNotNull(orderIdInt) { "Order processing skipped: Invalid ID" }
// Get the full order from the partial order
val response = httpClient.getPartialOrder(orderIdInt)
if (!response.isSuccessful()) {
throw Exception(response.errorMsg)
}
val order = dbClient.findFullOrder(response.toDomainObject())
if (order.status != Status.PENDING) {
// The order was already processed. No need to log an error.
return
}
val updatedOrder = dbClient.updateStatus(order.id, Status.PROCESSING)
?: throw Exception("Status of order ${order.id} could not be updated")
// Calculate total, email customer, and update DB
updatedOrder.total = updatedOrder.price + billingService.calculateTax(updatedOrder)
emailService.sendConfirmation(updatedOrder)
dbClient.finalizeOrder(updatedOrder)
?: throw Exception("Order ${updatedOrder.id} could not be finalized")
By introducing local variables, early returns, and vertical space between blocks of functionality, the code becomes linear, scannable, and drastically easier to debug. More importantly, each variable now has an obvious type and purpose. orderIdInt is an Int. response is the response from a network request. There is no need to keep track of what it represents as it changes from type to type.
The error handling is also more meaningful. By replacing our catch-all error log at the end of the method, each operation can communicate its own failure.
Just to show what can be done when there are methods that do return null upon error, let's posit that updateStatus() and finalizeOrder() return modified order objects when successful and null when there is an error. This gives us the opportunity to use the Elvis operator. But even here, I don't think the existence of nullable return values warrants writing the entire method as one long expression.
There are, of course, plenty of times that it can be useful to chain many method calls together in one expression. To me those times are when there are few to no object type conversions and the logic is simple and fast. Consider:
?.filterNotNull()
?.filter { it.gender == MALE }
?.map { it.name }
?.sorted()
?: log.error("Student list was null.")
This is fine as a single expression. There is only one type transition (List<Student> to List<String>) and the logic is all very straightforward. Every operation is cheap and operates on the same collection.
We could certainly return early upon finding that students is null, but doing so would make the code more verbose without making it substantially easier to read.
The problem isn't Kotlin's method chaining. The problem is using method chaining to hide complexity. When a chain starts mixing type conversions, network calls, database I/O, conditional logic, and side effects, the chain stops being a useful abstraction and starts becoming an unreadable mess.
Conciseness isn't the same thing as readability. Sometimes the most Kotlinic code is the code that is easiest to read and maintain.
No comments:
Post a Comment