March 28th, 2020
As a developer, you will spend more time reading than writing code. As Robert C. Martin puts it in Clean Code: A Handbook of Agile Software Craftsmanship:
“Indeed, the ratio of time spent reading versus writing is well over 10 to 1. We are constantly reading old code as part of the effort to write new code. ...[Therefore,] making it easy to read makes it easier to write.”
It's really important to have code that is so well organized that it tells you exactly what it does at first glance.
I will take a piece of javascript code as an example and I will progressively refactor it.
You are a developer for a company that develops a software that handles authors and books. One part of this app takes a list of authors straight from your database and converts it into an upper case string separated by commas (you know, business requirements...)
You take a look at the function in charge of this feature and fall on this piece of code:
export default (authors, category) => {
var authorsArray = []
var authorsString = ""
for (var i in authors) {
if (authors[i].category == category) {
authorsArray.push(authors[i].name.toUpperCase())
}
}
authorsArray = authorsArray.sort(function (a, b) {
if (a < b)
return -1
if (a > b)
return 1
return 0
})
for (var i in authorsArray) {
authorsString = authorsString
+ authorsArray[i]
+ (i + 1 < authorsArray.length ? ", " : "")
}
return authorsString
}
What an atrocious piece of code! Everything about it is utterly unclear. I've got headaches just by reading this!
The sort with a
and b
comparison is a nightmare to read it's probably something the developer has copied in a hurry from Stackoverflow.
The i + 1
condition looks like the developer tried multiple things to make the condition match, and the condition doesn't tell a lot about what it is doing (you can guess with the second part of the ternary that it puts commas in between words except for the last one.)
Variables have shitty names like authorsArray
and authorsString
. Also, they are mutated all the way through the process.
Imagine having an extra requirement and having to modify this function? Headaches ensuing.
As bad as it looks, this piece of code works. It's in production right now on the website and it brings value to the company. It's only problem is that it is unmaintainable. As is, every modification will just add up to the horror until a point where no modification is possible or is very risky (things can break). That's why we will refactor it.
As it works right now, we have to make sure our rewriting will not change the behavior of this function.
To do that, we will write a simple test.
import getAuthorsByCategory from './authors'
describe('testing getAuthorsByCategory', () => {
const exampleAuthorsList = [
{name: "Martin Fowler", category: "programming"},
{name: "Seth Godin", category: "entrepreneurship"},
{name: "Kent Beck", category: "programming"},
{name: "Michael E. Gerber", category: "entrepreneurship"},
]
it('returns a string of alphabetically sorted authors for a category, separated by commas', () => {
expect(getAuthorsByCategory(exampleAuthorsList, "entrepreneurship")).toEqual(
"MICHAEL E. GERBER, SETH GODIN"
)
})
})
I'm using Jest as a test runner. I also use Babel. Babel is a tool that allows you to write clean modern Javascript that will be transpiled to older Javascript so that it can run on most browsers.
See? This test is very short. In 15 lines I can make sure that the process still do what it's supposed to do! A low price to pay for making sure everything will keep working as intended.
I'll do multiple things here:
.sort(...)
by _.sortBy
. Lodash is a Javascript library that provides a lot of functions for working with data structures like Arrays, Objects, etc. Why reinvent the wheel and write something very hard to read when you can just use Lodash? _.sortBy takes a list of objects and sort them by key. That's exactly what we need!for
loops and if
conditions by map
, filter
and reduce
. If you don't know about these functions, I would strongly suggest that you become familiar with them. They are probably available (sometimes with different names) in every programming language.import _ from "lodash"
export default (authors, category) =>
_.sortBy( // sorting the authors alphabetically by name
authors.filter( // filtering the authors by category
author => author.category === category
),
["name"]
).map( // turning the array of object into an array of uppercase strings of the author's name
author => author.name.toUpperCase()
).reduce( // turning the array of names into a string of names separated by commas.
(acc, author, index) => {
const separator = index !== 0 ? ", " : ""
return `${acc}${separator}${author}`
},
""
)
Here is the documentation for map, filter and reduce.
Now that we have a more functional code, let's rewrite it using pipelines. Babel has a plugin called @babel/plugin-proposal-pipeline-operator. Install the plugin and add the following line to your .babelrc
(or equivalent):
{
//...
"plugins": [
//...
["@babel/plugin-proposal-pipeline-operator", { "proposal": "fsharp" }]
]
}
Keep in mind that this plugin is a proposal and should be used carefully in a production environment!
A pipeline works like this:
const myFunction = (data) => //... doing stuff with data
data |> myFunction
// is equivalent to
myFunction(data)
So what's the point of writing code like this and not like we are used to? Well, that makes code more readable when composing multiple functions:
data
|> myFunction1
|> myFunction2
|> myFunction3
// is much more readable than
myfunction1(
myfunction2(
myFunction3(data)
)
)
It's easier to follow the data flow when using pipelines!
Now rewriting the previous code with pipelines:
export default (authors, category) =>
authors
|> authors => authors.filter(author => author.category === category)
|> authors => _.sortBy(authors, ["name"])
|> authors => authors.map(author => author.name.toUpperCase())
|> authors => authors.reduce((acc, author, index) => {
const separator = index !== 0 ? ", " : ""
return `${acc}${separator}${author}`
}, "")
That's good but it could be better. The data flow is easy to read but at first glance, you don't see the big picture of what the exported function does. Let's fix this!
I will extract the functions that are piped into and give them a name. Here is the result:
const filterByCategory = category => authors => authors.filter(author => author.category === category)
const sortByName = authors => _.sortBy(authors, ["name"])
const turnNameToUpperCase = authors => authors.map(author => author.name.toUpperCase())
const createString = authors => authors.reduce((acc, author, index) => {
const separator = index !== 0 ? ", " : ""
return `${acc}${separator}${author}`
}, "")
export const (authors, category) =>
authors
|> filterByCategory(category)
|> sortByName
|> turnNameToUpperCase
|> createString
Here you go! by reading this, the intention is very clear. It can almost be read like a sentence! "Take an authors list, filter it by category, sort them by name, turn their names to upper case, then create the resulting string"
The next developer reading this will know clearly what your intention was just by reading this piece of code!
Now your code is split across multiple functions. Here is another advantage: they are easier to unit test. You can now make assertions on smaller parts of the program if you want to. This is a very powerful technic when applied to a larger program.
Now we have code that is easy to read and a documentation that executes (that's what I like to call tests). Wouldn't it be better if the signature of the function told you a bit more about how to use this function?
That's where Typescript enters the game. You have multiple possibilities for adding Typescript. If you are already using Babel, the babel-preset-typescript. I don't think pipelines are available in Typescript for the moment so you will have to use Babel for this example. I won't go into details here on the different ways to install Typescript.
So I'll add some type definition to the function:
const filterByCategory = category => authors => authors.filter(author => author.category === category)
const sortByName = authors => _.sortBy(authors, ["name"])
const turnNameToUpperCase = authors => authors.map(author => author.name.toUpperCase())
const createString = authors => authors.reduce((acc, author, index) => {
const separator = index !== 0 ? ", " : ""
return `${acc}${separator}${author}`
}, "")
type Author = {
name: String,
category: String
}
export const (authors: Array<Author>, category: String): String =>
authors
|> filterByCategory(category)
|> sortByName
|> turnNameToUpperCase
|> createString
Now, if you are using proper tooling, your IDE should tell you everything you need to know about what parameters are expected AND prevent you to use this function improperly! I think it's really great that we can prevent errors before they happen.
We turned hard to read code into something that almost reads like English. Refactoring to better code is a step that's vastly overlooked in the industry. Management often thinks that the velocity their development team has right now will stay constant for months. That's simply not true. As much as unreadable, unmaintainable code pile up, the next small change you want to do the codebase will take more and more time and you'll take a risk of breaking other features if not the entire project.
So, every once in a while, take some time to rewrite some code. Consider cleaning up after yourself as part of delivering a feature, like cleaning up the kitchen is part of cooking. Your futur coworkers will thank you!
Edit: I've had the occasion to discuss the article with some people and someone proposed a solution using Ramda, a functional programming library for javascript that would have a similar usage to Lodash. Feel free to use whatever solution you prefer!
import R from 'ramda'
const byCategory: (category: Category) => (author: Author) => boolean =
category => R.propEq('category', category)
const getAuthorsByCategory: (category: Category) => (auhtors: Array<Author>) => string =
(authors, category) => R.pipe(
R.filter(filterByCategory(category)),
R.sortBy(R.prop('name')),
R.map(R.prop('name')),
R.map(R.toUpper),
R.join(', ')
)(authors)