4
\$\begingroup\$

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, 0), rv;
            MF.foreach(this.nodes, function(k,v){
                rv = fbody.apply(v, args);
                if(rv !== undefined) return false;
            });

            return this.nodes.length ? rv === undefined ? this : rv : undefined;
       }
   });
};

MF.foreach({
    html: function(html) {
        if(html === undefined){ 
            return this.innerHTML;
        }else{
            this.innerHTML = '';
            MF(this).append(html);
        }
    },
    append: function(item) {
        if(item instanceof MF){
            item.invoke(function($this){ $this.appendChild(this); }, this);
        }else{
            (item instanceof Node) ? this.appendChild(item) : this.insertAdjacentHTML('beforeend',item.toString());
        };
    },
    ...
}, MF.function);

Here is an example use:

HTML

<div id='foo'></div>
<div class='bar'></div>

JavaScript

MF('#foo').html('bar');
MF('.bar').html('foo');

You will find it pretty similar to jQuery. So basically by using MF(selector) I can select an element by either id, class, tag or provide a DOM element or list as a selector. The reason why I chose this over .querySelectorAll() because I believe this to be faster as I remember seeing a benchmark that proved .querySelectorAll() to be pretty slow. There is also the .find() method in the prototype that uses .querySelectorAll() in order for all CSS selectors to be available. The way I extend the prototype is with the method MF.function. As you see a single object may hold a number of nodes, so if a method is called with a syntax like so

MF('div').addCLass('class');

And there are more than one divs, then method addClass needs to be called on each one of them. That's what MF.function takes care of, and also returns the object if the method did not return a value, for chaining to be available.

I'd love to hear some advise on how to improve this little project. Even though it is not finished yet, here is the full code I have at the moment.

I don't include support for those million-years-old browsers. My way of discouraging their use!

\$\endgroup\$
  • 4
    \$\begingroup\$ Since nobody has reviewed this yet, I don't think there would be a problem with updating the question with your new code. No sense having something reviewed that might not exist anymore. \$\endgroup\$ – Dagg Jul 30 '14 at 13:21
  • \$\begingroup\$ @Dagg Seems fair, will update it in a couple of hours because I have to run now. \$\endgroup\$ – php_nub_qq Jul 30 '14 at 13:36
  • \$\begingroup\$ I've revised your post a bit. Please keep in mind that this is not a forum, and updates don't need to be put in different places. Instead, try to fit the update in with everything else, and also refrain from having others skip to the bottom of the post first. Since this question hasn't been answered yet, you're free to completely overwrite your old code if needed. \$\endgroup\$ – Jamal Jul 30 '14 at 16:19
  • 1
    \$\begingroup\$ Wait a little bit, I'm currently writing a review. \$\endgroup\$ – ComFreek Aug 2 '14 at 17:34
5
\$\begingroup\$

Wrap your library code and use strict mode

(function (exports) {
  "use strict";

  // ...
  return MF;
})(window.MF || (window.MF = {}));
  1. It allows you to assign a different name ("namespace") to your library.
  2. 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 naming

attr and val are fine, but x is definitely not! Conerning the functions which return MF instances or raw elements depending on x, it may be better to introduce another function. For instance, I wondered how insertBefore can be recursive. Apropos node doesn't even use its x parameter.

AjaxifyForm

if(form.node().nodeName === 'FORM'){
  // MUCH CONTENT
}else{
  throw 'Type mismatch, only forms should be passed to method [AjaxifyForm]';
}

A better (more readable and maintainable) structure would be:

if(form.node().nodeName !== 'FORM') {
  throw 'Type mismatch...';
}
// MUCH CONTENT

You don't need to roll your own form data collector: A FormData object can be created from a form and can then be passed to XMLHttpRequest.prototype.send().

This function is very unflexible since it relies on hardcoded values in the depot. It also restricts the notification area to a div.statusReporter.

Don't use formData[formData.length] = .... Use formData.push(...)!

The use of onsubmit instead of on('submit', ...) prevents other handlers to listen for the submit event.

As far as I understand your code, uploadFile only uploads a single file!


The examination of the full code is by far not complete, but the most important points should be mentioned if I'm not mistaken.


Conclusion

Writing a DOM library is definitely a good exercise, but you happen to use a lot of functions jQuery and similar libraries provide. In this case, including a library would be a welcome trade-off for me.

Have a look at Zepto.js for a small version of jQuery.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your time. A lot of points taken! I have provided a link to the full code, which you may have missed. I thought adding a bunch of functions here would be too much so I included only the core functions in the question. However I have a question about point 3. Why shouldn't I reuse the variable, I thought this was actually the smarter way, because this.nodes[0] needs to become the node, but getElementsById returns null if element is not found, and I don't want to have null in this.nodes. I don't understand why the way I've written it is not good? \$\endgroup\$ – php_nub_qq Aug 2 '14 at 18:50
  • 1
    \$\begingroup\$ @php_nub_qq the issue there is what you're calling things, not what you're doing. You're storing an element in a variable called selector; it's confusing. \$\endgroup\$ – Dagg Aug 2 '14 at 19:05
  • \$\begingroup\$ @Dagg Oh, now I see! \$\endgroup\$ – php_nub_qq Aug 2 '14 at 19:07
  • \$\begingroup\$ @php_nub_qq See the update for a partial analysis of the full code. \$\endgroup\$ – ComFreek Aug 3 '14 at 11:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.