Another GOTO to avoid
Tags: practices, refactoring, tips, programming
Published: 2019-02-08 18:47:32

---
title: Another GOTO to avoid
published: true
description: Is your programming structured? The answer may surprise you!
tags: [practices, refactoring, tips, programming]
cover_image: https://thepracticaldev.s3.amazonaws.com/i/kclechgwfo8gppbyywtl.png
---

#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:

```js
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:

```js
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.

```js
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:

```js
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:

```js
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?

{% youtube WljpG7sH3r4 %}

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:

```js
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:

```js
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.