Introduction

 

“A code smell is a surface indication that usually corresponds to a deeper problem in the system.” — Martin Fowler


“Bad code smells can be an indicator of factors that contribute to technical debt.” — Robert C. Martin

Martin Fowler and Robert C. Martin definition’s complete with each other because Martin Fowler’s definition indicates a clue for a software issue and Robert Martin’s definition refers to a side effect caused by code smells.

Good code is easy to understand and maintainable. It is one of the important keys for bug-free software and easy to understand the codebase.

Using clean code practices you can make your codebase easy to understand and maintainable. If other developers working on the same project it will be easy to follow for them which means lesser questions.

 

Make it Understandable

Choose easy to understand and short names for variables and functions.

Bad Variable names
 
theproduct, src, rslt

Better variable names
 
product, source, result

Also bad variable names

countFromOneToTen, createNewMemberIfNotExists

Avoid describing a value with your variable or function name.


Might not make sense in some countries

isOverEighteen()


Everyone can understand it now

isLegalAge()

Your code is a story - make your storyline easy to follow!


Avoid Globals

You run the danger of your code being overwritten by any other JavaScript added to the page after yours. You can use local variables, closures, and the module pattern

Bad

All variables are global and can be accessed; access is not contained, anything on the page can overwrite what you do.

var current = null;
var labels = {
   'index':'index',
   'about':'about',
   'contact':'contact'
};
function init(){
};
function show(){
   current = 1;
};
function hide(){
   show();
};

Repetition of the module name, the different syntax for inner functions.

module = function(){
   var labels = {
      'index':'index',
      'about':'about',
      'contact':'contact'
   };
   return {
      current:null,
      init:function() { },
      show:function() { module.current = 1; },
      hide:function() { module.show(); }
} }();

 

Good

Far more consistent isn't it?  See the difference.

module = function(){
   var current = null;
   var labels = {
      'home':'home',
      'articles':'articles',
      'contact':'contact'
   };
   var init = function(){ };
   var show = function(){ current = 1; };
   var hide = function(){ show(); }
   return{init:init, show:show, current:current}
}();
module.init();

 

Stick to a Strict Coding Style

Browsers are very forgiving JavaScript parsers. However, a bad coding style will be a problem when you shift to another environment or hand it over to another developer. A valid code is a more secure code.

Validate your code: http://www.jslint.com/

Comment as Much as Needed but Not More

“Good code explains itself” is an arrogant sentence.
Comment what you needed - but you don’t have to write your own life story.

Also don't leave commented out code in your codebase. Version control systems exist for a reason.

Avoid using the line comment //. /* */ is much safer to use because it doesn’t cause errors when the line break is removed.

Comments should never go out to the end-user in plain HTML or JavaScript.Because development code is not live code

Think Simple

Example

Put a red border around all fields with a class of “mandatory” when they are empty.

Bad

var form = document.getElementById('mainform');
var inputs = form.getElementsByTagName('input');
for(var i=0,j=inputs.length;i<j;i++){
   if(inputs[i].className === 'mandatory' && inputs.value === ''){
      inputs[i].style.borderColor = '#f00';
      inputs[i].style.borderStyle = 'thin';
      inputs[i].style.borderWidth = '2px';
   }
}

Good
People shouldn’t have to change the JavaScript code to change the design

var form = document.getElementById('mainform');
var inputs = form.getElementsByTagName('input');
for(var i=0,j=inputs.length;i<j;i++){
   if(inputs[i].className === 'mandatory' && inputs.value === '') { inputs[i].className+=' error'; }
}

 

Use Shortcut Notations

Shortcut notations make your code easier to read

Bad
var direction;
if(x>100) {direction=1;} else {direction = -1;}

Good
var direction; (x>100) ? 1:-1;

Bad
if(isValid) {gotoLogin()}

Good
isValid && gotoLogin();

Both the above expressions work the same way. Only when isValid is true, the gotoLogin()  the function would be executed.

 

Better Configuration and Translation

Everything likely to change in your code should not be scattered throughout your code like texts, CSS classes, and others
By putting these into a config file and making them public it will be far more simple, understandable, and maintainable.

For example:

carousel = function(){
   var config = {
      CSS:{
         classes:{ current:'current', scrollContainer:'scroll' },
         IDs:{ maincontainer:'carousel' }
      },
labels:{
         previous:'back',
         next:'next',
         auto:'play'
      },
      settings:{
         amount:5,
         skin:'blue',
         autoplay:false }, };
   function init(){ };
   function scroll(){ };
   function highlight(){ };
   return {config:config,init:init}
}();

Don’t Trust Any Data

Good code doesn't trust any data.

 

Don’t believe the HTML document

Any user can change it for purposes.

 

Don’t trust that data that reaches your function is of the right format.

Test with Type Of and then do something with it.

 

Don’t expect elements in the DOM to be available.

Test for them and that they indeed are what you expect them to be before altering them.

 

Never use JavaScript to protect something.

JavaScript is as easy to crack as it is to code :)

Optimize Loops

Loops sometimes can be terribly slow in JavaScript.
Actually most of the time it is because you’re doing things in them that don’t make sense.
Don’t make JavaScript read the length of an array at every iteration of a for a loop. Store the length value in a different variable. Keep computation-heavy code outside of loops. This includes regular expressions but first and foremost DOM manipulation.
You can create the DOM nodes in the loop but avoid inserting them into the document.

 

Use Javascript for Functionality Not Content

If you are creating a lot of HTML in JavaScript, you might be doing something wrong.

It is not convenient to create using the DOM, it’s risky to use innerHTML (IE’s Operation Aborted error), and it’s hard to keep track of the quality of the HTML you produce.

If you really have to do it, load the interface as a static HTML document via Ajax. That way you keep maintenance in HTML and allow for customization.


Use === Comparison

When you use === your variables will be converted to match types.

0 == ""; true 
0 === ""; //false

The operator forces a comparison of values and types.

Don't Declare Unnecessary Variables

More variables mean more memory space and that's not a good thing.

For example, instead of writing this:

const sidebar = sidebar.querySelector(#sidebar);
const paragraph = foo.querySelector('p');
paragraph.textContent = 'foo';

You can write this:

document.querySelector('#sidebar p').textContent = 'foo';

In the first case, we had two variables. Now we have no variables.
 

Do not use wildcard imports

This makes sure you have a single default export. 

Bad
import * as Foo from './Foo';
 
Good
import Foo from './Foo';

Try and avoid unneeded ternary statements

Bad

const bo = a ? a : b;

Good

const bo = a || b;

Use Parameter Defaults

When you call a function and forget to pass a parameter to it.

function logNumber(num) {
if (num === undefined) {
num = 25;
}
console.log(num);
}
logNumber();

ES6 introduced default parameters for the function. You can do this:

function logNumber(num = 25) {
console.log(num);
}
logNumber();

It is a good habit to assign default values to arguments.
 

Don't Use new Object()

Use {} instead of new Object()
Use "" instead of new String()
Use 0 instead of new Number()
Use false instead of new Boolean()
Use [] instead of new Array()
Use /()/ instead of new RegExp()
Use function (){} instead of new Function()

The advantages are:

Code is shorter and more readable
Code is safer. Literals will still work when the Object constructors have been overridden
It is a little bit faster

Use Logical Or

The logical OR (||) the operator will return true if at least one of the operands true.

const arg1 = null;
let args2
if(arg1) {
arg2 = arg1;
}
else {
arg2 = “Default”;
}
// use this
const arg2 = arg1 || “Default”

So, && returns the first falsy value and || returns the first truthy value. Do note that using logical operator chaining could lead to readability issues in some scenarios.

 

If statements

I could not find a name for this problem.
But I see this a lot.

Problem

if(value === 'duck' || value === 'dog' || value === 'cat') {
  // ...
}

Solution

const options = ['duck', 'dog', 'cat'];
if (options.includes(value)) {
  // ...
}

By doing this, you create a readable piece of code that looks like an English sentence.

Early exit

There are a dozen ways of naming this principle, but I picked the name "Early exit".

function handleEvent(event) {
  if (event) {
    const target = event.target;
    if (target) {
      // Your awesome piece of code that uses target
    }
  }
}

Here we are trying to check if the object event is not falsy and the property target is available. Now the problem here is that we are already using 2 if statements.
Let's see how you could do an "early exit" here.

function handleEvent(event) {
  if (!event || !event.target) {
    return;
  }
  // Your awesome piece of code that uses target
}

By applying the "early exit" here, you check if event and event.target is not falsy. It's immediately clear that we are sure that event.target is not false. It's also clear that no other code is executed if this statement is false.
 

Unique values

There will be times where you’ll need to remove duplicate values from an array. One way of doing is this by looping over the values and keep track of the items you’ve already looped over. This already sounds complicated and seems way too much code for something so easy.

const numbers = [1, 2, 3, 4, 5, 6, 2, 7, 4]
const uniqueNumbers = Array.from(new Set(numbers)) 
console.log(uniqueNumbers) // [1, 2, 3, 4, 5, 6, 7]

The reason this works is that values in a Set-object may only occur once. This means that a Set only works with unique values. All we do with this little trick is cast our initial array to a Set and then convert it back to an array. This way the duplicate values will get filtered out.
 

Counting values

Let’s say we have an array that contains orders which look like this:

const orders = [
{ number: 78, vat: 12, total: 101, },
{ number: 83, vat: 5, total: 40, },
{ number: 84, vat: 18, total: 67, }
]

We want to know how much the total of all the orders is so we know how much revenue we have. One way of doing this is by using a for-loop.

let total = for(let i = 0; i < orders.length; i++) {
total += orders[i].total
}

Although this works, the problem with for-loops is that they’re ugly. There is a more convenient way of doing this that uses fewer lines of code. It’s time to meet the reduce method.
The reduce method reduces an array to a single value. The first argument is the callback which has two arguments: the accumulator and the current value.
To get the total value of all the orders we can use the reduce method. We can do this by adding the total of each order to the accumulator.
 

orders.reduce((acc, cur) => acc + cur.total, 0) // 208

The last argument is the initial value of the accumulator, which is 0 in this example. If you don’t specify an initial value the first element in the array will be used as the initial value.

 

Use Template literals

The last trick that we’re going over in this article is about template literals. Template literals allow you to concatenate strings elegantly. This way you don’t have to concatenate strings using the plus sign ever again.

You’ve probably concatenated strings using the plus sign, which looks like this:

const first = "John"
const last = "Doe"const full = first + " " + last

Not the prettiest way you could concatenate these strings. Luckily, there’s an alternative.

const full = first + " " + lastconst full = `${first} ${last}`

 

Don’t enforce the need for memorizing the variable context

Bad

const products = ["T-Shirt", "Shoes", "Jackets", "Gloves"];

products.forEach(p => {
  doSomething();
  doSomethingElse();
  // ...
  // ...
  // What is `p` for?
  Add(p);
});

Good

const products = ["T-Shirt", "Shoes", "Jackets", "Gloves"];

products.forEach(product => {
  doSomething();
  doSomethingElse();
  // ...
  // ...
  // ...
  register(product);
});

Use 'Object.assign’ to set default objects

Bad

const shapeConfig = {
  type: "circle",
  width: 150,
  height: null
};

function createShape(config) {
  config.type = config.type || "circle";
  config.width = config.width || 300;
  config.height = config.height || 300;
}

createShape(shapeConfig);

Good

const shapeConfig = {
  type: "circle",
  width: 200
  // Exclude the 'height' key
};

function createShape(config) {
  config = Object.assign(
    {
      type: "circle",
      width: 400,
      height: 400
    },
    config
  );
  ...
}
  
createShape(shapeConfig);

 

Never use var use let instead

A simple one, using let will help with any scoping issues var causes in JavaScript.

Naming conventions in JavaScript

let should be camelCase
const if at the top of a file use uppercase snake case. MY_CONST. If not at the top of the file using camelCase
class should be PascalCasing. MyClass
functions should be camelCase . MyFunction
 

Use ES6 arrow functions where possible

Arrow functions are a more concise syntax for a writing function expression. They’re are anonymous and change the way this binds in functions.

Fail(s):
var multiply = function(a, b) {
return a* b;
};

Pass: 
const multiply = (a, b) => { return a * b}; 

Lowercase file names

MyFile.js should be myFile.js

 

Using Array. every

What if you’re given an array of pets and asked to check whether all the pets have four legs or not?
Typically, we will do as below:

const pets = [
{ name: ‘cat’, nLegs: 4 }, { name: ‘snake’, nLegs: 0 }, { name: ‘dog’, nLegs: 4 },
{ name: ‘bird’, nLegs: 2 } ];
const check = (pets) => {
for (let i = 0; i < pets.length; i++) {
if (pets[i].nLegs != 4) {
return false;
}
}
return true;
}
check(pets); // false

We got a for and an if statement here. It’s not a lot but it’s still great if we can get rid of them. In fact, we can complete the task with only one line:

let areAllFourLegs = pets.every(p => p.nLegs === 4);
 

Helper Functions

We also get else statements that don't directly result in a return. This is usually through some special-case logic that isn't isolated properly. For example

  let charities
  if (country != "") {
    if (tier != "") {
      charities = getCharitiesByCampaignCountryAndTier(campaign, country, tier)
    } else {
      charities = getCharitiesByCampaignAndCountry(campaign, country)
    }
  } else {
    charities = getCharitiesByCampaign(campaign)
  } 
// Do something with charities

The readability of this can be improved by pulling the charity-getting logic into its own function. This then lets the special cases be handled appropriately, and return early. By inverting some of the if statements, this can be improved further.
For example,

function getCharities(campaign, country, tier) {
  if (country == "") {
    return getCharitiesByCampaign(campaign)
  }

  if (tier == "") {
    return getCharitiesByCampaignAndCountry(campaign, country)
  }

  return getCharitiesByCampaignCountryAndTier(campaign, country, tier)
}

This helper function neatly encapsulates all the logic we'd need, removes the need for any else statements, and does a much better job of keeping the happy-path code to the left. This is much easier to scan through, and much more readable as a result.
 

Avoid Heavy Nesting

Code gets unreadable after a certain level of nesting.
A really bad idea is to nest loops inside loops as that also means taking care of several iterator variables (i,j,k,l,m...).
 

function renderProfiles(o){
   var out = document.getElementById('profiles');
   for(var i=0;i<o.members.length;i++){
      var ul = document.createElement('ul');
      var li = document.createElement('li');
      li.appendChild(document.createTextNode(o.members[i].name));
      var nestedul = document.createElement('ul');
      for(var j=0;j<o.members[i].data.length;j++){
         var datali = document.createElement('li');
         datali.appendChild(
            document.createTextNode(
               o.members[i].data[j].label + ' ' +
               o.members[i].data[j].value
            )
         );
         nestedul.appendChild(detali);
      }
      li.appendChild(nestedul);
   }
   out.appendChild(ul);
}

You can avoid heavy nesting and loops inside loops with specialized tool methods.

function renderProfiles(o){
   var out = document.getElementById('profiles');
   for(var i=0;i<o.members.length;i++){
      var ul = document.createElement('ul');
      var li = document.createElement('li');
      li.appendChild(document.createTextNode(data.members[i].name));
      li.appendChild(addMemberData(o.members[i]));
   }
   out.appendChild(ul);
}
function addMemberData(member){
   var ul = document.createElement('ul');
   for(var i=0;i<member.data.length;i++){
      var li = document.createElement('li');
      li.appendChild(
         document.createTextNode(
            member.data[i].label + ' ' +
            member.data[i].value
         )
      );
   }
   ul.appendChild(li);
   return ul;
}

Think of bad editors and small screens.

Use method chaining

Many libraries such as Jquery and Lodash using that technique. If you have a method that accepted something like 10 parameters it will be better to use method chaining

Bad

class Product {
  
  constructor(name) {
    this.name = name;
  }
  
  setPrice(price) {
    this.price = price;
  }
  
  setUnits(units) {
    this.units = units;
  }
  
  save() {
    console.log(this.name, this.price, this.units);
  }
}

const product = new Product("Bag");

product.setPrice(23.99);
product.setUnits(12);
product.save();

Good

class Product {
  
  constructor(name) {
    this.name = name;
  }
  
  setName(name) {
    this.name = name;
    // Return this for chaining
    return this;
  }
  
  setPrice(price) {
    this.price = price;
    // Return this for chaining
    return this;
  }
  
  save() {
    console.log(this.name, this.price, this.units);
    // Return this for chaining
    return this;
  }
}

const product = new Product("T-Shirt")
    .setName("Jeans")
    .setAge(31.99)
    .save();


Use Prettier 

Prettier is an opinionated code formatter.
It enforces a consistent style by parsing your code and re-printing it with its own rules.

https://prettier.io/

Use searchable names

Mostly we need to read more code than write. It's important that the code we do write is readable and searchable. By not naming variables is also a bad practice. Make your names searchable.

Bad:

// What 12300000 is for?
setTimeout(blastOff, 12300000);

Good:

// Give a meaningful names them
const MILLISECONDS_IN_A_DAY = 60 * 60 * 24 * 1000; //86400000;

setTimeout(blastOff, MILLISECONDS_IN_A_DAY);

Functions should do one thing

That’s the most important and basic rule in software engineering. When functions do more than one thing, they are harder to maintain and test . If you isolate a function to just one action, it can be refactored easily and your code will read much cleaner.

Bad:

function emailClients(clients) {
  clients.forEach(client => {
    const clientRecord = database.lookup(client);
    if (clientRecord.isActive()) {
      email(client);
    }
  });
}

Good:

function emailActiveClients(clients) {
  clients.filter(isActiveClient).forEach(email);
}

function isActiveClient(client) {
  const clientRecord = database.lookup(client);
  return clientRecord.isActive();
}

Avoid type-checking

JavaScript is an untyped language, which means your functions can take any kind of argument. There are some disadvantages of that freedom and that’s why sometimes you need to type check in your functions. There are many ways to avoid having to do this.

Bad:

function travelToTexas(vehicle) {
  if (vehicle instanceof Bicycle) {
    vehicle.pedal(this.currentLocation, new Location("texas"));
  } else if (vehicle instanceof Car) {
    vehicle.drive(this.currentLocation, new Location("texas"));
  }
}

Good:

function travelToTexas(vehicle) {
  vehicle.move(this.currentLocation, new Location("texas"));
}