Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

My own CMS is currently using jQuery, but as one of the goals is to have the whole project to be very small, I've decided to write my own basic library. I only really need to select elements and modify them using results from my server (via Ajax).

The JavaScript-library v0.01: (Attempt 1)

(function(){
    a=this.Function;
    a.prototype.extend=(function(a,b){this[a]=b;return this});
    a.prototype.implement=(function(a,b){this.prototype[a]=b;return this});
    $=(function(a,b,c){return (b?$(b)[c?c:0]:document).querySelectorAll(a)})
    .extend("post",(function(a,b){
        c=[];
        for(x in a)
            c[c.length]=[x,a[x]].join("=");
        d=XMLHttpRequest?new XMLHttpRequest():new ActiveXObject("Microsoft.XMLHTTP");
        d.open("POST","./",true);
        d.setRequestHeader("Content-type","application/x-www-form-urlencoded");
        d.onreadystatechange=b;
        d.send(c.join("&"));
        return this;
    }))
    .extend("each",(function(a,b,c){
        for(x in a)
            if(a.hasOwnProperty(x))
                b.call(c, a[x], x, a);
        return this;
    }));
})();

I know that querySelectorAll can't be relied on, but it is just good for the start of this.

The JavaScript-library v0.02: (Attempt 2)

I believe this is much improved. It has reasonable variable names, works better, and most of all is quite buggy.

var _    = new function Sample(){};  ////////////////////////////////////////
    _.temp  = {};                    // This Object is intended just for
    _.pages = {};                    // testing, I do the rest in the console
    _.pid   = {};                    // <- Stores the current pageID
    _.el    = {};                    // /PAGE-ID/PAGE-TITLE or #!page=PAGE-ID
    _.fn    = {};                    ////////////////////////////////////////
    _.$    = {};                     // <- Lets leave JQuery alown. :)
(function(){
    this.$ = function(a,b,c){
        if(b&&b.isType("string"))   b = $(b);
        d=b?b[c|0]:document;
        return document.getElementsByClassName.call(d,a);
    };
    var $FP = Function.prototype, $OP = Object.prototype;
    $OP.isType = function (type){
        return typeof this === type;
    }
    $OP.each   = function(fun){ // `fun()` runs 3 times HOW??
        if(this.isType("array")&&this.forEach) return this.forEach(a); // if this is an array and we have a browser with Array().forEach then lets use native code instead.
        if(this.isType("object")||this.isType("array"))
            for(var x in this)
                if(this.hasOwnProperty(x))
                    fun.call(this[x],x,this);
        return this;
    };
    $OP.toString    = function(){
        if(!this.isType("object")&&!this.isType("array")) return this;
        var arr=[];
        this.each(function(a){
            arr[arr.length] = a+"="+this;
        });
        return arr.join("&");
    }
    $FP.multiInput = $FP.MI   = function(){
        var self = this;
        return function(obj){
            if(obj.isType("object")||obj.isType("array"))
                obj.each(function(a,b){
                    self.call(b,a,this);
                });
            else
                self.apply(this,arguments);
            return this;
        };
    };
    $OP.extend = function(key,val){ // I want to add all my functions dynamicly and in bulk.
        (this.prototype||this)[key]=val; // Either myObject.extend("key","val") or my Object.extend({"key":"val"}) work.
        return this;
    }.MI(); // MI/multiInput simply allows objects to be passed instead of making it more sence to use obj.prototype.test=myvalue;
    this.$.extend({
        "post": function(a,b,c,d){
            var e=XMLHttpRequest ? new XMLHttpRequest() : new ActiveXObject("Microsoft.XMLHTTP");
        e.open("post", a, true);
            e.setRequestHeader('Content-Type', 'application/x-www-form-urlencoded');
            e.onreadstatechange = function(){
                if(xmlhttp.readyState==4) // Page has fully loaded, now did we get a status code of "200 ok" or an error? Please tell me if "304 Not modified" should be added as run `c` and not `d`
                    (xmlhttp.status==200?c:d).call(xmlhttp.responseText,xmlhttp);
            };
            e.send(b.toString());
            return this;
        },
        "post_json": function(a,b,c){
            return this.post(a,b,function(){
                c.call("return "+$.Function(this)()); // This should run c with this being the object. What works better `()` or `.call()` ?? 
            });
        }
    });
}).call(_); // Lets Leave JQuery alown.

_.$.prototype.each(function(){console.log(this)});
share|improve this question
add comment

3 Answers

Rewrite it from scratch, if I were to encounter such code in any commit, I would immediately remove that code and talk with the one who committed it about JS in general...

// anonymous wrapper if fine...
(function(){

    // but not using var still creates global variables
    a=this.Function; // why are you extending the builtin function constructor?

                           // no need for extra parenthesis here
                           // also, this is hardly needed.. for the code below
    a.prototype.extend=    (function(a,b){this[a]=b;return this});


    // what is this being used for?
    a.prototype.implement=(function(a,b){this.prototype[a]=b;return this});


    // overrides an existing mapping of jQuery
    $=(function(a,b,c){return (b?$(b)[c?c:0]:document).querySelectorAll(a)})

    // hard to read
    .extend("post",(function(a,b){

        // more leakage
        c=[];

        // if you don't use {} you might as well put it on the same line
        // Oh and I shouldn't forget my usual rant about the missing
        // hasOwnProperty call
        for(x in a) // leakage of x
            c[c.length]=[x,a[x]].join("="); // what about c.push() ?

        // leakage leakage leakage
        d=XMLHttpRequest?new XMLHttpRequest():new ActiveXObject("Microsoft.XMLHTTP");

        d.open("POST","./",true);
        d.setRequestHeader("Content-type","application/x-www-form-urlencoded");
        d.onreadystatechange=b;
        d.send(c.join("&"));
        return this;
    }))

    .extend("each",(function(a,b,c){
        for(x in a) // leakage of x
            if(a.hasOwnProperty(x)) // why all of a sudden hasOwnProperty here?
                b.call(c, a[x], x, a);
        return this;
    }));
})();

EDIT

An untested cleaned up version from me.

(function() {
    function $(selector, parentSelector, index) {
        var element = parentSelector ? $(parentSelector)[index || 0] : document;
        return element.querySelectorAll(a);
    };

    $.extend = function(obj, props) {
        for(var i in props) {
            if (props.hasOwnProperty(i)) {
                obj[i] = props[i];
            }
        }
        return this;
    };

    $.extend($, {
        post: function(data, callback) {
            var params = [];

            for(var i in data) {
                if (data.hasOwnProperty(i)) {

                    // might need url encoding...
                    params.push([i, data[i]].join('='));
                }
            }

            // this might need for fallbacks, check the jQuery source for that
            var req;
            if (XMLHttpRequest) {
                req = new XMLHttpRequest();

            } else {
                req = new ActiveXObject("Microsoft.XMLHTTP");
            }

            req.open('post', '/', true);

            // is that really cross browser these days?
            req.setRequestHeader('Content-Type', 'application/x-www-form-urlencoded');
            req.onreadstatechange = callback;
            req.send(params.join('&'));
            return this;
        },

        each: function(obj, func, that) {
            for(var i in obj) {
                if (obj.hasOwnProperty(i)) {
                    func.call(that, obj[i], obj, i);
                }
            }
            return this;
        }
    });
    window.$ = $;
})();
share|improve this answer
    
What do you think about my new edits? –  JamesM-SiteGen Feb 15 '11 at 8:02
add comment

This code uses some poor mistakes and has a few mistakes in it.

See annotated code.

Recommended reading : Let's make a framework , learn from the jQuery source 1 and 2 and the jquery source (warning. Daunting!)

(function () {
    // Ok so were going to extend the global Function object?
    // Don't extend native objects it really messes with other people's code
    // If you must you can either clone them or wrap around them

    // Let's not forget that a is implecitly global. We do not do this.
    // If we want a global we MAKE it global. so declare it with 
    // var a = ...
    // and use global.a or window.a = ... to set things in global scope
    // a doesn't even need to be global and if it's does its a horrible name.
    a = this.Function;
    // Were calling extend twice. Why don't we define $.post and $.each
    // seperately instead. This is a classic case of hardcore over-engineering

    // We do not extend native prototypes. This is bad, other people make 
    // assumptions of what the native prototypes are like

    // No need to wrap functions in ( ). This is only neccesary for when you
    // want to do 
    // (function() { }()) and  there are no other "symbols" on the line.
    // We only use ( ) because function () { }() is an invalid expression 
    // and throws an error without ( )
    a.prototype.extend = (function (a, b) {
        // why do we need a f.extend("foo", o) 
        // It's neater to just call f.foo = o
        // This function is redundant. 
        this[a] = b;
        // Ok it implements chaining. It's really not worth it for chaining
        // all you can chain is f.extend. 
        return this
    });
    // Dead code
    a.prototype.implement = (function (a, b) {
        // Even if its not dead again your just calling f.implement("foo", o)
        // instead of f.prototype.foo = o
        // this really hurts readability and feels unneccesary.
        this.prototype[a] = b;
        return this
    });
    // Woh another implecit global this is bad. Oh and let's overwrite anyone
    // who defines $ shall we. Bye jQuery, bye prototype, bye mootools. 
    // if your writing a framework then don't overwrite common names like $.
    // Dont use excuses like I will only use it. If your going to do it, 
    // then do it properly.
    $ = (function (a, b, c) {
        // I might get round to "understanding" this block. It's messy though
        return (b ? $(b)[c ? c : 0] : document).querySelectorAll(a)
    }).extend("post", (function (a, b) {
        c = [];
        for (x in a)
        c[c.length] = [x, a[x]].join("=");
        d = XMLHttpRequest ? new XMLHttpRequest() : new ActiveXObject("Microsoft.XMLHTTP");
        d.open("POST", "./", true);
        d.setRequestHeader("Content-type", "application/x-www-form-urlencoded");
        d.onreadystatechange = b;
        d.send(c.join("&"));
        return this;
    })).extend("each", (function (a, b, c) {
        for (x in a)
        if (a.hasOwnProperty(x)) b.call(c, a[x], x, a);
        return this;
    }));
})();
share|improve this answer
    
I removed extend and made implement extend, I thought I updated it here. –  JamesM-SiteGen Feb 6 '11 at 5:21
    
Humm, does $.extend extend $ or Function? –  JamesM-SiteGen Feb 6 '11 at 5:25
    
@JamesM-SiteGen $.extend(objectToExtend, ObjectToExtendWith) –  Raynos Feb 6 '11 at 14:18
    
What do you think about my new edits? –  JamesM-SiteGen Feb 15 '11 at 8:03
add comment

I would suggest writing your code with full variable names and generating your production version with a minimizer to improve readability. As it stands this is reviewable because it's small, but I have no desire to be thorough because your variables are annoying to trace. This will also make maintenance less of a hassle.

Overall it looks to accomplish the objective. Each ajax request will require manual error checking if you plan on having any fault-tolerance. If this will be used several times you may want to refactor the error checking into the base to save overall code length later.

share|improve this answer
    
Why the hell has this answer +4? –  Ivo Wetzel Feb 3 '11 at 20:30
1  
Code is full of WTF (no offensive to coder. @IvoWetzel points out some basic mistakes) –  Raynos Feb 3 '11 at 20:38
    
+1 for making the code readable and optimizing at a later step if needed using tools. –  Eric Bréchemier Feb 4 '11 at 13:36
    
@Ivo: lol, I guess it doesn't have a +4 anymore, but I imagine it did because people agreed with my comment? Just a thought. They are valid review pointers, even if yours are more thorough... –  TheXenocide Feb 8 '11 at 14:28
    
@TheXenocide The lack of var causing variables to be leaked into the global space (and thus defeating the use of the (function{})() wrapper) is a serious WTF - I would say that the reason why it was not immediately obvious why the code is bad is because the author minified it, making it really hard to read –  Yi Jiang Feb 11 '11 at 9:48
show 3 more comments

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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