4
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ 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... \$\endgroup\$ – kentcdodds Jul 10 '13 at 1:52
  • \$\begingroup\$ A better one would be not to recreate the DOM elements after sorting. Wouldn't it be better to directly sort the list? \$\endgroup\$ – onefourth Jul 10 '13 at 8:33
  • \$\begingroup\$ You're not recreating DOM elements though... Your changing the innerHTML of existing DOM elements... \$\endgroup\$ – kentcdodds Jul 10 '13 at 10:55
1
\$\begingroup\$

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).

\$\endgroup\$
  • \$\begingroup\$ Ten lines reduced to 2 + all the jquery + all the underscore code = god knows how many more \$\endgroup\$ – Jonny Sooter Jul 12 '13 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 Jul 12 '13 at 14:32
0
\$\begingroup\$

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/

\$\endgroup\$
0
\$\begingroup\$

There are a few things you might want to consider.

  1. 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?
  2. 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/

\$\endgroup\$
0
\$\begingroup\$

I would probably do it something like this, by moving the original 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

\$\endgroup\$

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.