Page 7 of 8 FirstFirst ... 5678 LastLast
Results 61 to 70 of 75

Thread: Ouch - Scathing Ext/Sencha code review!

  1. #61
    Sencha User
    Join Date
    Jan 2009
    Location
    Palo Alto, California
    Posts
    1,939

    Default

    These things are almost always dwarfed by optimizations in the DOM level, which is where we spend most of our time on performance optimization. One bad CSS class can do 100x more damage than functions like this (not in every case, but most of the time). Once we are satisfied with our DOM-level performance across the various platforms we'll be happy to come in an optimize functions like these.

  2. #62
    Sencha User
    Join Date
    Feb 2012
    Posts
    9

    Default

    Alright . a dom function it is then.

    Code:
    getDom : function(el, strict) {
                if (!el || !document) {
                    return null;
                }
                if (el.dom) {
                    return el.dom;
                } else {
                    if (typeof el == 'string') {
                        var e = document.getElementById(el);
                        // IE returns elements with the 'name' and 'id' attribute.
                        // we do a strict check to return the element with only the id attribute
                        if (e && isIE && strict) {
                            if (el == e.getAttribute('id')) {
                                return e;
                            } else {
                                return null;
                            }
                        }
                        return e;
                    } else {
                        return el;
                    }
                }
            },
    Usually the idea is to exit as early as possible to avoid unneeded checks and other processing.

    I have no clue at all why someone would want to get a dom node when they already have it but I'll leave that intact since ... i have no clue.

    Also the current method is unsafe due to the return el when it could be anything (and you just asked for a Dom element ... so you're going to do Dom processing ... which is actually done without checking anything in your own library so wtf ?).

    Code:
    getDom : function(el, strict) {
                if (!el || !document) {
                    return null;
                }
                if (el.dom) {//questionable use at best ??
                    return el.dom;
                } else if (typeof el == 'string') {
                    var e = document.getElementById(el);
                    // IE until 8 standards mode returns elements with the 'name' and 'id' attribute.
                    // we do a strict check to return the element with only the id attribute
                    if(!isIE){
                        return e;
                    }else{
                        if (e && strict) { //add the relevant checks for "IE8 standards"+ if that's faster than getAttr+cmp - doubtful though
                            if (el == e.getAttribute('id')) {
                                return e;
                            }
                        }
                    }
                }
                return null;
            },
    Would this indeed work out much better or am I missing something again ?

  3. #63
    Sencha User
    Join Date
    Nov 2008
    Location
    San Diego, Peoples' Republic of California
    Posts
    2,061

    Default

    I've been writing for ExtJS since the very early days (version 1 or earlier). I don't think I've ever called the getDom() function...

    That said, your function does not do what the ExtJS one does. The getDom() function may be called with a DOM element via some twisty path of logic flow through the application then framework. That twisty path may make sense in the context of things.

    As for my own coding with ExtJS, if I am dealing with the DOM directly, as when making custom form fields...

    Code:
    this.el && this.el.dom && doSomethingWith(this.el.dom);
    My code above has specific knowledge about the data structures and member types. The function deep in the bowels of the framework is far more flexible.

  4. #64
    Sencha User
    Join Date
    Feb 2012
    Posts
    9

    Default

    Quote Originally Posted by mschwartz View Post
    That said, your function does not do what the ExtJS one does. The getDom() function may be called with a DOM element via some twisty path of logic flow through the application then framework. That twisty path may make sense in the context of things.
    Why not. it also returns an array if you pass it an array because there is no type check on the el param.

    However, this makes absolutely no difference to the fact that checking for IE + strict + e before doing anything else is just inefficient.

    Code:
    getDom : function(el, strict) {
                if (!el || !document) {
                    return null;
                }
                if (el.dom) {
                    return el.dom;
                } else if (typeof el == 'string') {
                    var e = document.getElementById(el);
                    // IE until 8 standards mode returns elements with the 'name' and 'id' attribute.
                    // we do a strict check to return the element with only the id attribute
                    if(!isIE){
                        return e;
                    }else{
                        if (e && strict && el == e.getAttribute('id')) {
                            return e;
                        } else {
                            return null; //should we not try to find the element instead ?
                        }
                    }
                }else{
                    return el;
                }
            },
    I know this may look like details to you, but stacking inefficiencies like that on top of each other is what makes most GUI libs slow, and when it takes just a few minutes to find one and the evidence of more, it becomes clear that most of the lib's speed issues can be traced down to bad quality code rather than design choices.

    Want some more obvious ?

    Code:
     hasCls : Ext.supports.ClassList ?
                function(className) {
                    if (!className) {
                        return false;
                    }
                    className = className.split(spacesRe);
                    var ln = className.length,
                        i = 0;
                    for (; i < ln; i++) {
                        if (className[i] && this.dom.classList.contains(className[i])) {
                            return true;
                        }
                    }
                    return false;
                } :
                function(className){
                    return className && (' ' + this.dom.className + ' ').indexOf(' ' + className + ' ') != -1;
                },
    So this is a BASE function by any standard, quite majorly important (just a bit less than those around it though).

    So let's consider the following div:
    HTML Code:
    <div class='open shiny'></div>
    If you ask that function wether that div supports the class 'open dull' it will answer yes because it matched the first class. (It would also match 'closed shiny' by the way ...)

    So if you're going to split and then validate on the first success .. WTF

    On the other hand the default (non html5 Classlist) does what it should do . albeit it cannot match 'open shiny' with 'shiny open' or 'open bright shiny'.

    Code:
     hasCls : Ext.supports.ClassList ?
                //not optimized
                function(className) {
                    if (!className) {
                        return false;
                    }
                    className = className.split(spacesRe);
                    var ln = className.length,
                        i = 0;
                    for (; i < ln; i++) {
                        if (!(className[i] && this.dom.classList.contains(className[i]))) {
                            return false;
                        }
                    }
                    return true;
                } :
                function(className){
                    if (!className) {
                        return false;
                    }
                    className = className.split(spacesRe);
                    var ln = className.length,
                        i = 0;
                    for (; i < ln; i++) {
                        if ((' ' + this.dom.className + ' ').indexOf(' ' + className + ' ') == -1){
                            return false;
                        }
                    }
                },
    Anyway, the main reason I'm spending a bit of time on this is I need to find a *decent* solution for webapp GUI's, I'm more or less fluent in Flex which has far more features but is doomed due to current politics, I do pretty nice stuff with jQuery too but then I also know it's far from being the ultimate tool ...

    But I do believe Sencha is going the right way (i.e. trying to bring a flex-like solution to html5+JS world) and I would definitely like to be able to choose that option since Sencha looks like the most advanced one at the moment.

    So if you guys are just ready to acknowledge I can help you improve that thing big time, I'd be honored to participate in this project and help make it a lib even I would praise --

    My belief is that your project is a great one, your devs are prolly not all that bad, but in the end you have a mountain of crap code (the last example is way beyond the 'crap' level though) that needs to be rewritten in order to leap ahead of the competition in terms of speed AND reliability - I'm willing to work on that.

  5. #65
    Sencha Premium User
    Join Date
    Apr 2008
    Posts
    504

    Default

    I'll donate $20 to his salary.

  6. #66
    Sencha User
    Join Date
    Nov 2008
    Location
    San Diego, Peoples' Republic of California
    Posts
    2,061

    Default

    Quote Originally Posted by Morg. View Post
    Why not. it also returns an array if you pass it an array because there is no type check on the el param.
    Because your code doesn't return a DOM node if a DOM node is passed in. Your function returns NULL in that case.

  7. #67
    Sencha User
    Join Date
    Feb 2012
    Posts
    9

    Default

    Quote Originally Posted by mschwartz View Post
    Because your code doesn't return a DOM node if a DOM node is passed in. Your function returns NULL in that case.
    You might want to reread the post you quoted, it does include a version that is both much more efficient and takes that case into account.

  8. #68
    Sencha User
    Join Date
    Nov 2008
    Location
    San Diego, Peoples' Republic of California
    Posts
    2,061

    Default

    Quote Originally Posted by Morg. View Post
    You might want to reread the post you quoted, it does include a version that is both much more efficient and takes that case into account.
    Your code:

    Code:
    getDom : function(el, strict) {
                if (!el || !document) {
                    return null;
                }
                if (el.dom) {//questionable use at best ??
                    return el.dom;
                } else if (typeof el == 'string') {
                    var e = document.getElementById(el);
                    // IE until 8 standards mode returns elements with the 'name' and 'id' attribute.
                    // we do a strict check to return the element with only the id attribute
                    if(!isIE){
                        return e;
                    }else{
                        if (e && strict) { //add the relevant checks for "IE8 standards"+ if that's faster than getAttr+cmp - doubtful though
                            if (el == e.getAttribute('id')) {
                                return e;
                            }
                        }
                    }
                }
                return null;
            },
    It returns null if el passed in is not a string, period.

  9. #69
    Sencha User
    Join Date
    Feb 2012
    Posts
    9

    Default

    Quote Originally Posted by Morg. View Post
    Why not. it also returns an array if you pass it an array because there is no type check on the el param.

    However, this makes absolutely no difference to the fact that checking for IE + strict + e before doing anything else is just inefficient.

    Code:
    getDom : function(el, strict) {
                if (!el || !document) {
                    return null;
                }
                if (el.dom) {
                    return el.dom;
                } else if (typeof el == 'string') {
                    var e = document.getElementById(el);
                    // IE until 8 standards mode returns elements with the 'name' and 'id' attribute.
                    // we do a strict check to return the element with only the id attribute
                    if(!isIE){
                        return e;
                    }else{
                        if (e && strict && el == e.getAttribute('id')) {
                            return e;
                        } else {
                            return null; //should we not try to find the element instead ?
                        }
                    }
                }else{
                    return el;
                }
            },
    There is a distinct probability that it isn't the case in all the multiverses but i'm pretty sure that in this universe, the post containing the text you quoted (although the text you quoted is somewhat misleading, my bad) includes a corrected version that is also an improvement over the original.

  10. #70
    Sencha User
    Join Date
    Jan 2009
    Location
    Palo Alto, California
    Posts
    1,939

    Default

    Ok what I meant by DOM manipulation is the large-scale DOM interaction that the framework has to perform when creating and updating large amounts of UI. I'm talking about the order of operations leading to reflow/repaint more than individual DOM operations.

    There's a whole different set of problems to overcome when operating at the application-wide level, and they tend to have enormously more impact than optimizing specific functions.

Page 7 of 8 FirstFirst ... 5678 LastLast

Similar Threads

  1. How to integrate existing code with Sencha
    By ssdesign in forum Sencha Touch 1.x: Discussion
    Replies: 2
    Last Post: 28 Jun 2010, 5:58 PM
  2. isCellEditable and MultiRowUpdate combined - code review appreciated
    By HerrB in forum Ext 2.x: Help & Discussion
    Replies: 0
    Last Post: 24 Mar 2009, 10:24 AM
  3. Code Review - Loading Form and Grid simultaneously
    By elopez2022 in forum Ext 2.x: Help & Discussion
    Replies: 1
    Last Post: 8 May 2008, 1:17 PM
  4. Tutorial JS code review
    By ffzhuang in forum Community Discussion
    Replies: 9
    Last Post: 9 Oct 2007, 9:48 PM

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •