Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2016/03/31 针针见血:怎么消除JavaScript中的代码坏味道 #5

Open
sivagao opened this issue Mar 31, 2016 · 9 comments

Comments

Projects
None yet
7 participants
@sivagao
Copy link
Owner

commented Mar 31, 2016

本文罗列JavaScript代码中常见的代码坏味道,如临时定时器,双向数据绑定的坑,复杂的分支语句,重复赋值等,对它们进行分析如现场还原,糟糕代码回顾,问题诊断和识别(通过ESlint或其他工具),代码重构方案,给出了怎么写好代码的一手经验~

绕来绕去,很烧脑

问题现场:

如果单词以辅音开头(或辅音集),把它剩余的步伐移到前面,并且添加上『ay』如pig -> igpay
如果单词以元音开头,保持顺序但是在结尾加上『way』如,egg->eggway等

糟糕代码:

/* 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;
}

问题在哪:

  • 太多语句
  • 太多嵌套
  • 太高复杂度

检测出问题:

关于Lint的配置项:如最大语句数,复杂度,最大嵌套数,最大长度,最多传参,最多嵌套回调

/*jshint maxstatements:15, maxdepth:2, maxcomplexity:5 */
/*eslint max-statements:[2, 15], max-depth:[1, 2], complexity:[2, 5] */
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).

测试先行:

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

重构后代码:

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.includes(word[0]);
let startsWithConsonant = word => CONSONANTS.includes(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;
}

数据对比:

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

相关资源:

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

粘贴复制

问题现场:

我们需要实现如下的效果

糟糕的代码:

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

问题出在哪:

因为我们在粘贴复制!!

检测出问题:

检查出粘贴复制和结构类似的代码片段 - jsinspect
https://github.com/danielstjules

从你的JS,TypeScript,C#,Ruby,CSS,HTML等源代码中找到粘贴复制的部分 - JSCPD
https://github.com/kucherenko/jscpd

重构后代码:

  • 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

复杂的分支语句

糟糕的代码:

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

问题出在哪:

违反了 open/close 原则:

软件元素(类,模块和方法等)应该易于被打开扩展,但是除了本身不要多于的修改。既代码本身可以允许它的行为被扩展,但是不要修改源代码

可以使用诸如检查:
no-switch - disallow the use of the switch statement
no-complex-switch-case - disallow use of complex switch statements

重构后代码:

这时候添加一个代码就不像之前那样该原先的switch,直到它又长又臭,还容易把之前的代码逻辑broken掉。

(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 || {}));

魔法数字/字符串的坏味道

糟糕代码:

如上面看到的,如Magic Strings,对于诸如Triangle,Square这些就是特殊字符串。

问题出在哪:

这些魔法数字和字符串是直接写死在代码中,不容易修改和阅读。注入password.length > 9,这里面的9是指 MAX_PASSWORD_SIZE ,这样先定义后使用更清晰。同时如果多个地方需要这个判断规则,也可以避免多次修改类似9这样的数字

https://en.wikipedia.org/wiki/Magic_number_(programming)
http://stackoverflow.com/questions/47882/what-is-a-magic-number-and-why-is-it-bad

重构后代码:

1 通过对象

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: 

2 通过 const 和 symbols

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

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

代码深渊

糟糕代码:

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

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

  console.log('brushed');
};

问题出在哪:

奇奇怪怪的 self /that/_this 等

使用一下的eslint:

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

重构后代码:

利用Function.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');
};

脆裂的字符拼接

糟糕代码:

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

问题出在哪:

代码很丑陋,也很啰嗦,不直观。

使用 ES6的模板字符串(字符串插值和多行)
很多工具和框架也都提供了响应的支持,如lodash/underscore,angular,react 等

  • no-complex-string-concat

重构后代码:

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

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

jQuery询问

糟糕代码:

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

问题出在哪:

太多的链式调用

重构后代码:

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

临时定时器

糟糕代码:

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

问题出在哪:

out of sync timer 不能确认时序和执行

重构后代码:

等 3s 去执行timer(setTimeout),然后调用 someLongProcess (long process: random time ),接着在循环
使用setInterval fn(其中在进行 long process),还是通过callback传递来反复setTimeout(timer, )

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

重复赋值

糟糕代码:

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

问题出在哪:

有些重复和啰嗦

eslint-plugin-smells

  • no-reassign

重构后代码:

1 嵌套的函数调用
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);
});


// 3
// funcs 定义如上
data = funcs.reduce(function(memo, func) {
  return func(memo);
}, data);

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

不合理的情报

糟糕代码:

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

问题出在哪:

依赖被紧紧的耦合了
相互调用,耦合!如 product 和 shoppingCart 关系

重构后代码:

1 dependency injection 依赖注入
2 消息经纪人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);
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);

不断的交互调用

糟糕代码:

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

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

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

问题出在哪:

会造成卡顿,多余的计算等

重构后代码:

throttle 和 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));

匿名算法

糟糕代码:

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

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

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

问题出在哪:

匿名函数是个好东西,但是给函数命名可以帮助我们:

  • Stack Trace(结合Devtools,跟容易debug)
  • Dereferencing
  • Code Reuse

重构后代码:

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

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

未明确的执行

糟糕代码:

明确触发时机而不是,写在 domready

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

  // declare all the things

  // etc...
});

问题出在哪:

很难做单元测试

重构后代码:

利用单例模块,加上构建器函数
单例模式(单例有个init 方法,来Kick off) & 构造函数(new Application() -> 在原来的构造函数中Kick off your code!)

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

双向数据绑定

糟糕代码:

随便看看你们手头的MVVM项目(如Angular的等)

问题出在哪:

很难定位执行顺序和数据流(Hard to track execution & data flow )
(也是Angular1.x被大力吐槽的地方,被React的Flux)

重构后代码:

方案: flux(action, dispatcher, store->view)

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

结语

更多的lint规则,在npm上搜索 eslint-plugin 查找

Reference

@sivagao sivagao changed the title 2016/03/31 针针见血,怎么消除JavaScript中的代码坏味道(1) 2016/03/31 针针见血,怎么消除JavaScript中的代码坏味道 Mar 31, 2016

@sivagao sivagao changed the title 2016/03/31 针针见血,怎么消除JavaScript中的代码坏味道 2016/03/31 针针见血:怎么消除JavaScript中的代码坏味道 Mar 31, 2016

@iamcc

This comment has been minimized.

Copy link

commented Apr 1, 2016

第一个例子是不是有误?
元音开头的单词ENDING是way

@susan-github

This comment has been minimized.

Copy link

commented Apr 1, 2016

肿么感觉,看着糟糕代码都是我会写的,心好累

@SeanKChan

This comment has been minimized.

Copy link

commented Apr 2, 2016

我也是这种感觉,槽糕的代码都是我现在在写的

@flyinhigh

This comment has been minimized.

Copy link

commented Apr 19, 2016

#${(Math.random() * 0xFFFFFF << 0).toString(16)};前面的`#$ 是啥意思,只有我觉得这是很装逼的写法吗。

@sivagao

This comment has been minimized.

Copy link
Owner Author

commented Apr 20, 2016

@flyinhigh 哈哈,前面那个其实是包含 es6 的 template string 的语法(内插值)啦,去了解下就知道了~

@BuptStEve

This comment has been minimized.

Copy link

commented Apr 28, 2016

写得很棒~一点儿小意见...

  • 第一个例子既然用 const 了,可以用 includes 替代 indexOf...
  • 最后少了个 :let randomColor = () =>#${(Math.random() * 0xFFFFFF << 0).toString(16)};
@sivagao

This comment has been minimized.

Copy link
Owner Author

commented Apr 29, 2016

@BuptStEve 很仔细。用新的includes很好,来清除掉怪异的!!indexOf

@chengmu

This comment has been minimized.

Copy link

commented May 4, 2016

这个不是去年jsconf的js code smells的ppt吗? http://elijahmanor.com/javascript-smells/

是不是说明在文章开头原作者和来源好一点?

@sivagao

This comment has been minimized.

Copy link
Owner Author

commented May 4, 2016

@chengmu 说的很对,现在的文章都会在开头说明来源。老文章并没有这些意识 /(ㄒoㄒ)/~~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.