patternjavascriptMinor
Yet another JavaScript library
Viewed 0 times
yetjavascriptanotherlibrary
Problem
I have written myself a little JavaScript library for personal use, and anyone who'd like to use it for some reason. The reason why I decided to create my own library instead of using the already existing ones is that I have found out in the course of practice that I only use a very small amount of the functionality modern libraries offer. So I figured instead of loading big libraries and not using them, why not create a small one that fits my needs perfectly. However I am nowhere near a JavaScript expert and I have no clue whether what I have written is efficient at all. This is why I would like to ask you to review my little work of art and suggest any improvements.
Here's the very base of the library:
```
function MF(selector){
this.nodes = new Array();
if(!selector) return;
if(!(this instanceof MF)) return new MF(selector);
if(selector instanceof MF) return selector;
if (typeof selector === 'string') {
switch (selector.substring(0, 1)) {
case '#':
selector = document.getElementById(selector.substring(1));
if(selector){
this.nodes.push(selector);
}
break;
case '.':
this.nodes = Array.prototype.slice.call(document.getElementsByClassName(selector.substring(1).replace(/\./g, ' ')), 0);;
break;
default :
this.nodes = Array.prototype.slice.call(document.getElementsByTagName(selector), 0);
break;
}
} else if (selector.nodeType === 1 || selector === document || selector === window) {
this.nodes.push(selector);
} else if (selector instanceof HTMLCollection || selector instanceof NodeList) {
this.nodes = Array.prototype.slice.call(selector, 0);
}
};
MF.function = function(fname, fbody){
Object.defineProperty(MF.prototype, fname, {
value: function() {
var args = Array.prototype.slice.call(arguments,
Here's the very base of the library:
```
function MF(selector){
this.nodes = new Array();
if(!selector) return;
if(!(this instanceof MF)) return new MF(selector);
if(selector instanceof MF) return selector;
if (typeof selector === 'string') {
switch (selector.substring(0, 1)) {
case '#':
selector = document.getElementById(selector.substring(1));
if(selector){
this.nodes.push(selector);
}
break;
case '.':
this.nodes = Array.prototype.slice.call(document.getElementsByClassName(selector.substring(1).replace(/\./g, ' ')), 0);;
break;
default :
this.nodes = Array.prototype.slice.call(document.getElementsByTagName(selector), 0);
break;
}
} else if (selector.nodeType === 1 || selector === document || selector === window) {
this.nodes.push(selector);
} else if (selector instanceof HTMLCollection || selector instanceof NodeList) {
this.nodes = Array.prototype.slice.call(selector, 0);
}
};
MF.function = function(fname, fbody){
Object.defineProperty(MF.prototype, fname, {
value: function() {
var args = Array.prototype.slice.call(arguments,
Solution
Wrap your library code and use strict mode
Use [] instead of new Array();
It is shorter and more idiomatic in my opinion:
Don't reuse variable names for different contents
The obvious correction is to use a new variable, e.g.
Don't use too long lines
I'm not a fan of the 80 characters per line restriction, but 132 characters on a single line is too much in my eyes:
(BTW, in the line above there is also a superfluous semicolon.)
A possible replacement:
Also consider aliasing
Use constants, not their values
Consider using a available constant names (or make your owns):
It is now clearer and more readable.
Use meaningful variable names
var args = Array.prototype.slice.call(arguments, 0), rv;
I suppose
Code errors
Where is
undefined check
A variable called
Avoid chaining the ternary operator
It's sometimes readable, sometimes not. In your case, it isn't:
Change that to:
Logic-related comments
Always use brackets
Typography
There are only minor typographical issues in your code:
A special case:
BTW, where is
Miscellaneous
I wouldn't use a JavaScript keyword as a function name:
Here comes the rest for the full code.
Bugs
-
The show function resets the display style to
-
Why does
-
Why does
-
-
-
Why do you fallback to
-
"Modern standards"
Note that
The same goes for
For
And finally for
Consistency with jQuery
Your value function fails for textareas, which jQuery also supports via
invoke = superfluous?
The use of
Parameter
(function (exports) {
"use strict";
// ...
return MF;
})(window.MF || (window.MF = {}));- It allows you to assign a different name ("namespace") to your library.
- You are able to use the strict mode in a limited context.
Use [] instead of new Array();
It is shorter and more idiomatic in my opinion:
this.nodes = new Array();
---> this.nodes = [];Don't reuse variable names for different contents
// The "selector" becomes an element
selector = document.getElementById(selector.substring(1));
if(selector){
// Do we really push a selector to a list of nodes?
this.nodes.push(selector);
}The obvious correction is to use a new variable, e.g.
var element.Don't use too long lines
I'm not a fan of the 80 characters per line restriction, but 132 characters on a single line is too much in my eyes:
this.nodes = Array.prototype.slice.call(document.getElementsByClassName(selector.substring(1).replace(/\./g, ' ')), 0);;(BTW, in the line above there is also a superfluous semicolon.)
A possible replacement:
var classNames = selector.substring(1).replace(/\./g, ' ');
this.nodes = Array.prototype.slice.call(document.getElementsByClassName(classNames), 0);Also consider aliasing
Array.prototype.slice.call(..., 0) since its long name distracts too often and too much.Use constants, not their values
} else if (selector.nodeType === 1 /* ... */Consider using a available constant names (or make your owns):
} else if (selector.nodeType === Node.ELEMENT_NODE /* ... */It is now clearer and more readable.
Use meaningful variable names
var args = Array.prototype.slice.call(arguments, 0), rv;
I suppose
rv should stand for result value, but that is definitely not clear enough.Code errors
Where is
MF.foreach declared? Do you mean this.nodes.forEach?undefined check
A variable called
undefined can in fact be declared. I recommend using:if (typeof variable !== 'undefined') { ... }Avoid chaining the ternary operator
It's sometimes readable, sometimes not. In your case, it isn't:
return this.nodes.length ? rv === undefined ? this : rv : undefined;Change that to:
if (this.nodes.length) {
// You can still write an IF statement if you would like to.
return (typeof resultValue === 'undefined') ? this : resutlValue;
}
else {
// This will return an undefined value
// See http://royaltutorials.com/javascript-return-undefined/
return;
}Logic-related comments
// in html helper function
this.innerHTML = '';
MF(this).append(html);
// simplify to
this.innerHTML = html;Always use brackets
if(!selector) return;
--> if (!selector) {
return;
}Typography
There are only minor typographical issues in your code:
default :
default:
if(selector){
if (selector){A special case:
item.invoke(function($this){ $this.appendChild(this); }, this);
// can be shorted to
// See http://css.dzone.com/articles/javascript-fat-city
// But even not all modern browsers support it!
item.invoke($this => $this.appendChild, this);BTW, where is
invoke defined? I suppose it is part of your library, but not shown for brevity.Miscellaneous
I wouldn't use a JavaScript keyword as a function name:
MF.function. For example, it confuses syntax highlighters. I'd recommend MF.fn.Here comes the rest for the full code.
Bugs
-
The show function resets the display style to
block. This is only the default choice for a subset of elements (and their CSS styles).-
Why does
prepend append the item to prepend as a child of the current element?-
Why does
style only allow to set existing values?-
attr fails for falsy values for val! Use typeof val !== 'undefined for the IF condition.-
hasAttr also fails for falsy attribute values. Use Element.hasAttribute.-
Why do you fallback to
offsetWidth in width? When does the fallback happen? I don't know any case where clientWidth yields a falsy value.-
parents can lead to errors (parentNode is eventually null) if no matching parent element can be found."Modern standards"
Note that
ChildNode.remove() is not supported by IE 11.hasClass: you can use element.classList: return this.classList.contains(_class);.The same goes for
addClass: this.classList.add(_class);.For
rmvClass: this.classList.remove(_class);.And finally for
toggleClass as well: this.classList.toggle(_class);.Consistency with jQuery
Your value function fails for textareas, which jQuery also supports via
.val().invoke = superfluous?
The use of
invoke only led to confusions for me. What is its use case? What problem does it try to solve?// in the prepend function
item.invoke(function($this){ $this.insertBefore(this, $this.firstChild); }, this);
// can be rewritten to a much simpler version!
this.insertBefore(item, this.firstChild);Parameter
Code Snippets
(function (exports) {
"use strict";
// ...
return MF;
})(window.MF || (window.MF = {}));this.nodes = new Array();
---> this.nodes = [];// The "selector" becomes an element
selector = document.getElementById(selector.substring(1));
if(selector){
// Do we really push a selector to a list of nodes?
this.nodes.push(selector);
}this.nodes = Array.prototype.slice.call(document.getElementsByClassName(selector.substring(1).replace(/\./g, ' ')), 0);;var classNames = selector.substring(1).replace(/\./g, ' ');
this.nodes = Array.prototype.slice.call(document.getElementsByClassName(classNames), 0);Context
StackExchange Code Review Q#58484, answer score: 4
Revisions (0)
No revisions yet.