Chris Bongers

Posted on Jan 14, 2021 • Originally published at daily-dev-tips.com

Refactoring old code

#javascript

As a developer, you write code (duh), but this code might be outdated in one, three, five years.

I think it's our responsibility to maintain this code if the budget allows it.

In this case, the code has been written by my colleague pre ES6 times.
I'm now asked to write an extension on this codebase.

When looking at the old code, I noted there was some legacy code that was using loops and wasn't really efficient with the tools we have nowadays.

Introducing the old code

In the example we are looking at, we have the following datasets.

const data = {
  Angular: 3,
  React: 1,
  Vue: 2,
  Next: 1,
  HTML: 2,
  Other: 3
};
const colors = ['#d17a29', '#da9554', '#e3af7f', '#edcaa9', '#f6e4d4', '#204e77'];

The goal is to get an output like this:

[
  ['Angular', 3, '#d17a29'],
  ['Other', 3, '#204e77'],
  ['Vue', 2, '#e3af7f'],
  ['HTML', 2, '#f6e4d4'],
  ['React', 1, '#da9554'],
  ['Next', 1, '#edcaa9']
];

Looking at the old code, which is the following:

let sortable = [];
let index = 0;

for (let temp in data) {
  sortable.push([temp, data[temp], colors[index] ? colors[index] : '#D3D3D3']);
  index++;
}

sortable.sort(function(a, b) {
  return b[1] - a[1];
});

The person achieved the exact goal, using a loop and manual plus on an index. Perfect solution, but perhaps we can make it even better?

Refactoring the code

Immediately is was thinking of using the Map method to map the data into the desired format.

But we can't use the Map method on an object?

Right, so let's convert this object into an array.

const newOutput = Object.entries(data);

This will give us the following array.

[['Angular', 3], ['React', 1], ['Vue', 2], ['Next', 1], ['HTML', 2], ['Other', 3]];

Wow, that already halfway there, it's not sorted yet, and we are missing the color, but it's a really good start.

Note: With this, we omitted the manual push into a new array.

Now let's try and add the color based on an index.
Start by adding a Map method to the entries.

const newOutput = Object.entries(data).map(item => item);

This will return the exact same as what we had.
But did you know we can add an index to this?

const newOutput = Object.entries(data).map((item, index) => item);

Another cool thing we can do inside this map is to break down the item into separate variables.

const newOutput = Object.entries(data).map(([title, amount], index) => title);

The above example will return:

['Angular', 'React', 'Vue', 'Next', 'HTML', 'Other'];

You might see where this is going now, so instead of returning just the title, we can return an array.

const newOutput = Object.entries(data).map(([title, amount], index) => [
  title,
  amount,
  colors[index] || '#fff'
]);

There we go. We added the color to our output.

[
  ['Angular', 3, '#d17a29'],
  ['React', 1, '#da9554'],
  ['Vue', 2, '#e3af7f'],
  ['Next', 1, '#edcaa9'],
  ['HTML', 2, '#f6e4d4'],
  ['Other', 3, '#204e77']
];

The last thing we need is to have the array sorted based on the number (second variable).

const newOutput = Object.entries(data)
  .map(([title, amount], index) => [title, amount, colors[index] || '#fff'])
  .sort((a, b) => b[1] - a[1]);

Let's check the output now:

[
  ['Angular', 3, '#d17a29'],
  ['Other', 3, '#204e77'],
  ['Vue', 2, '#e3af7f'],
  ['HTML', 2, '#f6e4d4'],
  ['React', 1, '#da9554'],
  ['Next', 1, '#edcaa9']
];

We did it. We refactored the old code to a single line using combined methods.

I hope you see how refactoring code makes sense and how the thinking process works.

Thank you for reading, and let's connect!

Thank you for reading my blog. Feel free to subscribe to my email newsletter and connect on Facebook or Twitter

Top comments (10)

Alex Lohr • Jan 14 '21

Maybe use the spread operator instead:

const newOutput = Object.entries(data)
  .map((item, index) => [...item, colors[index] || '#fff'])
  .sort((a, b) => b[1] - a[1]);

I think it makes more clear that you are not changing the item itself.

Chris Bongers • Jan 14 '21

Hey Alex,

Great addition! 100% makes sense for readability!
Refactor on the refactor 👀

Jan von der Assen • Jan 14 '21 • Edited

I like the way you refactored the old snippet.
However, I think it is often important to consider during refactoring what is being improved. The performance? Testability? Readability?

I have a bad feeling about just refactoring because new methods exist or because the number of LOC is being reduced.
For example, I think the readability is improved by how you refactored the code.

Personally I would go even further and extract the lambda functions. IMO this improves readability, reusability and testability of the considered module. But I'm sure there are plenty of people who don't like it:

const newOutput = makeIterable(data)
  .map(toTriple)
  .sort(byFirstKeyDesc)

function makeIterable(data) { return Object.entries(data) }

function toTriple (pair, index) {
  const defaultColor = '#fff'
  return [ ..pair, colors[index] || defaultColor ]
}

function byFirstKeyDesc (a, b) {
  return b[1] - a[1]
}

Chris Bongers • Jan 14 '21

Hey/Hoi Jan,

The main goal here was readability, I had to look at the old code for a good minute to understand what was happening.

Might be because the new declarative methods are just more of a "naming" thing to me.

Anyhow,
I really like your way, it makes sense to extract the function, might also improve them being re-used by other code, so valid point!

As for the article,
I'm considering renaming it to "Modernizing code" because the refactor is a bit obsolete, other than readability it doesn't change anything.

Jan von der Assen • Jan 14 '21 • Edited

The main goal here was readability

I think you already greatly improved the codebase. And also generally speaking, stressing the importance of refactoring is a very good point to be made in general!

Agreed, that title would nicely show your intent!