My first javascript sorting

Question

I started learning JavaScript a week ago, and I made a sorting function on my own. It does work, but I need reviews and how to write a better one.

<body>
<ul id="list">
    <li>Art</li>
    <li>Mobile</li>
    <li>Education</li>
    <li>Games</li>
    <li>Magazines</li>
    <li>Sports</li>
</ul>

var list = document.getElementById("list");
var myList = list.getElementsByTagName("li");
var a = [];
for (var i = 0; i < myList.length; i++) {
    a[i] = myList[i].innerHTML;
}
a.sort();
for (var i = 0; i < myList.length; i++) {
    myList[i].innerHTML = a[i];
}

Output:

Art

Education

Games

Magazines

Mobile

Sports

What is your purpose for this? What would constitute a "better" sorting function? As I see it, it seems to accomplish the purpose fairly nicely... — kentcdodds, Commented Jul 10, 2013 at 1:52
A better one would be not to recreate the DOM elements after sorting. Wouldn't it be better to directly sort the list? — onefourth, Commented Jul 10, 2013 at 8:33
You're not recreating DOM elements though... Your changing the innerHTML of existing DOM elements... — kentcdodds, Commented Jul 10, 2013 at 10:55

tokland · Accepted Answer · 2013-07-10 14:54:59Z

1

Some notes:

var myList: Don't use generic names like myList. My list of what? Variable names are very important, they should inform about the nature of their content, not their type (although it's indeed important to use singular variable names for single elements and plurals for collections).
var myList = list.getElementsByTagName("li");. What if list is null? no checks?
myList[i].innerHTML = a[i];. You are overwriting HTML contents, it would probably be better to move DOM elements instead.

I'll leave a pure Javascript re-write to others, if you are interested in solutions that use external libraries, using jQuery and underscore I'd write:

var sorted_lis = _.sortBy($("#list li"), function(li) { return li.innerText; });
$("#list").empty().append(sorted_lis);

The original ten lines reduced to two (one actually, but it's good to break things a bit and give names to intermediate values), thanks to functional style (underscore) and easy DOM manipulation (jQuery).

edited Jul 10, 2013 at 14:54

answered Jul 10, 2013 at 12:19

tokland

11.1k1 gold badge21 silver badges26 bronze badges

\$\begingroup\$ Ten lines reduced to 2 + all the jquery + all the underscore code = god knows how many more \$\endgroup\$
– Jonny Sooter
Commented Jul 12, 2013 at 14:26
\$\begingroup\$ @JonnySooter: Well, we agree that using underscore/jQuery or any other external library if all we want to do is sort some tags makes no sense. But it's usually the case that you're doing some other things and including them pays off on the long run. Also, you can count the whole sum of lines to see, I don't know, the load size of the page, but not when considering code complexity (the one, most important factor in programming). We don't care if those libraries require 100 or 100K lines to work, we are worried about the complexity of the code we have to maintain. Here, that's 2 lines :-) \$\endgroup\$
– tokland
Commented Jul 12, 2013 at 14:32

Add a comment |

Stuart · Accepted Answer · 2013-07-10 19:48:41Z

Unfortunately a list of html nodes (NodeList) in Javascript does not have a sort method, otherwise you could apply that directly. However, if you transform it into an array, you can sort it easily by content and then append it back to the list. On balance I'm not sure this is really better than your original solution though!

var myListParent = document.getElementById('list');
    myListChildren = myListParent.children,
    myListArray = [];
for (var i = 0; i < myListChildren.length; i++) {
    myListArray.push(myListChildren[i]);
}
function sortByInnerHTML(a, b) {
    var h1 = a.innerHTML,
        h2 = b.innerHTML;
    if (h1 == h2) return 0;
    else if (h1 < h2) return -1;
    else return 1;
}

myListArray.sort(sortByInnerHTML);

for (var i = 0; i < myListArray.length; i++) {
    myListParent.appendChild(myListArray[i]);
}

(jsfiddle)

However, to make your code neater and more reusable you could create some functions here.

function sortHTML(parent) {
    var a = [], children = parent.children;
    for (var i = 0; i < children.length; i++) {
        a.push(children[i]);
    }
    a.sort(sortBy('innerHTML'));
    for (var i = 0; i < a.length; i++) {
        parent.appendChild(a[i]);
    }
}

function sortBy(p) {
    return function(a, b) {
        return a[p] < b[p] ? -1 : a[p] > b[p];
    }
}

sortHTML(document.getElementById('list'));

http://jsfiddle.net/DL7Xa/2/

JayC · Accepted Answer · 2013-07-12 14:31:11Z

There are a few things you might want to consider.

The way you are sorting, the nodes stay in the same place, but the content changes. However, what if each node had DOM events attached to them?
Any time you change content, you may cause page reflow events. This can be avoided, well, not entirely, but it can be done all at once using a Document Fragment: https://developer.mozilla.org/en-US/docs/Web/API/document.createDocumentFragment

Combining the two, here's an idea for a sorter: http://jsfiddle.net/jfcox/647YA/

If you are concerned about IE8 support, you apparently cannot use children, you'd have to use childNodes and only add the elements (skipping text nodes, etc.).

Edit: It turns out that in sorting, because the elements to be sorted are already in the DOM tree, reflow still occurs (as the elements are removed and placed into the documentFragment). To remedy, I've modified it to move the parentElement to the documentFragment, move the elements within the fragment, and then move the parent back to the DOM. It's quite a bit faster: http://jsfiddle.net/jfcox/HPxZz/

Xotic750 · Accepted Answer · 2013-07-14 23:56:01Z

I would probably do it something like this, by moving the original dom elements, using some generic functions that can be reused and that are cross-browser (should work IE5+).

Javascript

function getText(node, text) {
    if (typeof text !== "string") {
        text = "";
    }

    if (node.nodeType === 3) {
        text += node.nodeValue;
    }

    node = node.firstChild;
    while (node) {
        text += getText(node, text);
        node = node.nextSibling;
    }

    return text;
}

function emptyNode(node) {
    while (node.firstChild) {
        node.removeChild(node.firstChild);
    }
}

function sortByTextFn(a, b) {
    var textA = getText(a),
        textB = getText(b);

    if (textA === textB) {
        return 0;
    }

    if (textA < textB) {
        return -1;
    }

    return 1;
}

function sortHTMLList(element) {
    if (typeof element === "string") {
        element = document.getElementById(element);
    }

    var lis = element.getElementsByTagName("li"),
        length = lis.length,
        array = [];
        i = 0;

    while (i < length) {
        array.push(lis[i]);
        i += 1;
    }

    array.sort(sortByTextFn);
    emptyNode(element);
    while (array.length) {
        element.appendChild(array.shift());
    }
}

sortHTMLList("list");

On jsfiddle

Stack Exchange Network

My first javascript sorting

4 Answers 4

Hot Network Questions

My first javascript sorting

4 Answers 4

Related

Hot Network Questions