This article lists common bad code smell in JavaScript code, such as bidirectional data binding temporary timers, bidirectional data binding pits, complex branching statements, duplicate assignments, etc., and analyzes them such as live restore, bad code review, problem diagnosis and identification (via ESlint or other tools), code refactoring schemes, Gives first hand experience on how to write good code ~

It’s so convoluted, it’s crazy

Problem site:

If the word begins with a consonant (or consonant set), move the rest of its steps to the front and add “ay” (pig -> igpay). If the word begins with a vowel sound, keep the order but add “way” to the end (egg->eggway, etc.)

Bad 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; }Copy the code

What’s wrong:

Problem detected:

Lint configuration items such as maximum number of statements, complexity, maximum number of nested statements, maximum length, maximum number of arguments, maximum number of nested callbacks

/*jshint maxstatements:15, maxdepth:2, maxcomplexity:5 */
/*eslint max-statements:[2, 15], max-depth:[1, 2], complexity:[2, 5] */Copy the code

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).Copy the code

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'); }); }); });Copy the code

Refactored code:

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;
}Copy the code

Data comparison:

Max-statements: 16 → 6 Max-Depth: 5 → 2 Complexity: 7 → 3 Max-Len: 65 → 73 Max-params: 1 → 2 Max-nested-callbacks: 0 → 1

Related resources:

Jshint-jshint.com/ eslint-eslint.org/ jscomplexity – jscomplexity.org/ escomplex – github.com/philbooth/e… jasmine – jasmine.github.io/

Paste –

Problem site:

We need to achieve the following effect



Bad code:

var boxes = document.querySelectorAll('.Box');

[].forEach.call(boxes, function(element, index) {
  element.innerText = "Box: " + index;
  element.style.backgroundColor =
    '#' + (Math.random() * 0xFFFFFF << 0).toString(16);
});

var circles = document.querySelectorAll(".Circle");

[].forEach.call(circles, function(element, index) {
  element.innerText = "Circle: " + index;
  element.style.color =
    '#' + (Math.random() * 0xFFFFFF << 0).toString(16);
});
Copy the code

What went wrong:

Because we are paste copy!!

Problem detected:

Check out paste copy and structurally similar code snippets – jsinspect github.com/danielstjul…



Find pasted copy sections from your JS, TypeScript, C#, Ruby, CSS, HTML source code – JSCPD github.com/kucherenko/…



Refactored code:

  • Let’s pull out the random color portion…
  • Let’s pull out the weird [].forEach.call portion…
  • Let’s try to go further…
let randomColor = () => `#${(Math.random() * 0xFFFFFF << 0).toString(16)};

let $ = selector => [].slice.call(document.querySelectorAll(selector || '*'));

let updateElement = (selector, textPrefix, styleProperty) => {
  $(selector).forEach((element, index) => {
    element.innerText = textPrefix + ': ' + index;
    element.style[styleProperty] = randomColor();
  });
}

updateElement('.Box', 'Box', 'backgroundColor'); // 12: Refactored

updateElement('.Circle', 'Circle', 'color'); // 14: Refactored
Copy the code

Complex branch statements

Bad 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');Copy the code

What went wrong:

Violate the Open /close principle:

Software elements (classes, modules, methods, etc.) should be easy to open for extension, but not more than their own modifications. The code itself can allow its behavior to be extended, but do not modify the source code

You can use things like checking: no-switch – disallow the use of the switch statement no-complex-switch-case – disallow use of complex switch statements

Refactored code:

Adding code at this point is not the same as the original switch, until it is long and smelly, and easy to break the previous code logic.

(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'); // 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 || {}));Copy the code

Bad taste of magic numbers/strings

Bad code:

So Magic Strings, as you saw above, are special Strings for things like Triangle and Square.

What went wrong:

These magic numbers and strings are written directly into the code and are not easy to modify or read. Insert password.length > 9, where 9 refers to MAX_PASSWORD_SIZE. At the same time, if the judgment rule is needed in more than one place, you can avoid changing the number like 9 multiple times

En.wikipedia.org/wiki/Magic_… Stackoverflow.com/questions/4…

Refactored code:

1 Passing objects

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:Copy the code

2 Using const and symbols

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

function getArea(shape, options) {
  var area = 0;
  switch (shape) {
    case shapeType.triangle: // 8: Enum-ishCopy the code

Code the deep

Bad code:

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

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

  console.log('brushed');
};
Copy the code

What went wrong:

Weird self /that/_this etc

Use esLint:

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

Refactored code:

Bind, 2nd parameter of forEach, es6

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

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');
};

Person.prototype.brush = function() {
  this.teeth.forEach(tooth => { // 2: Use ES6 Arrow Function to bind `this`
    this.clean(tooth);
  });
  console.log('brushed');
};Copy the code

Brittle character concatenation

Bad code:

var build = function(id, href, text) {
  return $( "
        
 + href + "' id='" + id + "'>" +
    text + "" );
}Copy the code

What went wrong:

The code is ugly, verbose, and unintuitive.

Many tools and frameworks that use ES6’s template strings (string interpolation and multi-line) also provide support for responses, such as Lodash /underscore, Angular, React, etc

Refactored code:

var build = (id, href, text) =>
  `
        

         ${text}
        
`;

var build = (id, href, text) => `
        

  ${text}
`;Copy the code

JQuery asked

Bad 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'); });Copy the code

What went wrong:

Too many chain calls

Refactored code:

// 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'); });Copy the code

Temporary timer

Bad 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;
}Copy the code

What went wrong:

Out of sync timer could not confirm timing and execution

Refactored code:

Call someLongProcess (long Process: Random time), then the setInterval fn (which is doing the long process) is iterated, or setTimeout(timer,) is passed through the callback repeatedly.

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) {} */Copy the code

Repeat the assignment

Bad code:

data = this.appendAnalyticsData(data);
data = this.appendSubmissionData(data);
data = this.appendAdditionalInputs(data);
data = this.pruneObject(data);Copy the code

What went wrong:

It’s repetitive and wordy

eslint-plugin-smells

Refactored code:

1 Nested function calls 2 forEach 3 Reduce 4 flow

// 1 data = this.pruneObject( this.appendAdditionalInputs( this.appendSubmissionData( this.appendAnalyticsData(data) ) ) ); // 2 var funcs = [ this.appendAnalyticsData, this.appendSubmissionData, this.appendAdditionalInputs, this.pruneObject ];  funcs.forEach(function(func) { data = func(data); }); Funcs = funcs.reduce(function(memo, func) {return func(memo); }, data); // 4 data = _.flow( this.appendAnalyticsData, this.appendSubmissionData, this.appendAdditionalInputs, this.pruneObject )(data);Copy the code

Bad information

Bad 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);Copy the code

What went wrong:

Dependencies are tightly coupled calling each other, coupling! Such as the relationship between Product and shoppingCart

Refactored code:

1 Dependency injection 2 Message broker

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);Copy the code

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);Copy the code

Continuous interaction calls

Bad code:

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

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

  console.log(e.target.value);
});Copy the code

What went wrong:

It can cause lag, redundant calculations, etc

Refactored code:

Throttle and debounce


var search = document.querySelector('.Autocomplete');
search.addEventListener('input', _.throttle(function(e) {
  // Make Ajax call for autocomplete
  console.log(e.target.value);
}, 500));

var search = document.querySelector('.Autocomplete');
search.addEventListener('input', _.debounce(function(e) {
  // Make Ajax call for autocomplete
  console.log(e.target.value);
}, 500));Copy the code

Anonymous algorithm

Bad code:

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

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

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

What went wrong:

Anonymous functions are a good thing, but naming functions can help us:

  • Stack Trace (with Devtools and easy debug)
  • Dereferencing
  • Code Reuse

Refactored code:

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

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

Undefined execution

Bad code:

Specify the trigger time instead of domReady

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

  // declare all the things

  // etc...
});Copy the code

What went wrong:

It’s hard to unit test

Refactored code:

Use the singleton module, plus the constructor function singleton pattern (singleton has an init method to Kick off) & constructor (new Application() -> Kick off your code in the original constructor!)

(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(); }); 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(); });Copy the code

Two-way data binding

Bad code:

Take a look at any MVVM project you have (Angular, etc.)

What went wrong:

It’s Hard to track execution and data flow. (It’s also where Angular1.x gets a lot of ridicule, and React Flux.)

Refactored code:

Flux (action, dispatcher, store->view)

React Flux

An Angular2 Todo App: First look at App Development in Angular2

conclusion

For more Lint rules, find the ESlint-plugin on NPM

Reference