Skip to content

Commit

Permalink
refactor: Declare vars when used, streamline code (#1588)
Browse files Browse the repository at this point in the history 
* Remove unnecessary branches
* Improve comments
* Declare vars when used
* Use `function` statements
* Use `domEach` where appropriate
* Disallow shadowing, use before define
  • Loading branch information
@fb55
fb55 committed Dec 24, 2020
1 parent 6a90bda commit 69ae308
Show file tree
Hide file tree
Showing 15 changed files with 204 additions and 325 deletions.
2 changes: 2 additions & 0 deletions .eslintrc.json
View file
Expand Up @@ -11,6 +11,8 @@
"curly": [2, "multi-line"],
"one-var": [2, "never"],
"no-else-return": 2,
"no-shadow": 2,
"no-use-before-define": [2, "nofunc"],

"jsdoc/check-param-names": 2,
"jsdoc/check-tag-names": 2,
Expand Down
18 changes: 6 additions & 12 deletions benchmark/suite.js
View file
Expand Up @@ -25,13 +25,11 @@ Suites.prototype.cheerioOnly = function () {
};

Suites.prototype.add = function (name, fileName, options) {
var markup;
var suite;
if (!filterRe.test(name)) {
return;
}
markup = fs.readFileSync(path.join(documentDir, fileName), 'utf8');
suite = new Benchmark.Suite(name);
var markup = fs.readFileSync(path.join(documentDir, fileName), 'utf8');
var suite = new Benchmark.Suite(name);

suite.on('start', function () {
console.log('Test: ' + name + ' (file: ' + fileName + ')');
Expand Down Expand Up @@ -70,10 +68,8 @@ Suites.prototype._benchJsDom = function (suite, markup, options) {

jQueryScript.runInContext(dom.getInternalVMContext());

var setupData;
if (options.setup) {
setupData = options.setup.call(null, dom.window.$);
}
var setupData = options.setup && options.setup.call(null, dom.window.$);

suite.add('jsdom', function () {
testFn.call(null, dom.window.$, setupData);
});
Expand All @@ -83,10 +79,8 @@ Suites.prototype._benchJsDom = function (suite, markup, options) {
Suites.prototype._benchCheerio = function (suite, markup, options) {
var $ = cheerio.load(markup);
var testFn = options.test;
var setupData;
if (options.setup) {
setupData = options.setup.call(null, $);
}
var setupData = options.setup && options.setup.call(null, $);

suite.add('cheerio', function () {
testFn.call(null, $, setupData);
});
Expand Down
132 changes: 51 additions & 81 deletions lib/api/attributes.js
View file
Expand Up @@ -25,7 +25,7 @@ var rboolean = /^(?:autofocus|autoplay|async|checked|controls|defer|disabled|hid
// Matches strings that look like JSON objects or arrays
var rbrace = /^(?:\{[\w\W]*\}|\[[\w\W]*\])$/;

var getAttr = function (elem, name) {
function getAttr(elem, name) {
if (!elem || !isTag(elem)) return;

if (!elem.attribs) {
Expand Down Expand Up @@ -55,15 +55,15 @@ var getAttr = function (elem, name) {
) {
return 'on';
}
};
}

var setAttr = function (el, name, value) {
function setAttr(el, name, value) {
if (value === null) {
removeAttribute(el, name);
} else {
el.attribs[name] = value + '';
}
};
}

/**
* Method for getting and setting attributes. Gets the attribute value for only
Expand Down Expand Up @@ -109,23 +109,23 @@ exports.attr = function (name, value) {
return getAttr(this[0], name);
};

var getProp = function (el, name) {
function getProp(el, name) {
if (!el || !isTag(el)) return;

return name in el
? el[name]
: rboolean.test(name)
? getAttr(el, name) !== undefined
: getAttr(el, name);
};
}

var setProp = function (el, name, value) {
function setProp(el, name, value) {
if (name in el) {
el[name] = value;
} else {
setAttr(el, name, rboolean.test(name) ? (value ? '' : null) : value);
}
};
}

/**
* Method for getting and setting properties. Gets the property value for only
Expand All @@ -145,21 +145,19 @@ var setProp = function (el, name, value) {
* @see {@link http://api.jquery.com/prop/}
*/
exports.prop = function (name, value) {
var i = 0;
var property;

if (typeof name === 'string' && value === undefined) {
switch (name) {
case 'style':
property = this.css();

Object.keys(property).forEach(function (p) {
property[i++] = p;
case 'style': {
var property = this.css();
var keys = Object.keys(property);
keys.forEach(function (p, i) {
property[i] = p;
});

property.length = i;
property.length = keys.length;

break;
}
case 'tagName':
case 'nodeName':
property = this[0].name.toUpperCase();
Expand Down Expand Up @@ -199,7 +197,7 @@ exports.prop = function (name, value) {
}
};

var setData = function (el, name, value) {
function setData(el, name, value) {
if (!el.data) {
el.data = {};
}
Expand All @@ -208,21 +206,16 @@ var setData = function (el, name, value) {
if (typeof name === 'string' && value !== undefined) {
el.data[name] = value;
}
};
}

// Read the specified attribute from the equivalent HTML5 `data-*` attribute,
// and (if present) cache the value in the node's internal data store. If no
// attribute name is specified, read *all* HTML5 `data-*` attributes in this
// manner.
var readData = function (el, name) {
function readData(el, name) {
var readAll = arguments.length === 1;
var domNames;
var domName;
var jsNames;
var jsName;
var value;
var idx;
var length;

if (readAll) {
domNames = Object.keys(el.attribs).filter(function (attrName) {
Expand All @@ -236,9 +229,10 @@ var readData = function (el, name) {
jsNames = [name];
}

for (idx = 0, length = domNames.length; idx < length; ++idx) {
domName = domNames[idx];
jsName = jsNames[idx];
for (var idx = 0; idx < domNames.length; ++idx) {
var domName = domNames[idx];
var jsName = jsNames[idx];
var value;
if (hasOwn.call(el.attribs, domName) && !hasOwn.call(el.data, jsName)) {
value = el.attribs[domName];

Expand All @@ -259,7 +253,7 @@ var readData = function (el, name) {
}

return readAll ? el.data : value;
};
}

/**
* Method for getting and setting data attributes. Gets or sets the data
Expand Down Expand Up @@ -334,20 +328,11 @@ exports.val = function (value) {
switch (element.name) {
case 'textarea':
return this.text(value);
case 'input':
if (querying) {
return this.attr('value');
}

this.attr('value', value);
return this;

case 'select':
case 'select': {
var option = this.find('option:selected');
var returnValue;
if (option === undefined) return undefined;
if (!option) return undefined;
if (!querying) {
if (!hasOwn.call(this.attr(), 'multiple') && typeof value == 'object') {
if (this.attr('multiple') == null && typeof value == 'object') {
return this;
}
if (typeof value != 'object') {
Expand All @@ -359,20 +344,16 @@ exports.val = function (value) {
}
return this;
}
returnValue = option.attr('value');
if (hasOwn.call(this.attr(), 'multiple')) {
returnValue = [];
domEach(option, function (__, el) {
returnValue.push(getAttr(el, 'value'));
});
}
return returnValue;

return this.attr('multiple')
? option.toArray().map(function (el) {
return getAttr(el, 'value');
})
: option.attr('value');
}
case 'input':
case 'option':
if (!querying) {
this.attr('value', value);
return this;
}
return this.attr('value');
return querying ? this.attr('value') : this.attr('value', value);
}
};

Expand All @@ -383,11 +364,11 @@ exports.val = function (value) {
* @param {node} elem - Node to remove attribute from.
* @param {string} name - Name of the attribute to remove.
*/
var removeAttribute = function (elem, name) {
function removeAttribute(elem, name) {
if (!elem.attribs || !hasOwn.call(elem.attribs, name)) return;

delete elem.attribs[name];
};
}

/**
* Splits a space-separated list of names to individual
Expand All @@ -396,9 +377,9 @@ var removeAttribute = function (elem, name) {
* @param {string} names - Names to split.
* @returns {string[]} - Split names.
*/
var splitNames = function (names) {
function splitNames(names) {
return names ? names.trim().split(rspace) : [];
};
}

/**
* Method for removing attributes by `name`.
Expand Down Expand Up @@ -443,19 +424,18 @@ exports.removeAttr = function (name) {
* //=> true
*
* @param {string} className - Name of the class.
* @returns {boolean}
*
* @see {@link http://api.jquery.com/hasClass/}
*/
exports.hasClass = function (className) {
return this.toArray().some(function (elem) {
var attrs = elem.attribs;
var clazz = attrs && attrs['class'];
var clazz = elem.attribs && elem.attribs['class'];
var idx = -1;
var end;

if (clazz && className.length) {
while ((idx = clazz.indexOf(className, idx + 1)) > -1) {
end = idx + className.length;
var end = idx + className.length;

if (
(idx === 0 || rspace.test(clazz[idx - 1])) &&
Expand Down Expand Up @@ -505,17 +485,14 @@ exports.addClass = function (value) {

// If we don't already have classes
var className = getAttr(this[i], 'class');
var numClasses;
var setClass;

if (!className) {
setAttr(this[i], 'class', classNames.join(' ').trim());
} else {
setClass = ' ' + className + ' ';
numClasses = classNames.length;
var setClass = ' ' + className + ' ';

// Check if class already exists
for (var j = 0; j < numClasses; j++) {
for (var j = 0; j < classNames.length; j++) {
var appendClass = classNames[j] + ' ';
if (setClass.indexOf(' ' + appendClass) < 0) setClass += appendClass;
}
Expand Down Expand Up @@ -544,10 +521,6 @@ exports.addClass = function (value) {
* @see {@link http://api.jquery.com/removeClass/}
*/
exports.removeClass = function (value) {
var classes;
var numClasses;
var removeAll;

// Handle if value is a function
if (typeof value === 'function') {
return domEach(this, function (i, el) {
Expand All @@ -558,9 +531,9 @@ exports.removeClass = function (value) {
});
}

classes = splitNames(value);
numClasses = classes.length;
removeAll = arguments.length === 0;
var classes = splitNames(value);
var numClasses = classes.length;
var removeAll = arguments.length === 0;

return domEach(this, function (i, el) {
if (!isTag(el)) return;
Expand All @@ -570,11 +543,10 @@ exports.removeClass = function (value) {
el.attribs.class = '';
} else {
var elClasses = splitNames(el.attribs.class);
var index;
var changed;
var changed = false;

for (var j = 0; j < numClasses; j++) {
index = elClasses.indexOf(classes[j]);
var index = elClasses.indexOf(classes[j]);

if (index >= 0) {
elClasses.splice(index, 1);
Expand Down Expand Up @@ -629,19 +601,17 @@ exports.toggleClass = function (value, stateVal) {
var numClasses = classNames.length;
var state = typeof stateVal === 'boolean' ? (stateVal ? 1 : -1) : 0;
var numElements = this.length;
var elementClasses;
var index;

for (var i = 0; i < numElements; i++) {
// If selected element isn't a tag, move on
if (!isTag(this[i])) continue;

elementClasses = splitNames(this[i].attribs.class);
var elementClasses = splitNames(this[i].attribs.class);

// Check if class already exists
for (var j = 0; j < numClasses; j++) {
// Check if the class name is currently defined
index = elementClasses.indexOf(classNames[j]);
var index = elementClasses.indexOf(classNames[j]);

// Add if stateValue === true or we are toggling and there is no value
if (state >= 0 && index < 0) {
Expand Down
3 changes: 1 addition & 2 deletions lib/api/forms.js
View file
Expand Up @@ -38,8 +38,7 @@ exports.serialize = function () {
exports.serializeArray = function () {
// Resolve all form elements from either forms or collections of form elements
var Cheerio = this.constructor;
return this.map(function () {
var elem = this;
return this.map(function (i, elem) {
var $elem = Cheerio(elem);
if (elem.name === 'form') {
return $elem.find(submittableSelector).toArray();
Expand Down

0 comments on commit 69ae308

Please sign in to comment.