Nya

Posted on Oct 10, 2019 • Edited on Oct 17, 2019

The Art of Refactoring: 5 tips to Write Better Code

#javascript #codenewbie #beginners #webdev

Bad code works. We all know this. Developers have been writing code for years without giving a single thought to whether they were doing it right or not. It's understandable, isn't it? After all, we already have to deal with the stress of keeping up with the industry and the demands of our job…

The answer is no. Writing bad code comes at a price. Have you ever faced the issue of not understanding your own code after a couple of weeks, and having to spend hours, or even days figuring out what was going on?

The solution to this (extremely) common problem is to make your code as clear and informative as possible. I will go as far as to say that your code should be understood even by a non-technical person. It's time to drop our excuses, and improve the quality of your code.

Writing clean code isn't that complicated. This tutorial will show you 5 simple techniques to improve your code, with practical examples:

Get rid of switch statements
Make your conditionals descriptive
Use guard clauses to avoid nested if statements
Avoid code duplication
Functions should only do one thing

Get rid of switch statements

We normally use switch statements to avoid large if else if statements. However, switch statements are very verbose, hard to maintain and even harder to debug. They clutter up our code, and, in my humble opinion, have an odd, uncomfortable syntax. When adding more cases, we have to manually add each case and break statement, which is quite error-prone.

Let's take a look at an example of a switch statement:

Imagine that we need to add more cases to the switch statement. The amount of code that we would have to write is considerable. We would probably end up copy-pasting code, and we all know how that ends.

So, how do we avoid switch statements? By using an object literal. Object literals are simple, easy to write, read and maintain. We are all used to handling objects in JavaScript, and the syntax is a lot fresher than that of the switch statement. Here is an example:

As you can see, we can add a default value by using the || operator. If the type isn't found in the pokemon object, the getPokemon function will return 'Mew' as a default value.

Note: As you will probably have noticed, we declare the pokemon object outside the function, instead of inside. We do this to prevent it from being created each time we execute the function.

We can also use a map to achieve the same result. A map is a collection of key value pairs, just like an object. The difference is that map allows keys of any type, while objects only allow strings as keys. Also, map has an interesting series of properties and methods. You can read more about the map structure here.

Here's how to use map:

As you can see, our code looks a lot cleaner and straightforward when replacing switch statements with an object literal or map.

Make your conditionals descriptive

Conditional statements are an absolute necessity when writing code. However, they can quickly get out of hand, and end up being impossible to understand. This leads to either having to write comments explaining what the statement does, or having to spend valuable time tracing back our own code to understand what's going on. This is bad.

Take a look at the following statement:

If we only look at the code inside the if statement in the previous function, it's difficult to understand what's going on. Our code isn't clear, and unclear code leads only to technical debt, bugs, and significant headaches.

How can we improve our conditional? By extracting it into a function. Here's how:

By extracting the conditional into a function with a descriptive name: isGameLost(), our checkGameStatus function is now understandable at a glance. Why? Because our code is informative, it tells us what is going on, which is what we should always strive for.

Use guard clauses to avoid nested if statements

Nested if statements are one of the worst things we can encounter in code. I've seen nested ifs 10 levels deep… Believe me when I tell you that it was an absolute nightmare to be able to fully grasp what was going on in that code. Here's an example of a nested if statement (only three levels deep though, I'm not a monster):

You probably have to take a couple of minutes, and read up and down to follow the flow of the function. Nested if statements are hard to both read and understand. So, how do we get rid of the nasty nested if statement? By reversing the logic and using what we call a guard clause.

In computer programming, a guard is a boolean expression that must evaluate to true if the program execution is to continue in the branch in question. - Wikipedia

By reversing the logic of our function, and placing the conditions that cause an early exit in the beginning of the function, they will act as guards, and only allow our function to continue executing if all the conditions are met. This way, we can avoid else statements. Here's how to refactor our previous function to use guard clauses:

As you can see, the code is a lot cleaner and easier to understand. We can see what the function does simply by reading down, following the natural flow of the function, unlike before, where we had to read up and down.

Avoid code duplication

Duplicating code always ends badly. It leads to situations such as: "I fixed this bug here, but forgot to do it there" or "I need to make a change/add a new feature, and have to do it in five different places".
Just as the DRY (don't repeat yourself) principle states:

Every piece of knowledge or logic must have a single, unambiguous representation within a system.

Therefore, having less code is good: It saves us both time and effort, is easier to maintain, and reduces the chances of bugs appearing.

So, how do we get rid of duplicated code? The answer is not always simple, but extracting logic to functions/variables usually works just fine. Let's take a look at the following code, which I ran across when refactoring an application:

You'll probably have noticed that the for loop is exactly the same in both functions, except for one little detail: the type of news that we want, which is either JavaScript or Rust news. To avoid this duplication, we can extract the for loop into a function, which we then call from the getJavascriptNews, getRustNews and getGolangNews functions. Here's how:

After extracting the for loop into the getNewsContent function, our getJavascriptNews, getRustNews and getGolangNews functions have turned into simple, clear one liners.

Further refactoring

However, have you realized that, once again, both functions are exactly the same except for the type string that we pass into the getNewsContent function? This is something that usually happens when we refactor our code. More often than not, one change leads to another change, and so on, until our refactored code ends up being half the size of the original. Let your code tell you what it needs:

Further refactoringWhere did our getJavascriptNews, getRustNews and getGolangNews functions go? We substituted them for a getNews function, which receives the type of news as an argument. This way, no matter how many more types of news we add, we always use the same function. This is called abstraction, and allows us to reuse functions, thus being incredibly useful. Abstraction is one of the techniques I use most frequently in my code.

Bonus: Make the for loop more readable with ES6 features

This is the last refactoring, I swear.
For loops aren't precisely readable. With the introduction of ES6 Array functions, we can avoid using them 95% of the time. In our case, we can use Array.filter combined with Array.map to substitute the original loop:

With Array.filter we return only the elements whose type equals the type passed as an argument.
With Array.map, we return only the content property of the item object, instead of the whole item.

Congratulations, after three simple refactorings, our initial three functions have been reduced to two, which are much easier to understand and maintain. Also, by using abstraction, we made the getNews function reusable.

Functions should only do one thing

Functions should only do one thing, and one thing only. Functions that do more than one thing are the root of all evil, and one of the worst things we can encounter in code (together with nested ifs). They are messy, and make our code hard to understand. Here's an example of a complex function from a real application:

Note: Since the handlers for the event listeners were not needed for this example, I chose to remove them.

As you can see, it's confusing, and hard to understand what's going on in there. If any bugs come up, it will be quite difficult to find and fix them. How can we improve our startProgram function? By extracting common logic into functions. Here's how:

Let's go through the changes made to the startProgram function:

First, we got rid of the if else statement by using a guard clause. Then, we extracted the logic needed to start the database into an initDatabase function and the logic to add event listeners to a setListeners function.

The logic for printing the employee list is slightly more complex, so we created three functions: printEmployeeList, formatEmployeeList, and getEmployeeList.

The getEmployeeList is responsible for making a GET request to employeeList.json, and returning the response in json format.

It is then called by the printEmployeeList function, which takes the list of employees, and passes it to the formatEmployeeList function, which formats and returns it. Then, the list is printed.

As you can see, every function is responsible for doing only one thing.

We could still make a few more changes to the function, and honestly, the application is begging for the separation of the view from the controller, but on the whole, our startProgram function is now more informative, and there is absolutely no difficulty in understanding what it does. We would have no problem at all if we had to come back to this code after a couple of months.

Conclusion

Programmers are the only ones responsible for writing good, quality code. We should all make it a habit to write good code from the very first line. Writing clean code isn't complicated, and doing so will help both you and your colleagues.

By applying the 5 simple techniques shown in this tutorial, your code quality should improve considerably, and so will your productivity.

If you have any questions, don't hesitate to ask. Thank you for reading.

Top comments (16)

Daniel Worsnup • Oct 10 '19

Great article! An important thing to keep in mind when using objects as lookup tables is that objects have prototype chains that will also be checked if you are not careful. Look at this example, using some code from the article:

There are a few fixes for this:

Create the object without a prototype (Object.create(null)):

const pokemon = Object.assign(Object.create(null), {
    Water: 'Squirtle',
    Fire: 'Charmander',
    Plant: 'Bulbasur',
    Electric: 'Pikachu'
});
// pokemon.toString does not exist!

Only check the object's "own" properties (hasOwnProperty)

function getPokemon(type) {
  return pokemon.hasOwnProperty(type) ? pokemon[type] : 'Mew';
}

As you also suggested: Use a Map which inherently doesn't have this problem.

Thanks for the content!

oshell • Oct 10 '19

The example with the switch statement sounds cool first, but is far from any real world use case. Yes, if all I do with the switch is checking a string I can move it to an object. But normally I check against boolean values, returned from several function calls. Maybe you should mention that.

In my opinion the most important thing is to have readable statements. Instead of putting multiple values into the if statement, define a new variable describing what this statement contains. You sacrifice minimal resources for much more readability. No comments needed. E.g.:

const pictureIsValidForUpload = validWidth && validExtension && validMimeType;
If (pictureIsValidForUpload) { upload();}

Robert Larsson • Oct 12 '19

I have a better suggestion on the news part

function getNewsByType(type) {
  const allNews = getNewsFromWeb();
  return allNews.filter(news => news.type === type);
}

getNewsByType('javascript');
getNewsByType('rust');
getNewsByType('golang');

Magne • Dec 8 '20

you forgot the .map part, so it returns the news content.

Patrick Tingen • Oct 11 '19

Note that there is some debate wether you should be DRY (dont repeat yourself) or WET (write everything twice). Blindy pushing code to functions the second time you encounter it may not always be the best solution.

In addition: when you /do/ move it to one function, make really sure that the intention behind it is the same as well. You might end up changing the one function, only to discover that the change is only valid for one of the cases where it is called.

Denis Howe • Oct 11 '19

Can you point to any convincing argument in favour of WET?

My most convincing example in favour of DRY was fixing a bug then discovering that that code had been duplicated, fixing the copy, then finding that those two bits of code had both been duplicated, and so on. After fixing the same bug in 16 places, I turned them all into calls to one function. So, no, don't write anything twice if you can help it.

Patrick Tingen • Oct 11 '19

Yes, I could, but this post from @xtrasmal does it best. The article that is linked to is well worth reading.

Xander • Dec 19 '18 • Edited

Just wanted to leave this here for people who are interested.

"DRY is about Knowledge, Code duplication is not the issue."
verraes.net/2014/08/dry-is-about-k...

In your example, I believe that when you encounter 16 copies of a snippet, it is fair to say that you did a good job of bringing them together. The point of the article linked is that you need to take good care in deciding if two identical pieces of code really /mean/ the same thing.

Denis Howe • Oct 11 '19

Excellent. If only everyone understood the benefits of guard clauses! Some religions used to preach that a function should only have a single point of return, but that leads to hard-to-understand nested ifs. The other error is blindly going down the "happy path" first and leaving exceptions until later. Far better to test for the exceptional condition first and return or throw an error at that point in the code, allowing the reader forget about it and reducing nesting.

hidden_dude • Oct 14 '19

I think the obvious way to get rid of switch statements and long if/else clauses is with OO:

Instead of asking what "type" something is, use an objet that has all the attributes about that type in it.

so instead of :

getPokemon(type)

You'd write:

pokemon = Pokemon.fromString(typeStr)

name = pokemon.getName()

The obvious advantage is that instead of having a bunch of switch statements in your code, or a bunch of if/elses, or a bunch of tables that trigger on type, you have a single object that encapsulates the entire description of the pokemon, including name, preferences, color, size, etc.. You may have a table (or switch or if/else) to create that object from a string that indicates the "type". Or you could just use the specific class name everywhere (or prototypes if you prefer to do it that way).

That's fairly basic OO, I'm kind of surprised nobody mentioned this.