5 easy wins to refactor even the ugliest code

Writing clean code can be a challenge when you start on a new project. Trying to clean up code in an already existing application without breaking anything is similar to this:

Heavenly @EVO 🍑

@heavenlycontrol

If you pointed a gun to my head and told me to finish this level i would tell you to pull the trigger

02:10 AM - 16 Jul 2019

55961 164660

I have been a technical lead for a few years and during that time I have seen my fair share of spaghetti code that I had to maintain, or worst extend. Through many painstakingly frustrating hours, and a dozen rubber ducks or so I have developed a few tips and tricks to help refactor even the ugliest code bases.

I put together a list of 5 quick wins that you can either apply to any code base or keep in mind if you're starting a new project.

Each win will start with a bad code sample followed by a good one with an explanation of what changed.

1) No Invariant Methods

A method can be considered pretty much useless if it always returns the exact same thing no matter what is passed in.
As you can see in the example invariant-method-bad.ts, getName will always return the exact same "NO NAME FOUND" message.
We fix this in the invariant-method-good.ts method by simply returning the result that we intended to from the get go.

2) No magic numbers or strings

Magic numbers and strings are hardcoded values that you can find sprinkled throughout your code. If not done carefully you will end up repeating the exact same values across your entire code base, which will make changing your code, debugging and testing it a nightmare.
As you can see below in magic-strings-bad.ts we have a hardcoded string in our method. In the file right below it, magic-strings-good.ts, we fixed this issue by refactoring it to a private read only class variable.

A good question to ask regarding magic numbers and strings, is "Where should I put them?". Here is how I decide where to put them:

1. Is the value only in one file? 👉 a constant in the top of the file
2. Is the value used across multiple files? 👉 a specific enum class that groups the constants logically
3. Is the value used when the application boots (e.g. an api url, a timeout threshold…) 👉 a properties file

pssst I tweet about code stuff all the time. If you have questions about how to level up your dev skills give me a follow @mlevkov

3) Keep the default clause last

A switch statement can contain a default clause to handle unexpected value. Considering you are most likely working collaboratively on a code base, it's important to follow conventions to ease communication.
Considering the example below, just moving the default clause all the way to the bottom will already save a lot of frustration for your peers.

4) Clean up your redundant variables

Keep an eye out when you don't need to instantiate a variable at all. Redundant variables take up memory, clutter up your code, make it harder to debug and add new features. Try to keep your code as tidy as possible. In the examples below, it's painfully obvious we don't need to create an instance of a BackgroundImage each time, and can just assign the value for each case.

5) Forget the else, return if you can

An important concept in keeping code clean is cyclomatic complexity. Cyclomatic complexity is basically how many loops, conditionals or function calls do you have nested in our code. Really high cyclomatic complexity makes code nearly impossible to wrap your head around. You will definitely need a dozen rubber ducks if it gets out of control.
Let's simplify the example from above by going from a switch statement to a tidy series of ifs.

This last example keeps the flow of logic dead simple. There is no alternative flows that you have to map out, everything is very straight forward and becomes much easier to digest.

There you have it, 5 easy tips that you can apply to almost any codebase.

If you want to level up your coding skills, I'm putting together a playbook that includes:

1. 30+ common code smells & how to fix them

2. 15+ design pattern practices & how to apply them

3. 20+ common JS bugs & how to prevent them

Get early access to the Javascript playbook.

Comments

[Graham Trott]

If switch is implemented by the compiler as a look-up table it might offer somewhat better performance, especially as the number of options grows. Actually, I often prefer switch to chained ifs as to me it's more aesthetic; it's saying clearly "Here's a set of options to choose from". You can still implement return instead of break in any of the cases. But I guess a lot of things are down to personal preference.

Whenever I see multiple choices of this kind I like to use tables, so I'd try something like this, which also permits easy initialization from JSON (warning: untested code):

const images = {
   "hiphop": "https://unsplash.com/photos/Qcl98B8Bk3I",
   "jazz": "https://unsplash.com/photos/dBWvUqBoOU8",
   "rap": "https://unsplash.com/photos/auq_QbyIA34",
   "country": "https://unsplash.com/photos/RnFgs90NEHY"
};

const url = images[track.getGenre()];
return {
   dimension: 'small',
   url: url ? url : 'https://unsplash.com/photos/PDX_a_82obo'
};

[Mikhail Levkovsky]

Great point! This was just to illustrate an example but yours illustrates it even better :)
Thanks for sharing it!
To be honest, I would prefer to have a designated factory to create these things so as not to mix behavioural and creational logic in one class.

[Graham Trott]

I'd agree with you there Mikhail. It makes the overall program so much more concise and readable.

One of the joys (and frustrations) of JavaScript is there are so many different ways of doing the same thing. As I typed the words "look-up table" I suddenly recognized a pattern, where all the text could be extracted into constant data separate from the behavior. Sometimes you approach these things sideways and the pattern leaps out at you from nowhere.

[Juan Miramontes]

if I were to refactor the getName function, I would simply do:

public getName(): string {
  return this.name ? this.name : this.NAME_DEFAULT;
}

or even:

public getName(): string {
  return this.name || this.NAME_DEFAULT;
}

It includes points 1) and 2), and I think is simpler and more readable.

[Templar++]

I don't understand what's wrong with the switch statement? Instead of assigning the variable you could have return for each case and this will also save you the break statement as well.

getBackgroundArt(track: Track): BackgroundImage {
 let backgroundImage: BackgroundImage;
 switch(track.getGenre()) {
   case "hiphop":
     return {dimension: 'small', 'url': 'https://unsplash.com/photos/Qcl98B8Bk3I'};
   case "jazz":
     return {dimension: 'small', 'url': 'https://unsplash.com/photos/dBWvUqBoOU8'};
   case "rap":
     return {dimension: 'small', 'url': 'https://unsplash.com/photos/auq_QbyIA34'};
   case "country":
     return {dimension: 'small', 'url': 'https://unsplash.com/photos/RnFgs90NEHY'};
   default:
     return {dimension: 'small', url : 'https://unsplash.com/photos/PDX_a_82obo'};
 }
}

Moreover instead of calling track.getGenre() once, you do that multiple times, and functions may implement complex logic.

[Will Stone]

Was going to comment with the same. You can even ditch line 2 let backgroundImage: BackgroundImage; now this variable isn't used.

[Templar++]

Yep, forgot that one.. lint usually reminds me in this case :)

[Mikhail Levkovsky]

100% agreed about calling track.getGenre() once, should definetely do that.
as for that last example, that's actually the good one, where you return from each case, which IMO helps keep the flow of the code linear and simple

[John]

Number 5 - assign track.getGenre() to a string once at the beginning and compare to that variable.

var genre = track.getGenre();
if (genre == "jazz"){}
...

No need to run the overhead of whatever logic is involved in getGenre() in each if.

[Mikhail Levkovsky]

good call! thanks for spotting it :)

[Cubicle Buddha]

Hypothetical question: What happens if you add a new genre and you don’t want the default banner to be returned?

What I’m getting at is that it’s much safer to throw in a default. Or if it’s typescript it’s safer to return a never type. That way you can exhaustively consider enum values or any union of types. I describe that technique here.

[Mikhail Levkovsky]

Good point, for sure a switch statement should have a default.
I didn't include it in this case for brevity, and this is just an example. For something slightly more evolved I would actually resort to the factory pattern to create my return types and simplify the creation logic even more

[Nicolas Carlo]

Thank you for this post.
It sums up very nice, simple advices to make code cleaner 👌

Inline Variable (related to #4) and Extract Variable (related to #2) are the two refactorings I'm probably using the most, on a daily basis.

I'm currently working on a VS Code Extension to get the editor do these refactorings automatically for us. My goal is to make these typical operations easy to use.

For example, I can now solve #5 very quickly with Remove Redundant Else.

Maybe you'd be interested in using it.
It's called Abracadabra: bit.ly/vscode-abracadabra

If you feel like trying it, I'd love to get your feedbacks, so I can make it better.
Thanks again for sharing these practices. Have a nice day 😉