Why Clever Code Is Often a Bad Idea_

Most software developers occasionally feel tempted to write something clever simply because they can. I had exactly that feeling today, and it was a useful reminder that compact code is not always good code.

The JavaScript Array-Matching Problem

The task was straightforward: combine data from two large arrays containing tens of thousands of items. While iterating over one array, we needed to find the matching item in the other and build a new result array.

Consider an array of users, where each user references one or more avatars:

const users = [
  {id: 1, name: 'Joe', avatars: [1, 2, 4]},
  {id: 2, name: 'Jane', avatars: [3]},
  {id: 3, name: 'Roger', avatars: [6, 5]},
  // ...
  {id: 10000, name: 'Richard', avatars: [10000]},
]

The avatar data comes from a separate array:

const userAvatars = [
  {id: 1, avatar: 'img/avatars/joe.png'},
  {id: 2, avatar: 'img/avatars/dog.jpeg'},
  {id: 3, avatar: 'img/avatars/cat.png'},
  {id: 4, avatar: 'img/avatars/empty.png'},
  {id: 5, avatar: 'img/avatars/person.png'},
  {id: 6, avatar: 'img/avatars/r2.png'},
  // ...
  {id: 10000, userId: 10000, avatar: 'img/avatars/richie.bmp'},
]

We want to combine the two data sets into a result like this:

const usersWithAvatars = [
  {name: 'Joe', avatar: 'img/avatars/joe.png'},
  {name: 'Joe', avatar: 'img/avatars/dog.jpeg'},
  {name: 'Jane', avatar: 'img/avatars/cat.png'},
  {name: 'Joe', avatar: 'img/avatars/empty.png'},
  {name: 'Roger', avatar: 'img/avatars/person.png'},
  {name: 'Roger', avatar: 'img/avatars/r2.png'},
  // ...
  {name: 'Richard', avatar: 'img/avatars/richie.bmp'},
]

There are several ways to solve the problem. A practical approach is to build an index in which each avatar ID points to its user. That makes subsequent lookups fast and predictable.

The Clever but Hard-to-Read Approach

A compact implementation might look like this:

const usersByAvatarId = users
  .reduce((memo, user) => user.avatars
    .reduce((memo, avatar) => ({...memo, [avatar]: user}), memo), {})
const result = userAvatars
  .map((userAvatar) => ({name: usersByAvatarId[userAvatar.id].name, avatar: userAvatar.avatar}))

It is concise, but is it easy to understand at a glance? Probably not. Renaming the inner accumulator helps slightly:

const usersByAvatarId = users
  .reduce((memo, user) => user.avatars
    .reduce((outerMemo, avatar) => ({...outerMemo, [avatar]: user}), memo), {})
const result = userAvatars
  .map((userAvatar) => ({name: usersByAvatarId[userAvatar.id].name, avatar: userAvatar.avatar}))

The result is still difficult to read, even for developers familiar with reduce and modern JavaScript syntax. It also introduces a performance problem: {...outerMemo, [avatar]: user} creates a new copy of the accumulator on every iteration, which becomes expensive with large arrays.

A More Readable and Maintainable Solution

A less clever but clearer implementation looks like this:

const usersByAvatarId = users.reduce((memo, user) => {
  user.avatars.forEach((avatar) => memo[avatar] = user)
  return memo
}, {})
const result = userAvatars
  .map((userAvatar) => ({name: usersByAvatarId[userAvatar.id].name, avatar: userAvatar.avatar}))

Using Array.prototype.forEach adds a few lines, but the intent is much easier to follow. It also avoids repeatedly copying the accumulator, making the solution considerably faster for large data sets.

Readability Beats Cleverness

Clever code is rarely worth writing when it makes future maintenance harder. A compact expression can hide both logical and performance problems, and the person struggling with it later may be a colleague or your future self.

Before committing a clever solution, ask whether a simpler version would be easier to understand, test, and change. In most production code, clarity is the better optimization.