Smelly Code

/* const */ var CONSONANTS = 'bcdfghjklmnpqrstvwxyz';
/* const */ var VOWELS = 'aeiou';

function englishToPigLatin(english) {
  /* const */ var SYLLABLE = 'ay';
  var pigLatin = '';

  if (english !== null && english.length > 0 &&
    (VOWELS.indexOf(english[0]) > -1 ||
    CONSONANTS.indexOf(english[0]) > -1 )) {
    if (VOWELS.indexOf(english[0]) > -1) {
      pigLatin = english + SYLLABLE;
    } else {
      var preConsonants = '';
      for (var i = 0; i < english.length; ++i) {
        if (CONSONANTS.indexOf(english[i]) > -1) {
          preConsonants += english[i];
          if (preConsonants == 'q' &&
            i+1 < english.length && english[i+1] == 'u') {
            preConsonants += 'u';
            i += 2;
            break;
          }
        } else { break; }
      }
      pigLatin = english.substring(i) + preConsonants + SYLLABLE;
    }
  }

  return pigLatin;
}

Why Does This Smell?

LOTS OF REASONS!

• Too Many Statements
• Too Much Depth
• Too Much Complexity

Cyclomatic Complexity

"... is a software metric (measurement), used to indicate the complexity of a program. It is a quantitative measure of the number of linearly independent paths through a program's source code. It was developed by Thomas J. McCabe, Sr. in 1976." --wikipedia

Tooling

Linting

Rules

/*jshint maxstatements:15, maxdepth:2, maxcomplexity:5 */

/*eslint max-statements:[2, 15], max-depth:[1, 2], complexity:[2, 5] */

Result

7:0 - Function 'englishToPigLatin' has a complexity of 7.
7:0 - This function has too many statements (16). Maximum allowed is 15.
22:10 - Blocks are nested too deeply (5).

But Wait, There's More! (Current Stats)

• max-statements:[2, 16]
• max-depth:[2, 5]
• complexity:[2, 7]
• max-len:[2, 65]
• max-params:[2, 1]
• max-nested-callbacks:[2, 0]
  (ESLint Only)

So What!?!

Refactor

Unit Test First

describe('Pig Latin', function() {
  describe('Invalid', function() {
    it('should return blank if passed null', function() {
      expect(englishToPigLatin(null)).toBe('');
    });

    it('should return blank if passed blank', function() {
      expect(englishToPigLatin('')).toBe('');
    });

    it('should return blank if passed number', function() {
      expect(englishToPigLatin('1234567890')).toBe('');
    });

    it('should return blank if passed symbol', function() {
      expect(englishToPigLatin('~!@#$%^&*()_+')).toBe('');
    });
  });

  describe('Consonants', function() {
    it('should return eastbay from beast', function() {
      expect(englishToPigLatin('beast')).toBe('eastbay');
    });

    it('should return estionquay from question', function() {
      expect(englishToPigLatin('question')).toBe('estionquay');
    });

    it('should return eethray from three', function() {
      expect(englishToPigLatin('three')).toBe('eethray');
    });
  });

  describe('Vowels', function() {
    it('should return appleay from apple', function() {
      expect(englishToPigLatin('apple')).toBe('appleay');
    });
  });
});

Refactor

const CONSONANTS = ['th', 'qu', 'b', 'c', 'd', 'f', 'g', 'h', 'j', 'k',
'l', 'm', 'n', 'p', 'q', 'r', 's', 't', 'v', 'w', 'x', 'y', 'z'];
const VOWELS = ['a', 'e', 'i', 'o', 'u'];
const ENDING = 'ay';

let isValid = word => startsWithVowel(word) || startsWithConsonant(word);
let startsWithVowel = word => !!~VOWELS.indexOf(word[0]);
let startsWithConsonant = word => !!~CONSONANTS.indexOf(word[0]);
let getConsonants = word => CONSONANTS.reduce((memo, char) => {
  if (word.startsWith(char)) {
    memo += char;
    word = word.substr(char.length);
  }
  return memo;
}, '');

function englishToPigLatin(english='') {
   if (isValid(english)) {
      if (startsWithVowel(english)) {
        english += ENDING;
      } else {
        let letters = getConsonants(english);
        english = `${english.substr(letters.length)}${letters}${ENDING}`;
      }
   }
   return english;
}

Updated Stats

• max-statements: 16 → 6
• max-depth: 5 → 2
• complexity: 7 → 3
• max-len: 65 → 73
• max-params: 1 → 2
• max-nested-callbacks: 0 → 1

Resources

• jshint - http://jshint.com/
• eslint - http://eslint.org/
• jscomplexity - http://jscomplexity.org/
• escomplex - https://github.com/philbooth/escomplex
• jasmine - http://jasmine.github.io/

Smelly Code

function getArea(shape, options) {
  var area = 0;

  switch (shape) {
    case 'Triangle':
      area = .5 * options.width * options.height;
      break;

    case 'Square':
      area = Math.pow(options.width, 2);
      break;

    case 'Rectangle':
      area = options.width * options.height;
      break;

    default:
      throw new Error('Invalid shape: ' + shape);
  }

  return area;
}

getArea('Triangle',  { width: 100, height: 100 });
getArea('Square',    { width: 100 });
getArea('Rectangle', { width: 100, height: 100 });
getArea('Bogus');

Why Does This Smell?

Violates the "Open/Closed Principle"

Open/Closed Principle (OCP)

"...software entities (classes, modules, functions, etc.) should be open for extension, but closed for modification; that is, such an entity can allow its behaviour to be extended without modifying its source code." --wikipedia

Adding a New Shape

function getArea(shape, options) {
  var area = 0;

  switch (shape) {
    case 'Triangle':
      area = .5 * options.width * options.height;
      break;

    case 'Square':
      area = Math.pow(options.width, 2);
      break;

    case 'Rectangle':
      area = options.width * options.height;
      break;

    case 'Circle': // 17: New Shape Type
      area = Math.PI * Math.pow(options.radius, 2);
      break;

    default:
      throw new Error('Invalid shape: ' + shape);
  }

  return area;
}
 
 
 

So What!?!

Strategy Design Pattern

Strategy Design Pattern

(function(shapes) { // triangle.js
  var Triangle = shapes.Triangle = function(options) {
    this.width = options.width;
    this.height = options.height;
  };
  Triangle.prototype.getArea = function() {
    return 0.5 * this.width * this.height;
  };  
}(window.shapes = window.shapes || {}));

function getArea(shape, options) {
  var Shape = window.shapes[shape], area = 0;

  if (Shape && typeof Shape === 'function') {
    area = new Shape(options).getArea();
  } else {
    throw new Error('Invalid shape: ' + shape);
  }

  return area;
}

getArea('Triangle',  { width: 100, height: 100 });
getArea('Square',    { width: 100 });
getArea('Rectangle', { width: 100, height: 100 });
getArea('Bogus');

Adding a New Shape

// circle.js
(function(shapes) {
  var Circle = shapes.Circle = function(options) {
    this.radius = options.radius;
  };

  Circle.prototype.getArea = function() {
    return Math.PI * Math.pow(this.radius, 2);
  };

  Circle.prototype.getCircumference = function() {
    return 2 * Math.PI * this.radius;
  };
}(window.shapes = window.shapes || {}));

Any Other Smells?

Magic Strings

Magic Strings

function getArea(shape, options) {
  var area = 0;

  switch (shape) {
    case 'Triangle': // 5: Magic String
      area = .5 * options.width * options.height;
      break;
    /* ... more code ... */
  }

  return area;
}

getArea('Triangle', { width: 100, height: 100 }); // 14: Magic String
 
 

Magic Strings Refactor

Object Types

var shapeType = {
  triangle: 'Triangle' // 2: Object Type
};

function getArea(shape, options) {
  var area = 0;
  switch (shape) {
    case shapeType.triangle: // 8: Object Type
      area = .5 * options.width * options.height;
      break;
  }
  return area;
}

getArea(shapeType.triangle, { width: 100, height: 100 }); // 15: Object Type
 
 
 
 
 

Magic Strings Refactor

const & symbols

const shapeType = {
  triangle: Symbol() // 2: Enum-ish
};

function getArea(shape, options) {
  var area = 0;
  switch (shape) {
    case shapeType.triangle: // 8: Enum-ish
      area = .5 * options.width * options.height;
      break;
  }
  return area;
}

getArea(shapeType.triangle, { width: 100, height: 100 }); // 15: Enum-ish
 
 
 
 
 

Tooling!?!

Nope :(

eslint-plugin-smells

ESLint rules for JavaScript Smells http://bit.ly/eslint-plugin-smells

Rules

• no-switch - disallow the use of the switch statement
• no-complex-switch-case - disallow use of complex switch statements

What about edge cases?

/*eslint no-switch:[0] */

switch (test) { case 'val': break; }

/*eslint-disable */
switch (test) { case 'val': break; }
/*eslint-enable */

/*eslint-disable no-switch */
switch (test) { case 'val': break; }
/*eslint-enable no-switch */

switch (test) { case 'val': break; } // eslint-disable-line

switch (test) { case 'val': break; } // eslint-disable-line no-switch

Resources

• CodePen
• Addy Osmani's JavaScript Design Patterns eBook
• ESLint
• eslint-plugin-smells
• ES6 Scoping
• ES6 Symbols
• Learn ES6

Smelly Code

function Person() {
  this.teeth = [{ clean: false }, { clean: false }, { clean: false }];
};

Person.prototype.brush = function() {
  var that = this;

  this.teeth.forEach(function(tooth) {
    that.clean(tooth);
  });

  console.log('brushed');
};

Person.prototype.clean = function(tooth) {
  tooth.clean = true;
}

var person = new Person();
person.brush();
console.log(person.teeth);

Why Does This Smell?

that or self or selfie

Smelly Code

function Person() {
  this.teeth = [{ clean: false }, { clean: false }, { clean: false }];
};

Person.prototype.brush = function() {
  var that = this; // 6: Stink

  this.teeth.forEach(function(tooth) {
    that.clean(tooth); // 9: Stink
  });

  console.log('brushed');
};

Person.prototype.clean = function(tooth) {
  tooth.clean = true;
}

var person = new Person();
person.brush();
console.log(person.teeth);
 
 

Alternatives

1) bind

Person.prototype.brush = function() {
  this.teeth.forEach(function(tooth) {
    this.clean(tooth);
  }.bind(this)); // 4: Use .bind() to change context

  console.log('brushed');
};
 

Alternatives

2) 2nd parameter of forEach

Person.prototype.brush = function() {
  this.teeth.forEach(function(tooth) {
    this.clean(tooth);
  }, this); // 4: Use 2nd parameter of .forEach to change context

  console.log('brushed');
};
 

Alternatives

3) ECMAScript 2015 (ES6)

Person.prototype.brush = function() {
  this.teeth.forEach(tooth => { // 2: Use ES6 Arrow Function to bind `this`
    this.clean(tooth);
  });

  console.log('brushed');
};
 
 
 

4a) Functional Programming

Person.prototype.brush = function() {
  this.teeth.forEach(this.clean); // 2: Use functional programming

  console.log('brushed');
};
 

4b) Functional Programming

Person.prototype.brush = function() {
  this.teeth.forEach(this.clean.bind(this)); // 2: Bind `this` if clean needs it

  console.log('brushed');
};
 

Tooling

ESLint

• no-this-assign
  (eslint-plugin-smells)
• consistent-this
• no-extra-bind

Resources

• CodePen

Smelly Code

var build = function(id, href, text) {
  return $( "<div id='tab'><a href='" + href + "' id='" + id + "'>" +
    text + "</a></div>" );
}

Why Does This Smell?

Alternatives

1) Tweet Sized JavaScript Templating Engine by @thomasfuchs

function t(s, d) {
  for (var p in d)
    s = s.replace(new RegExp('{' + p + '}', 'g'), d[p]);
  return s;
}

var build = function(id, href, text) {
  var options = {
    id: id,
    href: href,
    text: text
  };

  return t('<div id="tab"><a href="{href}" id="{id}">{text}</></div>', options);
}

Alternatives

2) ECMAScript 2015 (ES6) Template Strings

var build = (id, href, text) =>
  `<div id="tab"><a href="${href}" id="${id}">${text}</a></div>`;

Alternatives

3) ECMAScript 2015 (ES6) Template Strings (Multiline)

var build = (id, href, text) => `<div id="tab">
  <a href="${href}" id="${id}">${text}</a>
</div>`;

Alternatives

4a) ECMAScript 2015 (ES6) Tagged Template Strings to Protect Against XSS (Cross Site Scripting)

var build = (id, href, text) => SanitizeHTML`<div id="tab">
  <a href="${href}" id="${id}">${text}</a>
</div>`;

Alternatives

4b) ECMAScript 2015 (ES6) Tagged Template Strings

function SanitizeHTML(literals, ...values) {
  let string = '';

  values.forEach((value, i) => {
    value = String(value);
    string += literals[i];
    string += value.replace(/&/g, '&')
      .replace(/</g, '<')
      .replace(/>/g, '>');
  });
  string += literals[literals.length - 1];

  return string;
}

Alternatives

4) Other micro-libraries or larger libraries/frameworks

• Lowdash or Underscore
• Angular
• React
• Ember
• etc...

Tooling

eslint-plugin-smells

• no-complex-string-concat

Resources

• Tweet Sized JavaScript Templating Engine by @thomasfuchs
• Learn ECMAScript 2015 (ES6) - http://babeljs.io/docs/learn-es6/

Smelly Code

$(document).ready(function() {
  $('.Component')
    .find('button')
      .addClass('Component-button--action')
      .click(function() { alert('HEY!'); })
    .end()
    .mouseenter(function() { $(this).addClass('Component--over'); })
    .mouseleave(function() { $(this).removeClass('Component--over'); })
    .addClass('initialized');
});

Why Does This Smell?

Extreme Chaining

So What!?!

Let's Refactor

Refactor

// Event Delegation before DOM Ready
$(document).on('mouseenter mouseleave', '.Component', function(e) {
  $(this).toggleClass('Component--over', e.type === 'mouseenter');  
});

$(document).on('click', '.Component button', function(e) {
  alert('HEY!');
});

$(document).ready(function() {
  $('.Component button').addClass('Component-button--action');
});

Demo

Tooling

eslint-plugin-smells

• no-complex-chaining

Resources

• CodePen

Smelly Code

setInterval(function() {
  console.log('start setInterval');
  someLongProcess(getRandomInt(2000, 4000));
}, 3000);

function someLongProcess(duration) {
  setTimeout(
    function() { console.log('long process: ' + duration); },
    duration
  );  
}

function getRandomInt(min, max) {
  return Math.floor(Math.random() * (max - min + 1)) + min;
}

Why Does This Smell?

Out of Sync Timer

Demo: setInternval

So What!?!

setTimeout

setTimeout

setTimeout(function timer() {
  console.log('start setTimeout')
  someLongProcess(getRandomInt(2000, 4000), function() {
    setTimeout(timer, 3000);
  });
}, 3000);

function someLongProcess(duration, callback) {
  setTimeout(function() {
    console.log('long process: ' + duration);
    callback();
  }, duration);  
}

/* getRandomInt(min, max) {} */

Demo: setTimeout

Resources

• CodePen

Smelly Code

data = this.appendAnalyticsData(data);
data = this.appendSubmissionData(data);
data = this.appendAdditionalInputs(data);
data = this.pruneObject(data);

Why Does This Smell?

A Bit Repetitive

Alternatives

1) Nested Function Calls

data = this.pruneObject(
  this.appendAdditionalInputs(
    this.appendSubmissionData(
      this.appendAnalyticsData(data)
    )
  )
);

Alternatives

2) forEach

var funcs = [
  this.appendAnalyticsData,
  this.appendSubmissionData,
  this.appendAdditionalInputs,
  this.pruneObject
];

funcs.forEach(function(func) {
  data = func(data);
});

Alternatives

3) reduce

var funcs = [
  this.appendAnalyticsData,
  this.appendSubmissionData,
  this.appendAdditionalInputs,
  this.pruneObject
];

data = funcs.reduce(function(memo, func) {
  return func(memo);
}, data);

Alternatives

4) flow

data = _.flow(
  this.appendAnalyticsData,
  this.appendSubmissionData,
  this.appendAdditionalInputs,
  this.pruneObject
)(data);

Tooling

eslint-plugin-smells

• no-reassign

Resources

• forEach
• reduce
• _.flow

Smelly Code

function ShoppingCart() { this.items = []; }
ShoppingCart.prototype.addItem = function(item) {
  this.items.push(item);
};

function Product(name) { this.name = name; }
Product.prototype.addToCart = function() {
  shoppingCart.addItem(this);
};

var shoppingCart = new ShoppingCart();
var product = new Product('Socks');
product.addToCart();
console.log(shoppingCart.items);

Why Does This Smell?

Tightly Coupled Dependencies

Smelly Code

function ShoppingCart() { this.items = []; }
ShoppingCart.prototype.addItem = function(item) {
  this.items.push(item);
};

function Product(name) { this.name = name; }
Product.prototype.addToCart = function() {
  shoppingCart.addItem(this); // 8: Inappropriate
};

var shoppingCart = new ShoppingCart();
var product = new Product('Socks');
product.addToCart();
console.log(shoppingCart.items);
 

So What!?!

1. Dependency Injection
2. Message Broker

1. Dependency Injection

function ShoppingCart() { this.items = []; }
ShoppingCart.prototype.addItem = function(item) {
  this.items.push(item);
};

function Product(name, shoppingCart) { // 6: Accept Dependency
  this.name = name;
  this.shoppingCart = shoppingCart; // 8: Save off Dependency
}
Product.prototype.addToCart = function() {
  this.shoppingCart.addItem(this);
};

var shoppingCart = new ShoppingCart();
var product = new Product('Socks', shoppingCart); // 15: Pass in Dependency
product.addToCart();
console.log(shoppingCart.items);
 
 
 

2. Message Broker

var channel = postal.channel(); // 1: Broker

function ShoppingCart() {
  this.items = [];
  channel.subscribe('shoppingcart.add', this.addItem); // 5: Listen to Message
}
ShoppingCart.prototype.addItem = function(item) {
  this.items.push(item);
};

function Product(name) { this.name = name; }
Product.prototype.addToCart = function() {
  channel.publish('shoppingcart.add', this); // 13: Publish Message
};

var shoppingCart = new ShoppingCart();
var product = new Product('Socks');
product.addToCart();
console.log(shoppingCart.items);
 
 
 

Tooling

Fuzzy

(Subjective)

Resources

• postal by @ifandelse

Smelly Code

var search = document.querySelector('.Autocomplete');

search.addEventListener('input', function(e) {
  // Make Ajax call for autocomplete

  console.log(e.target.value);
});

Why Does This Smell?

Way Too Chatty

So What!?!

Throttle

Throttle

var search = document.querySelector('.Autocomplete');

search.addEventListener('input', _.throttle(function(e) {
  // Make Ajax call for autocomplete

  console.log(e.target.value);
}, 500));

So What!?!

Debounce

Debounce

var search = document.querySelector('.Autocomplete');

search.addEventListener('input', _.debounce(function(e) {
  // Make Ajax call for autocomplete

  console.log(e.target.value);
}, 500));

Resources

• lodash
• throttle
• debounce
• CodePen

Smelly Code

var search = document.querySelector('.Autocomplete');

search.addEventListener('input', _.debounce(function(e) {
  // Make Ajax call for autocomplete

  console.log(e.target.value);
}, 500));

Why Does This Smell?

Anonymous Functions

Reasons to Name Functions

1. Stack Trace
2. Dereferencing
3. Code Reuse

1. Stack Trace

Where is it?

var search = document.querySelector('.Autocomplete');

search.addEventListener('input', function(e) {
  console.log(e.target.value);
});

1. Stack Trace

There it is!

var search = document.querySelector('.Autocomplete');

search.addEventListener('input', function(e) {
  console.log(e.target.value);
});

1. Stack Trace

After

var search = document.querySelector('.Autocomplete');

search.addEventListener('input', function matches(e) {
  console.log(e.target.value);
});

1. Stack Trace

debounce

var search = document.querySelector('.Autocomplete');

search.addEventListener('input', _.debounce(function matches(e) {
  console.log(e.target.value);
}, 500));

1. Stack Trace

debounce (Heavy Bottom Up)

var search = document.querySelector('.Autocomplete');

search.addEventListener('input', _.debounce(function matches(e) {
  console.log(e.target.value);
}, 500));

2. Dereferencing

One-time Event Handler

document.querySelector('button')
  .addEventListener('click', function handler() {
      alert('Ka-boom!');
    this.removeEventListener('click', handler);
  });

3. Code Reuse

var kaboom = function() { alert('Ka-boom'); };

document.querySelector('button').addEventListener('click', kaboom);

document.querySelector('#egg').addEventListener('mouseenter', kaboom);

Resource

• Chrome DevTools

Smelly Code

$(document).ready(function() {
  // wire up event handlers

  // declare all the things

  // etc...
});

Why Does This Smell?

Makes it Hard to Unit Test

Singleton Module

(function(myApp) {
  myApp.init = function() {
    // kick off your code
  };

  myApp.handleClick = function() {}; // etc...
}(window.myApp = window.myApp || {}));

// Only include at end of main application...
$(document).ready(function() {
  window.myApp.init();
});

Constructor Function

var Application = (function() {
  function Application() {
    // kick off your code
  }

  Application.prototype.handleClick = function() {};

  return Application;
}());

// Only include at end of main application...
$(document).ready(function() {
  new Application();
});

Smelly Code

"...you can set the directionality of it to be 2-Way Data Binding. That actually seems to be a good idea until you have a large scale application and then it turns out you have no idea whats going on... and turns out to be an anti-pattern for large apps." --Misko Hevery https://www.youtube.com/watch?v=uD6Okha_Yj0#t=1785

Why Does This Smell?

Hard to Track Execution & Data Flow

React Flux Architecture

Resources

• React
• React Flux
• An Angular2 Todo App: First look at App Development in Angular2