loading...
Cover image for Another GOTO to avoid

Another GOTO to avoid

qm3ster profile image Mihail Malo ・3 min read

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 though.
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 function 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.

Discussion

pic
Editor guide
Collapse
qm3ster profile image
Mihail Malo Author

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.

  1. That would be value * 2 + 1, not value + 2
# Code blocks break numeration on DEV.to
  1. That's a very different language, and without going into how recursion is memory-performant there and other niceties like pattern matching, the fact it's the only way justifies it already. Coming back to this case, I'd like to note that there's no iteration in the task at hand, yet when we first got rid of the switch case, we got a little runtime recursion.