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:

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 bad example, getName will always return the exact same "NO NAME FOUND" message. We fix this in the good example method by simply returning the result that we intended to from the get go.

🚫
public getName(): string {
 let result = "NO NAME FOUND";
 if(this.name) {
   result = this.name;
 }
 return "NO NAME FOUND";
}

✅
public getName(): string {
 let result = "NO NAME FOUND";
 if(this.name) {
   result = this.name;
 }
 return result;
}

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 we have a hardcoded string in our method. In the file right below it, we fixed this issue by refactoring it to a private read only class variable.

🚫
public getName(): string {
 let result = "NO NAME FOUND";
 if(this.name) {
   result = this.name;
 }
 return result;
}

✅
export class User {
 private readonly NAME_DEFAULT = "NO NAME FOUND";
 private name;


 public getName(): string {
   let result = this.NAME_DEFAULT;
   if(this.name) {
     result = this.name;
   }
   return result;
 }
}

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.

🚫
import { Track } from "../models/Track";

export class BackgroundImage {
 dimension: string;
 url: string;
}

export class BackgroundImageService {

 getBackgroundArt(track: Track): BackgroundImage {
   switch(track.getGenre()) {
     default:
       return null;
     case "hiphop":
       const hipHopImage: BackgroundImage = {dimension: 'small', 'url': 'https://unsplash.com/photos/Qcl98B8Bk3I'};
       return hipHopImage;
     case "jazz":
       const jazzImage : BackgroundImage = {dimension: 'small', 'url': 'https://unsplash.com/photos/dBWvUqBoOU8'};
       return jazzImage;
     case "rap":
       const rapImage : BackgroundImage = {dimension: 'small', 'url': 'https://unsplash.com/photos/auq_QbyIA34'};
       return rapImage;
     case "country":
       const countryImage: BackgroundImage = {dimension: 'small', 'url': 'https://unsplash.com/photos/RnFgs90NEHY'};
       return countryImage;
   }
 }
}

✅
getBackgroundArt(track: Track): BackgroundImage {
 switch(track.getGenre()) {
   case "hiphop":
     const hipHopImage: BackgroundImage = {dimension: 'small', 'url': 'https://unsplash.com/photos/Qcl98B8Bk3I'};
     return hipHopImage;
   case "jazz":
     const jazzImage : BackgroundImage = {dimension: 'small', 'url': 'https://unsplash.com/photos/dBWvUqBoOU8'};
     return jazzImage;
   case "rap":
     const rapImage : BackgroundImage = {dimension: 'small', 'url': 'https://unsplash.com/photos/auq_QbyIA34'};
     return rapImage;
   case "country":
     const countryImage: BackgroundImage = {dimension: 'small', 'url': 'https://unsplash.com/photos/RnFgs90NEHY'};
     return countryImage;
   default:
     return null;
 }
}

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.

🚫
getBackgroundArt(track: Track): BackgroundImage {
 switch(track.getGenre()) {
   case "hiphop":
     const hipHopImage: BackgroundImage = {dimension: 'small', 'url': 'https://unsplash.com/photos/Qcl98B8Bk3I'};
     return hipHopImage;
   case "jazz":
     const jazzImage : BackgroundImage = {dimension: 'small', 'url': 'https://unsplash.com/photos/dBWvUqBoOU8'};
     return jazzImage;
   case "rap":
     const rapImage : BackgroundImage = {dimension: 'small', 'url': 'https://unsplash.com/photos/auq_QbyIA34'};
     return rapImage;
   case "country":
     const countryImage: BackgroundImage = {dimension: 'small', 'url': 'https://unsplash.com/photos/RnFgs90NEHY'};
     return countryImage;
   default:
     const defaultImage: BackgroundImage = {dimension: 'small', 'url': 'https://unsplash.com/photos/PDX_a_82obo'};
     return defaultImage;
 }
}

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

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.

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

🚫
getBackgroundArt(track: Track): BackgroundImage {
 let backgroundImage: BackgroundImage;
 if(!track.getGenre()) {
   backgroundImage = {dimension: BackgroundImageDimensions.small, url : this.DEFAULT_BACKGROUND_IMAGE_URL};
 } else if (track.getGenre() == "hiphop") {
   backgroundImage = {dimension: BackgroundImageDimensions.small, url: this.HIPHOP_BACKGROUND_IMAGE_URL};
 } else if(track.getGenre() == "jazz") {
   backgroundImage = {dimension: BackgroundImageDimensions.small, url: this.JAZZ_BACKGROUND_IMAGE_URL};
 } else if(track.getGenre() == "rap") {
   backgroundImage = {dimension: BackgroundImageDimensions.small, url: this.RAP_BACKGROUND_IMAGE_URL};
 } else if(track.getGenre() == "country") {
   backgroundImage = {dimension: BackgroundImageDimensions.small, url: this.COUNTRY_BACKGROUND_IMAGE_URL};
 }
 return backgroundImage;
}

✅
getBackgroundArt(track: Track): BackgroundImage {
 let backgroundImage: BackgroundImage = {dimension: BackgroundImageDimensions.small, url : this.DEFAULT_BACKGROUND_IMAGE_URL};
 if (track.getGenre() == "hiphop") {
   return {dimension: BackgroundImageDimensions.small, url: this.HIPHOP_BACKGROUND_IMAGE_URL};
 }
 if(track.getGenre() == "jazz") {
   return {dimension: BackgroundImageDimensions.small, url: this.JAZZ_BACKGROUND_IMAGE_URL};
 }
 if(track.getGenre() == "rap") {
   return {dimension: BackgroundImageDimensions.small, url: this.RAP_BACKGROUND_IMAGE_URL};
 }
 if(track.getGenre() == "country") {
   return {dimension: BackgroundImageDimensions.small, url: this.COUNTRY_BACKGROUND_IMAGE_URL};
 }
 return backgroundImage;
}

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 🚀