Render elements from an array of objects

Question

The goal is to list out some data, but the methods vary on options, and whether the user is on a touch device.

This is my current function declaration. Very non-DRY. Values of isTouch and headlinesOnly don't change during loop, so I don't want to make if tests inside the loop.

Edit: This is primarily meant for a memory-sensitive environment which, when not handled properly, reboots to safe mode, not a regular browser. (But I'm making it for both.)

var makeElement;

if (isTouch) {
    if (headlinesOnly) {
        makeElement = function(item, top, left) {
            var a = document.createElement("a");
            a.setAttribute('ontouchstart', 'itemTouchStart()');
            a.setAttribute('ontouchmove', 'itemTouchMove()');
            a.setAttribute('ontouchend', 'itemTouchEnd(\'' + item.link + '\')');
            a.style.top = top + 'px';
            a.style.left = left + 'px';
            a.innerHTML = '<h1>' + item.title + '</h1>';
            return a;
        };
    }
    else {
        makeElement = function(item, top, left) {
            var a = document.createElement("a");
            a.setAttribute('ontouchstart', 'itemTouchStart()');
            a.setAttribute('ontouchmove', 'itemTouchMove()');
            a.setAttribute('ontouchend', 'itemTouchEnd(\'' + item.link + '\')');
            a.style.top = top + 'px';
            a.style.left = left + 'px';
            a.innerHTML = '<h1>' + item.title + '</h1>'
                        + '<div>' + item.content + '</h1>';
            return a;
        };
    }

    var dragging;

    window['itemTouchStart'] = function() {
        dragging = false;
    };

    window['itemTouchMove'] = function() {
        dragging = true;
    };

    window['itemTouchEnd'] = function(url) {
        if (dragging) return;

        if (cycript) {
            tryOpenUrl(url);
        }
        else {
            window.open(url, "_blank");
        }
    };
}
else {
    if (headlinesOnly) {
        makeElement = function(item, top, left) {
            var a = document.createElement("a");
            a.setAttribute('onmouseup', 'itemMouseUp(\'' + item.link + '\')');
            a.style.top = top + 'px';
            a.style.left = left + 'px';
            a.innerHTML = '<h1>' + item.title + '</h1>';
            return a;
        };
    }
    else {
        makeElement = function(item, top, left) {
            var a = document.createElement("a");
            a.setAttribute('onmouseup', 'itemMouseUp(\'' + item.link + '\')');
            a.style.top = top + 'px';
            a.style.left = left + 'px';
            a.innerHTML = '<h1>' + item.title + '</h1>'
                        + '<div>' + item.content + '</h1>';
            return a;
        };
    }

    window['itemMouseUp'] = function(url) {
        window.open(url, "_blank");
    };
}


var item = myCollection.shift();

container.appendChild(makeElement(item));

var columnWidth = container.childNodes[0].offsetWidth;
var containerWidth = container.offsetWidth;
var numberOfColumns = containerWidth / columnWidth;
var columns = [];

for (var i = 0; i < numberOfColumns; i++) {
    this.columns.push(0);
}

while (item = myCollection.shift()) {
    var sh = getShortestColumn();
    var itemElement = makeElement(item, sh.height, sh.index * columnWidth);
    container.appendChild(itemElement);
    columns[sh.index] += itemElement.offsetHeight;
}

Answer 1

The two makeElement functions are the same, except for the innerHTML. You could move the common part to its own function, and make it take innerHTML as parameter:

function createAnchor(item, top, left, innerHTML) {
    var a = document.createElement("a");
    a.setAttribute('ontouchstart', 'itemTouchStart()');
    a.setAttribute('ontouchmove', 'itemTouchMove()');
    a.setAttribute('ontouchend', 'itemTouchEnd(\'' + item.link + '\')');
    a.style.top = top + 'px';
    a.style.left = left + 'px';
    a.innerHTML = innerHTML;
    return a;
}

And then the code can be a bit simpler:

if (headlinesOnly) {
    makeElement = function(item, top, left) {
        var innerHTML = '<h1>' + item.title + '</h1>';
        return createAnchor(item, top, left, innerHTML);
    };
} else {
    makeElement = function(item, top, left) {
        var innerHTML = '<h1>' + item.title + '</h1>'
                    + '<div>' + item.content + '</h1>';
        return createAnchor(item, top, left, innerHTML);
    };
}

You can do likewise for the other functions too. You can have a chain of these helper functions: one function that creates a very primitive anchor, setting only top and left, and other helpers can build on top of it to set the event handlers and inner html that they need.

Answer 2

Two things right off the bat:

• You're assuming that item.title and item.content are "safe" to insert. I.e. that they don't contain HTML, something that could be mistaken for HTML, or that if they contain HTML, it's meant to be there.

  For instance, if title is "How to use the <a> tag", then the "<a>" will be interpreted as HTML by the browser, and you won't get the right result.

• Putting block elements (h1 and div) inside an inline element (a) is invalid HTML. (Edit: I was wrong - it's allowed, if you're using HTML5. Still invalid in earlier version, though. Tip 'o the hat to @dev-null for the correction). Anyway, your a elements hardly need to be a elements; you're handling all the interaction in JavaScript. They could just as well be div elements, or article elements or something else entirely.

Moving on. Janos already touched on how to improve makeElement, but I'd take it a step further.

I don't like the idea of defining a global function in an if/else to begin with, when said if/else could just as well be inside the function.

Using Janos' extracted createAnchor, you could simply define makeElement as:

function makeElement(item, top, left, includeContent) {
    var innerHTML = '<h1>' + item.title + '</h1>';
    if(includeContent) {
        innerHTML += '<div>' + item.content + '</h1>';
    }
    return createAnchor(item, top, left, innerHTML);
}

The includeContent argument can probably be left out, since headlinesOnly seems to be a global (which is not a good thing, by the way). Here, I've made the argument "positive", as in "yes, do include content", since that's the part you're deciding on. Headlines are always present, so headlinesOnly is a bit of a double negation in the sense that true actually means "Yes, don't include content".

Overall, however, makeElement is poor name. Everything on the page is an element, so "make element" doesn't explain anything about the function's purpose. I don't know what your page is displaying, but I'm sure there's some more descriptive name for those elements; forget that they're HTML elements and consider instead what they're representing.

And while I used Janos' createAnchor, I'm not a huge fan of it. Using onevent-style attributes is not good practice. It's been outmoded for many years now.

Instead you should use addEventListener to attach event handling functions to your elements.

This would also mitigate the other big no-no you're committing: window['something'] = function ....

First of all, why not just say window.something = ... instead of the bracket/subscript? Using brackets is only useful if you want to access a property using a dynamic string. Here, the strings are hardcoded so it makes no difference.

But much more importantly, why are you declaring so many global functions? It's plain bad practice to pollute the global namespace like that.

Again, using addEventListener would avoid all that.

I'm also wondering why the behavior is different for touch vs mouse interaction. If I drag on a touch screen, the touchend event is ignored. But if I drag with a mouse, the mouseup event is not ignored. So why not use the click event instead?

Of course, click does have a delay on many touch screen clients, since the device is waiting to see if there's a 2nd tap, i.e. if it's a double-tap that's meant to zoom or some similar "meta-interaction". Whereas on with a mouse and keyboard there's no inherent meaning to a double-click, hence there's no delay.

So maybe keep the touchstart/move/end handlers for touch screens, but just use click for mouse events.

Lastly, as idle speculation, do you need to absolutely position your elements? You could make "column" elements instead, arranged side-by-side with CSS, and simply add your item elements to those. The browser would figure out how to do the layout. But I don't know the particulars of your page.

---

Here's one way to factor the code. Not saying it's perfect (far from it), but there's no duplication:

function createItemElement(item, includeContent) { // still not a great name
  var element = document.createElement('div'), // not an <a>
      heading = document.createElement('h1'),
      content;

  heading.innerHTML = item.title; // NOTE: I'm *assuming* the title is "safe" to insert as HTML
  element.appendChild(heading);

  if(includeContent) {
    content = document.createElement('div');
    content.innerHTML = item.content; // NOTE: See the other note
    element.appendChild(content);
  }

  addItemClickListener(item, element);

  return element;
}

function addItemClickListener(item, element) {
  var touchmove;

  // define what should happen on click/tap
  function openItemUrl(event) {
    isTouch && cycript ? tryOpenUrl(item.url) : window.open(url, '_blank');
  }

  if(isTouch) {
    touchmove = function (event) {
      // remove move/end listeners in case of touchmove
      element.removeEventListener('touchend', openItemUrl);
      element.removeEventListener('touchmove', touchmove);
    };

    element.addEventListener('touchstart', function () {
      // only add the move/end listeners when a touchstart occurs
      element.addEventListener('touchmove', touchmove);
      element.addEventListener('touchend', openItemUrl);
    });
  } else {
    element.addEventListener('click', openItemUrl);
  }
}

// ....

// basic usage
var itemElement = createItemElement(item, !headlinesOnly);
itemElement.style.top = ...;
itemElement.style.left = ...;
container.appendChild(itemElement);

Here, createItemElement doesn't care about positioning; it just creates an element, and then it's up to you to do something with it. Likewise, it knows nothing about touch vs mouse; that's all contained in addItemClickListener. And the event handling doesn't use an explicit drag boolean; it just adds and removes the event listeners as needed.

Either function knows as little as possible, while the things that always change (positioning) is simply handled directly without being passed via arguments.

Optimally, the isTouch and cycript (whatever that is) variables wouldn't need to be global either, as they're what's creating the most coupling right now; addItemClickListener depends on both being present, while createItemElement is much better off just relying only on its arguments (and, well, on document, but that's a pretty safe thing to rely on in a browser).

I've also been a bit pedantic in building the elements, explicitly creating them with document.createElement. I'm just not a fan of defining chunks of HTML as strings. Besides, with this, you can more easily control how the title and content is added to the elements.