6,487 reads

5 VERY common code smells and how to fix them

by Mikhael LevkovskyAugust 10th, 2019

Too Long; Didn't Read

If you think something is missing, please check out my previous post, 5 easy wins to refactor even the ugliest code. In this post I will go over 5 very common code smells and how to fix them. Conditionals should each get their own line. It’s important to follow convention to ease development. Properly annotate your optional parameters in Typescript/Javascript. Use the ‘?’ instead of the undefined definition. Watch out for “Dead Stores Stores Stores’”.

Company Mentioned

featured image - 5 VERY common code smells and how to fix them

5 easy wins to refactor even the worst legacy code

So you just joined a new company, you’re excited to learn the newest technology and work on some super cool new projects and then BAM, you have to first learn and navigate the legacy system.

All of a sudden the excitement drains from your body as you start navigating helper file after helper file, unable to make heads or tails of the code base.In this post I will go over 5 VERY common code smells and how to fix them. If you think something is missing, please check out my previous post, .

1) Conditionals should each get their own line

Generally speaking your code will be a lot easier to read if each statement has its own line. The exception to the rule is to combine the else (or else/if) with the ending of the previous if. However, if you are writing a new if statement, it’s important to put it on a new line. This will prevent any future errors, as it may not be obvious that the two if statements are not logically connected.

//🚫
getBackgroundArt(track: Track): BackgroundImage {
 let backgroundImage: BackgroundImage;
 if(!track.getGenre()) {
   backgroundImage = {dimension: BackgroundImageDimensions.small, url : this.DEFAULT_BACKGROUND_IMAGE_URL};
 } if (track.getGenre() == "hiphop") {
   backgroundImage = {dimension: BackgroundImageDimensions.small, url: this.HIPHOP_BACKGROUND_IMAGE_URL};
 } if(track.getGenre() == "jazz") {
   backgroundImage = {dimension: BackgroundImageDimensions.small, url: this.JAZZ_BACKGROUND_IMAGE_URL};
 } if(track.getGenre() == "rap") {
   backgroundImage = {dimension: BackgroundImageDimensions.small, url: this.RAP_BACKGROUND_IMAGE_URL};
 } if(track.getGenre() == "country") {
   backgroundImage = {dimension: BackgroundImageDimensions.small, url: this.COUNTRY_BACKGROUND_IMAGE_URL};
 }
 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;
}

2) Properly annotate your optional parameters

Optional parameters can be found in most programming language. Typescript uses the ‘?’, Java uses the ‘Optional’ type, in PHP you can just assign a default value to a method parameter. In Typescript/Javascript, just like keeping the default clause last in a switch statement, it’s important to follow convention to ease development. When it comes to optional parameters, it’s preferable to use the ‘?’ over the undefined definition.

//🚫
export class Track {
 private name;
 private genre;
 private length;
 private backgroundImage : string | undefined;
 private album: string | undefined;
 private url: string | undefined;
}

✅
export class Track {
 private name;
 private genre;
 private length;
 private backgroundImage? : string;
 private album?: string;
 private url?: string;
}

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

3) Watch out for “Dead Stores”

A dead store is when you assign a value to a variable but is then re-assigned without actually using the original value. Calculating or setting a value, without actually using it is at best a waste of resources, and at worst a bug in our code. For the following examples, let’s assume we have an array of music tracks and we want to calculate the total runtime of all the songs.
A little added bonus in the following example, is the use of the reduce function to get our value.

//🚫
public getLength(): number {
 let totalLength = 0;
 totalLength = this.tracks.reduce((count, track) => count + track.getLength(),0);
 //convert to minutes
 return totalLength;
}

//✅
public getLength(): number {
 const totalLength = this.tracks.reduce((count, track) => count + track.getLength(),0);
// do some formatting later
 return totalLength;
}

4) Don’t Invert Your Booleans

One thing to keep in mind as you are developing, is that you are coding for humans, and not for the compilers. It’s better to keep things as simple and as humanly readable as possible. It’s way too complicated to read when you invert the result of a boolean expression, just use the opposite comparison instead.

//🚫
public deleteTrack(index: number) {
 // out of bound check
 if(!(index <= this.tracks.length)) {
   return;
 }

 this.tracks.splice(index,1);
}

//✅
public deleteTrack(index: number) {
 // out of bound check
 if(index >= this.tracks.length) {
   return;
 }

 this.tracks.splice(index,1);
}

5) Use Templates. Don’t concatenate!

When concatenating strings you should always stick to string templates instead of the concatenation operator. This will make your life much easier as it allows for multiline strings, reduces any errors if your strings have quotes and is generally a lot easier to read. Here is how it would look like when we try to create a TypeORM connection string without string templates and with.

//🚫
createConnection({
   type: 'sqlite',
   database: path.resolve(__dirname, '..')+'/data/musicnow.sqlite',
   entities: [
       __dirname+'/models/*.ts'
   ],
});

//✅
createConnection({
   type: 'sqlite',
   database: `${path.resolve(__dirname, '..')}/data/musicnow.sqlite`,
   entities: [
       `${__dirname}/models/*.ts`
   ],
});

There you have it, 5 more easy tips that you can apply to almost any codebase.To help you level up your coding skills, I’m putting together a playbook that includes: