Another GOTO hiding in your code
Or, in clickbait terms:
Is your programming structured? The answer may surprise you!
Just a tidbit I often see people miss, and on more than one occasion caused trouble.
TL;DR: If you start a refactor, you better finish it.
TL;DR 2: The fact that to some people "recursion is its own reward" means it's often "clever code" and as such should be the last resort in production.
TL;DR 3: There aren't spoiler tags on DEV.to, so this one is at the end of the post.
Note: The examples are going to be in JavaScript, for wide accessibility, but this applies to most languages.
A common modern "best practices" refactor is avoiding switch
fallthrough, especially when it's not an alias but some actions are performed before falling through.
We end up going from this:
function theFunction(operation, value) {
switch (type) {
case "bigIncrease":
case "bigIncreaseAlias":
value *= 2
case "smallIncrease":
value += 1
break
default:
throw new TypeError(`Unknown operation ${operation}`)
}
return value
}
To this:
function theFunction(operation, value) {
switch (type) {
case "bigIncrease":
case "bigIncreaseAlias":
value *= 2
value = theFunction("smallIncrease", value)
return value
case "smallIncrease":
value += 1
return value
default:
throw new TypeError(`Unknown operation ${operation}`)
}
This became a recursive function, but at least we don't have the "bad" fallthrough anymore, right?
Another one, that improves testability and especially tooling around determining what functions are unused, and generally finding references, is splitting the switch case up into functions.
const operations = {
bigIncrease: value => theFunction("smallIncrease", value * 2),
bigIncreaseAlias: value => operations.bigIncrease(value),
smallIncrease: value => value + 1
}
function theFunction(operation, value) {
const fn = operations[operation]
if (!fn) throw new TypeError(`Unknown operation ${operation}`)
return fn(value)
}
JK, that alias is better solved differently:
const bigIncrease = value => theFunction("smallIncrease", value * 2)
const operations = {
bigIncrease,
bigIncreaseAlias: bigIncrease,
smallIncrease: value => value + 1
}
function theFunction(operation, value) {
const fn = operations[operation]
if (!fn) throw new TypeError(`Unknown operation ${operation}`)
return fn(value)
}
At this point, the astute reader will be totally like
Whoa, you should totally inline that call instead of having "dynamic dispatch" on a string, dude. What if the string name changes?! This is bad for tooling AND a waste of performance.
The reader will of course be right, and we end up with this:
const bigIncrease = value => operations.smallIncrease(value * 2)
const operations = {
bigIncrease,
bigIncreaseAlias: bigIncrease,
smallIncrease: value => value + 1
}
function theFunction(operation, value) {
const fn = operations[operation]
if (!fn) throw new TypeError(`Unknown operation ${operation}`)
return fn(value)
}
The question
Is this good code?
Are we done here?
Well, in my opinion we aren't.
Consider what would happen if we were to change the definition of the abstractly-named .smallIncrease
function to value => value + 2
!
The effect on the unassuming operation.bigIncreaseAlias
would be disastrous! Everything ruined!
How could this happen? (No, this post isn't about the importance of unit testing.) We were so diligent!
Well, what we did is we relied on the smallIncrease
function. Not for its actual semantic meaning of "what needs to be done in response to the dynamic "smallIncrease"
command", but for its internal implementation details.
What can we do better? How can we avoid a repeat of such tragedy?
Well, for starters, we can avoid obsessively misusing the DRY principle, to avoid cryptic code:
const bigIncrease = value => value * 2 + 1
const operations = {
bigIncrease,
bigIncreaseAlias: bigIncrease,
smallIncrease: value => value + 1
}
function theFunction(operation, value) {
const fn = operations[operation]
if (!fn) throw new TypeError(`Unknown operation ${operation}`)
return fn(value)
}
Look mom, I just totally saved 22 characters! /sarcasm
In real life, the shared functionality may be much longer and more complex. What do?
Simple, use better naming and the Open-closed principle. In other words, write functions that do one thing, functions you will never have to edit, only delete if they become unused.
In our case, that looks like this:
const plusOne = value => value + 1
const bigIncrease = value => plusOne(value * 2)
const operations = {
bigIncrease,
bigIncreaseAlias: bigIncrease,
smallIncrease: plusOne
}
function theFunction(operation, value) {
const fn = operations[operation]
if (!fn) throw new TypeError(`Unknown operation ${operation}`)
return fn(value)
}
If someone edits that to be value + 2
they may be beyond help.
And operations
is now just a declarative mapping of strings to locally available functions, much easier to look over at a glance without long implementations inside.
In posher words, when your code's definitions make an acyclic graph you're gonna have a good time.
Disclaimer: Please be gentle, I've never written anything anywhere before.
As promised:
TL;DR 3: Recursive definition is still recursion.
It should be avoided when it doesn't benefit the mental model.
Top comments (1)
Thank you for reading and your feedback. I am aware it's a very contrived one, but I wanted it to be extremely short as I wanted to copy paste it many times over to show every step.
value * 2 + 1
, notvalue + 2