4
\$\begingroup\$

I am trying to build a very basic JS framework for my own uses, ultimately collating a lot of pieces of JavaScript I have created over the years. But I would not call myself a JavaScript expert.

What I have written below works well enough but I really would like to know if I am making any fundamental errors in the way this is set up, rather than re-writing a lot of code further down the line.

This basic start point is mirroring the way jQuery can be written to select an element and chain an event to that object, but using all the querySelectorAll instead of the older methods.

If anyone can find basic or advanced flaws in what I've done that would be most useful.

<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<script type="text/javascript">

///DO NOT COPY OR USE FOR COMMERCIAL PURPOSES
function Framework(sel) {

    function myFramework(sel) {

        ///Add selections to array 

        this.sel = sel;

        if (typeof this.sel !== "undefined") {

            if (typeof this.sel !== 'string') {
                this.selArray = new Array(this.sel);
            } else {
                this.what = this.sel.charAt(0);

                if (this.what == ".") {
                    this.selArray = document.querySelectorAll(this.sel);
                } else if (this.what == "[") {
                    this.selArray = document.querySelectorAll(this.sel);
                } else if (this.what == "#") {
                    this.sel = this.sel.substr(1);
                    this.selArray = new Array(document.getElementById(this.sel));
                }
            }
        }

        myFramework.prototype.ready = function (func) {

            document.onreadystatechange = function () {
                var state = document.readyState
                if (state == 'interactive') {
                    //  init();
                } else if (state == 'complete') {
                    func();
                }
            }
        }

        myFramework.prototype.click = function (func) {
            for (i = 0; i < this.selArray.length; i++) {

                if (window.addEventListener) {
                    this.selArray[i].addEventListener('click', func, false);
                } else if (window.attachEvent) {
                    this.selArray[i].attachEvent('onclick', func);
                }
            }

            return this;
        }

        return this;
    }

    return newTask = new myFramework(sel);
}


Framework().ready(function () {
    Framework('.test').click(function () {
        alert("it works!");
    });
});


</script>
</head>

<body>
    <div style="background-color:#FFFF00; height:10px" class="test"></div>
</body>
</html>
\$\endgroup\$
  • 5
    \$\begingroup\$ ///DO NOT COPY OR USE FOR COMMERCIAL PURPOSES You do know that posting a question on a Stack Exchange is under cc by-sa 3.0. I am not a lawyer and don't know anything really about license or anything related to it, but I just wanted to make you aware of that fact. \$\endgroup\$ – Marc-Andre Oct 23 '14 at 17:42
  • \$\begingroup\$ @Marc-Andre was already mentioned on the (now on hold) SO question, you're fighting a lost war... \$\endgroup\$ – Vogel612 Oct 23 '14 at 18:37
  • 1
    \$\begingroup\$ @Marc-Andre What baffles me is why the OP, a self-confessed inexperienced JS programmer, writing a "basic framework," feels the need to try and protect their IP given all of the high quality open source software libraries and frameworks all over the internet. \$\endgroup\$ – Ant P Oct 23 '14 at 18:37
  • 1
    \$\begingroup\$ Maybe just worry about something else, there are bigger issues in the world. \$\endgroup\$ – Guesser Oct 23 '14 at 20:52
3
\$\begingroup\$

What you've got is not a framework. It is a library.

I can see you are basically creating a jQuery-like wrapper for selecting DOM nodes and event handling. Nothing terribly wrong with the main idea.

Why have a function called Framework which has a function inside of it? Every call to Framework(...) causes myFramework to be recreated. Instead, just use a regular prototype-based "class" in JavaScript:

function Framework(sel) {
    if (!(this instanceof Framework)) {
        // return a new Framework object if called without the "new" keyword
        return new Framework(sel);
    }

    this.sel = sel;

    if (this.sel) {
        if (typeof this.sel !== 'string') {
            this.selArray = new Array(this.sel);
        } else {
            this.what = this.sel.charAt(0);

            if (this.what == ".") {
                this.selArray = document.querySelectorAll(this.sel);
            } else if (this.what == "[") {
                this.selArray = document.querySelectorAll(this.sel);
            } else if (this.what == "#") {
                this.sel = this.sel.substr(1);
                this.selArray = new Array(document.getElementById(this.sel));
            }
        }
    }
}

Framework.prototype = {
    constructor: Framework,

    ready: function(func) {
        // ...
    },

    click: function(func) {
        // ...
    }
};

The code that inspects the selector and calls querySelectorAll or getElementById is completely redundant and unneeded. If this.sel is a string, just blindly pass it to document.querySelectorAll. You've gained nothing by inspecting the selector. The speed difference between getElementById("foo") and querySelectorAll("#foo") will be negligible at worst, so why complicate your code?

if (this.sel) {
    this.selArray = typeof this.sel === "string"
        ? document.querySelectorAll(this.sel)
        : [this.sel];
}

Optimizing The Attaching Of Events

In your code, you loop over the array of elements, and on each iteration you test for attachEvent or addEventListener. Browsers won't magically change their native DOM API, so testing for these methods repeatedly when the result will be the same is extra work for the browser. Instead, create a method that does the event binding, which we will call bind.

Instead of testing for the "attachEvent" method on each iteration of the loop in click, test it once:

Framework.prototype = {
    constructor: Framework,

    ready: function(func) {
        // ...
    },

    click: function(func) {
        for (var i = 0; i < this.selArray.length; i++) {
            this.bind(this.selArray[i], "click", func);
        }

        return this;
    },

    bind: !!document.addEventListener
        ? function(element, event, func) {
            element.addEventListener(event, func, false);
            return this;
        }
        : function(element, event, func) {
            element.attachEvent(event, "on" + func);
            return this;
        }
};

The bind method is set on the prototype and makes use of a ternary operator to assign a function to the bind property that supports the current browser's native API instead of checking for support.

       // The Test: Does the browser have a document.addEventListener method?
    bind: !!document.addEventListener

     // Yes: The browser does. Use this function to bind events using standard API
        ? function(element, event, func) {
            element.addEventListener(event, func, false);
            return this;
        }

     // No: The browse does not. Use Internet Explorer's API
        : function(element, event, func) {
            element.attachEvent(event, "on" + func);
            return this;
        }

The decision to use attachEvent or addEventListener is made once at page load. The Framework.prototype.bind method will directly use the supported API for each browser rather than test for it.

More information on ternary operators: Conditional (ternary) Operator

Implicitly Declared Global Variables

Don't forget that variable assignments without var before the variable name create implicitly declared global variables. The for loop in the click method does this with the i variable, and your original Framework method mysteriously declares a global variable newTask.

\$\endgroup\$
  • \$\begingroup\$ That's fantastic Greg, I didn't even know about that constructor syntax. \$\endgroup\$ – Guesser Oct 23 '14 at 21:23
  • \$\begingroup\$ I'm a bit confused about the bind method for event listeners isn't this method being called in the loop anyway or does this have to do with memory management? \$\endgroup\$ – Guesser Oct 23 '14 at 21:24
  • \$\begingroup\$ could you maybe explain this shorthand it's not something I recognise at least not the square brackets and line after the if statement... this.selArray = typeof this.sel === "string" ? document.querySelectorAll(this.sel) : [this.sel]; \$\endgroup\$ – Guesser Oct 23 '14 at 21:39
  • \$\begingroup\$ Sure thing, Guesser. Let me update my answer. \$\endgroup\$ – Greg Burghardt Oct 24 '14 at 12:40
  • \$\begingroup\$ Is calling the method "bind" not a bit confusing with this developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/… \$\endgroup\$ – Guesser Oct 27 '14 at 11:42

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.