How I refactor large functions using composition

Before I go into the How, let's start with WHY.

So why would I refactor, and in this case decompose a large function? Each time I stumble upon such a function I have to read through it again and understand it. Then new such function appears, and so on. Not to mention writing tests for such functions - it is you who has to calculate what all needs to be covered by tests, it is you who has to carefully read through and put a modification if needed. And then tests may be ruined, mostly. In my opinion there are following reasons better to do so:

1. Testability - smaller (and pure, if possible) functions are easy to test. They depend only on input and output, unless there are some side effects like reading from database, or branching like if-else and try-catch. But even with branching smaller functions are easy to read and understand.
2. Readability - when you look at the name of each function (you should be descriptive with names and not use comments everywhere), you can easily follow what's going on, and inject extra steps in between or remove unnecessary ones. Or reuse them.
3. Reusability - these functions don't understand the context under which they are being put into and executed. They only care about their inputs.

Let's say we have following, significantly simplified for purposes of this article, function:

const notesRoute = async (ctx, next) => {
  const timeBegin = getTime()
  const { notesCollection } = ctx.dbInstance
  const id = ctx.params.id
  const { updatedAt } = ctx.query
  const messages = ctx.request.body
  const { user } = ctx.auth

  trace('Updating messages /api/notes/:id/messages')

  if (isNil(user)) {
    error('PUT notes: user is not set', ctx.auth)
    throw unauthorized('Not Authorized')
  }

  if (!Array.isArray(messages)) {
    error('PUT notes: empty or malformed body sent', messages)
    throw badRequest(
      'PUT notes: empty or malformed body sent. Expects an array, received: ' +
        JSON.stringify(messages)
    )
  }

  try {
    const note = await notesCollection.findOne({ id })
    if (isEmpty(note) || isNil(note)) {
      error(`Note with id ${id} not found`)
      throw notFound(`Note with id ${id} not found`)
    }

    const isOldVersion = note.meta && note.meta.updatedAt !== updatedAt
    if (isOldVersion) {
      warn('PUT notes: updating old version')
      throw conflict('Updating old version', note)
    }

    const meta = {
      updatedAt: getUpdatedAt(),
      createdBy: getCreatedBy(),
    }
    note.messages = messages.map(item => ({ ...item, meta }))

    const processedMessages = await messagesHandler.processMessages(note)
    ctx.response.status = 200
    ctx.response.type = 'application/json'
    ctx.body = JSON.stringify(processedMessages)
  } catch (e) {
    error(e.message)
    throw boomify(e)
  } finally {
    const endTime = getTimeDuration(startTime)
    log.trace(`Duration: ${endTime}, id:  ${id}`)
    await next()
  }
}

There are several if-else/try-catch branches, and a finally, and variables which are used far later in the code after they are defined. Also, there is a side effect, reading from database, which is not synchronous operation.
I like to use ramda library for anything functional and to compose a functionality (or simply a program) I use compose function.
First thing first, with compose function you can only do synchronous composition, but we need to compose together sync and async functions. For such use case we can use this asyncCompose function:

const asyncCompose = (...functions) => input =>
  functions.reduceRight(
    (chain, func) => chain.then(func),
    Promise.resolve(input)
  )

I wouldn't be bothered to understand it, but what it does is simply .then it all the way. This way we force synchronous functions to be async, i.e. wrap them with a Promise. At the end we have result.then().then.then() and THEN, we await it.
In practise we use it like this:

const createHandler = asyncCompose(func1, asyncFunc2, func3, func4)
const result = await createHandler()

What's important here is that any argument passed can be taken further to the next function in chain. You'll see this in action later on.
What I like to do first is to "spread dependencies". This means to initiate first block of variables needed for chain execution. Notice when you compose functions, first one to be executed goes last as an argument to asyncCompose. (f(g(h(x))) here function h executes first. Pipe is what you need if reverse order is important to you.

Function to initiate dependencies may look like:

const spreadDependencies = ({ ctx, messagesHandler }) => {
  const timeBegin = getTime()
  const { notesCollection } = ctx.dbInstance
  const id = ctx.params.id
  const { updatedAt } = ctx.query
  const messages = ctx.request.body
  const { user } = ctx.auth
  const { auth } = ctx
  trace('Updating messages /api/notes/:id/messages')
  return {
    timeBegin,
    notesCollection,
    id,
    updatedAt,
    messages,
    user,
    auth,
    messagesHandler
  }
}

What's good here is also that now you can easily see what exactly is what your program needs. Notice how I just pass here messagesHandler further - so it's available later. Reason behind this is if you import messagesHandler from './someHandler' and use it directly in your function, it will be hard to test it. Sure there are packages out there to proxy your imports but I find that ugly approach. Here you can you sinon for example and stub your arguments easily, which will be covered later on. Also, trace and any kind of logging we can use directly in function, because in many cases you don't want to test that part. If you'd like to though, you can pass it to dependencies as well and stub it, OR write middleware function like tap which only does some side effect and pass on input arguments further.

Next step would be to check if user is not received in request:

const throwIfNoUser = ({ user, auth, ...props }) => {
  if (isNil(user)) {
    error('PUT notes: user is not set', auth)
    throw unauthorized('Not Authorized')
  }
  return { user, ...props }
}

Notice here how auth is just needed for this function and we won't send it further anymore. If you want to go fully functional you can use for example Ramda's when or ifElse, although many teams are not ready for this, but branching isn't bad for small functions (and all should be!) and is often more readable since we got used to this, mostly.

I think by now you're getting it - next step is to check for integrity of our messages:

const throwIfMessagesAreEmptyOrMalformed = ({ messages, ...props }) => {
  if (!Array.isArray(messages)) {
    error('PUT notes: empty or malformed body sent', messages)
    throw badRequest(
      'PUT notes: empty or malformed body sent. Expects an array, received: ' +
        JSON.stringify(messages)
    )
  }
  return { ...props }
}

I write here long names for reading purposes, they don't have to be (but makes writing comments obsolete too!). You can check here for other falsy values too.

So now how about that big try-catch block? Hint: nothing really. You can continue with composing as if it's not there. Since processMessages is the only thing we don't control, the boomify will be applied to the whole chain (we'll see that later on), or the very framework can deal with it, if possible.

Let's get note from database:

const dbGetNote = async ({ id, notesCollection, ...props }) => {
  const note = await notesCollection.findOne({ id })
  if (isEmpty(note) || isNil(note)) {
    error(`Note with id ${id} not found`)
    throw notFound(`Note with id ${id} not found`)
  }
  return { note, ...props }
}

Here we create a new variable note to pass further. Function spreadDependencies was only a way to initialise something to begin with. Also I was wrong, reading from database is again something we don't have control over - it can...break. Remember createHandler? Wrapping it with try-catch is very simple and solves our gigantic blocks, and with our functions we only want to focus on what we have control over.

Let's continue:

const throwOldVersionConflict = ({ note, updatedAt, ...props }) => {
  const isOldVersion = note.meta && note.meta.updatedAt !== updatedAt
    if (isOldVersion) {
      warn('PUT notes: updating old version')
      throw conflict('Updating old version', note)
    }
  }
  return { note, ...props }
}

Variable updatedAt is here since beginning, created in spreadDependencies function, available in ...props all the time. It was only meant to be used here and now we don't need it anymore, so we just return (pass on) note and other props whatever is there.

Let's do last step of our composition:

const asyncProcessMessages = async ({ note, messages, timeBegin, messagesHandler }) => {
  const meta = {
    updatedAt: getUpdatedAt(),
    createdBy: getCreatedBy(),
  }
  note.messages = messages.map(item => ({ ...item, meta }))

  const processedResult = await messagesHandler.processMessages(note)
  return {
    processedResult,
    timeBegin,
  }
}

Here we don't have any try-catch since processMessages is beyond our control and we're gonna wrap the whole thing with single try-catch to handle unknowns. We don't need ...props either - disregard anything except what's needed for last step - readability - now we see only what we actually need. (You can have specific try-catch here, to throw your own self described error)
For the sake of this article and it's simplicity, I iterated over messages with map, but lenses are far more beautiful.

Modern frameworks and environments typically have route definitions like so: route.get('/myroute/here', controller). In such an environment lets discuss how can we make our composed function a handler for the controller.

What we have so far is composed function of:

const createMessageHandler = asyncCompose(
  asyncProcessMessages,
  throwOldVersionConflict,
  dbGetUser,
  throwIfMessagesAreEmptyOrMalformed,
  throwIfNoUser,
  spreadDependencies
)

Suppose we have on top of our file:

import { messageProcessor } from 'some-processor'

Let's say our route is using PUT on /notes/:id.
Additionally, we want to have control of all our side effects, including the messageProcessor, so we want to pass it as an argument (a dependency) instead of calling it directly.

Let's define our main controller handler function. Usually in modern Node.js frameworks controllers are defined as (contextObject, nextFunction) => {} and here I'm placing HOC (higher order function) beforehand so that we can inject our dependencies to the controller's function context:

const messagesRouteHandler = ({ messageHandler, processor }) => async (ctx, next) => {
  const handlerResult = await messageHandler({ ctx, processor })
  const { processedResult, timeBegin } = handlerResult
  const duration = getDuration(timeBegin)
  trace(`Duration: ${duration}`)
}

And here we define our route. Now we have controller's definition (ctx, next) => {} passed to put handler. Our dependencies createMessageHandler and messageProcessor are now available in controller, most importantly they can be replaced as dummy promises in tests.

router.put(
  '/notes/:id',
  messagesRouteHandler({
    messageHandler: createMessageHandler,
    processor: messageProcessor,
  })
)

Now that handler is set, let's write one integration test and one unit test. Let's write integration test for our route handler, i.e. test which mimics behaviour of our main dependencies: our own messages handler and third party messages processor.

test('messagesRouteHandler', async () => {
  const createProcessor = Promise.resolve({
    processedResult: {},
    timeBegin: getTime(),
  })
  const createMessageHandler = Promise.resolve({})
  const ctx = {
    response: {},
    query: {},
    request: { header: {} },
  }
  const next = sinon.stub().resolves()
  await messagesRouteHandler({
    messageHandler: createMessageHandler,
    processor: createProcessor,
  })(ctx, next)
  sinon.assert.calledOnce(next)
})

This test doesn't have greatest value, but it shows you how you can mock/stub/spy dependencies and is rather a demonstration. One idea can be that you can mock methods of a third party dependency, making your tests inform you if its API changed after upgrading it to newer version for example - tests could expect calls on some methods which don't exist anymore! So now you know that you shouldn't upgrade that specific package or change your app to conform it.

Let's unit test asyncProcessMessages. We only want to know that array of items will contain meta data.

test('asyncProcessMessages', async () => {
  const note = { id: 1, messages: [] }
  const messages = [{ text: 'Test 1' }, { text: 'Test 2' }]
  const messagesHandler = { processMessages: () => Promise.resolve({}) }
  const result = await asyncProcessMessages({ note, messages, messagesHandler }) // don't need timeBegin
  expect(result).toEqual({
    id: 1,
    messages: [{ 
      text: 'Test 1',
      meta: {
        updatedAt: '2020-04-18',
        createdBy: 'Jane Doe',
      }
    }, {
      text: 'Test 2',
      meta: {
        updatedAt: '2020-04-18',
        createdBy: 'Art Vandelay',
      }
    }]
  })
})

See how you can now test only a piece of your functionality. This way tests become easier, if trivial is too much to say. (Let's pretend we know what getUpdatedAt and getCreatedBy return, otherwise we would make them as dependencies, too)

I hope with this article I helped other people in need of better application flow and easier testing. I wrote this article also for me to go back to it in dire times when complexity of monolith strikes me. Feel free to throw tomatoes at my face or give better suggestions - overall idea is let's write better software! Other ideas are more than welcome.