I saw this blog article, Why Swift guard Should Be Avoided, and it got me thinking about things I believe are fallacies but are continued to be talked about as the “right way to program”. I will preface this by saying that I don’t think there is a “right way” to program, but rather, there are trade-offs for particular paths that we chose to go down. Some of these paths provide better fruit than others. However, just as fruits have seasonality to them, some paths might not always produce the best fruit in all circumstances. Context matters. All that said, I still believe that are paths that never produce good fruit, and I think two of those paths are demonstrated in the linked blog article.
So, what are these bad paths (or as I call programming fallacies)?
- Functions should be between 6 and 10 lines 2. Single Responsibility means doing only one thing
The first is an arbitrary way to determine quality and complexity of code that holds no bearing in the actual domain of the problem. It also asserts that shorter code is better than longer code with no real presumption on the complexity of the shorter code. I believe that code clarity is far more important than code length.
The quote presented to defend this was from Robert C. Martin:
The first rule of functions is that they should be small. The second rule of functions is that they should be smaller than that.
Here’s what I believe is a better philosophical position:
Functions should be as small as possible to do there job, but no smaller than that.
The other fallacy is that “single responsibility” means a single action. This leads you down the path of premature refactoring, which I’ll talk about a bit with examples from the post. A function should do a single task, but tasks are generally multi-step transactions. This should be completely obvious because we still need to have the vend() function; without it, we have to duplicate the logic everywhere in which a vend() must take place.
Pre-mature Refactoring
Ok, let’s get into the post a bit. Here’s the Swift code from Apple’s example:
struct Item {
var price: Int
var count: Int
}
enum VendingMachineError: ErrorType {
case InvalidSelection
case InsufficientFunds(coinsNeeded: Int)
case OutOfStock
}
class VendingMachine {
var inventory = [
"Candy Bar": Item(price: 12, count: 7),
"Chips": Item(price: 10, count: 4),
"Pretzels": Item(price: 7, count: 11)
]
var coinsDeposited = 0
func dispense(snack: String) {
print("Dispensing \(snack)")
}
func vend(itemNamed name: String) throws {
guard var item = inventory[name] else {
throw VendingMachineError.InvalidSelection
}
guard item.count > 0 else {
throw VendingMachineError.OutOfStock
}
guard item.price <= coinsDeposited else {
throw VendingMachineError.InsufficientFunds(coinsNeeded: item.price - coinsDeposited)
}
coinsDeposited -= item.price
--item.count
inventory[name] = item
dispense(name)
}
}
And here’s the refactored version from the post:
func vend(itemNamed name: String) throws {
let item = try validatedItemNamed(name)
reduceDepositedCoinsBy(item.price)
removeFromInventory(item, name: name)
dispense(name)
}
private func validatedItemNamed(name: String) throws -> Item {
let item = try itemNamed(name)
try validate(item)
return item
}
private func reduceDepositedCoinsBy(price: Int) {
coinsDeposited -= price
}
private func removeFromInventory(var item: Item, name: String) {
--item.count
inventory[name] = item
}
private func itemNamed(name: String) throws -> Item {
if let item = inventory[name] {
return item
} else {
throw VendingMachineError.InvalidSelection
}
}
private func validate(item: Item) throws {
try validateCount(item.count)
try validatePrice(item.price)
}
private func validateCount(count: Int) throws {
if count == 0 {
throw VendingMachineError.OutOfStock
}
}
private func validatePrice(price: Int) throws {
if coinsDeposited < price {
throw VendingMachineError.InsufficientFunds(coinsNeeded: price - coinsDeposited)
}
}
Let’s break it down:
vend(itemNamed name: String) throws
The author suggests that the refactored version is better. But here’s the first thing to note: the responsibility of the function has not changed; it is still responsible for doing the same thing it did before. So in the first regard, nothing should have changed from an API usage standpoint. This is vital because when we refactor functionality, this is the actual goal: to break about couple functionality that didn’t belong together.
private func validatedItemNamed(name: String) throws -> Item
I actually don’t know what this is supposed to do. In order to follow what is going on, I need to actually read through the code all of the code that it calls. Doing that, I can see that it does the following:
- Ensures the item is in the dictionary 2. That the count of items is not zero 3. That the number of coins deposited is greater than or equal to the price of the item
However, this took four functions and three levels of function calls to achieve. The oversight in this approach is that it is inherently fragile because it makes use of four functions to achieve it’s goal. A change to any single function can have a ripple effect on unintended side-effects.
Example: The vending machine needs a new function, addItem(). It’s purpose is to allow additional items to be added to the vending machine. However, there are some constraints we want to add for new items:
- The name must not be empty 2. The name must be word-capitalized (e.g. Big Candy Bar) 3. The price must be less than 100
I can pretty much guarantee you that the validateItem() function is going to be updated here to add these requirements. So not only are we going to be validating things that we simply don’t care about on ever call to vend(), if there is a data already in the vending machine that doesn’t already meet these requirements, the vend() is going to fail.
This may seem like a contrived example, but it absolutely is not. I’ve had to fix issues like this in real code because of this exact type of refactoring.
reduceDepositedCoinsBy(price: Int)
This function will lead to data corruption. It makes the assumption that validate() has been called. This function, if we’re going to actually have it, should actually do the verification that this is a legal operation. Otherwise, there is absolutely no purpose for it.
removeFromInventory(var item: Item, name: String)
Same comments apply here as well: data corruption!
itemNamed(name: String) throws -> Item
This one is an interesting one. If Swift had throwable subscripts, then this wouldn’t be necessary. However, I think in principle, this one is good, but the implementation is prone to bugs. This is the poster child for the guard statement.
private func itemNamed(name: String) throws -> Item {
guard let item = inventory[name] {
throw VendingMachineError.InvalidSelection
}
return item
}
This is objectively better as it ensures that the only code-path that can exist after the guard is one where the dictionary actually has the item. If also ensures that if the dictionary doesn’t have the item, we error out early.
Summary
Be wary of refactoring to meet arbitrary goals. When that is the purpose of the refactoring, it is very easy to get into a spot that you’ve actually created more complexity and introduced more error paths in your code.
My guiding principle: a function should handle it’s responsibility and only its responsibility. The number of steps for that responsibility is mostly immaterial.
owensd.io 