HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Yet another JavaScript library

Submitted by: @import:stackexchange-codereview··
0
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,

Solution

Wrap your library code and use strict mode

(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.