Render elements from an array of objects

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 = '' + item.title + '';
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 = '' + item.title + ''
+ '' + item.content + '';
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) {

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 tag", then the "" 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 = '' + item.title + '';
    if(includeContent) {
        innerHTML += '' + item.content + '';
    }
    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
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.removeEve

StackExchange Code Review Q#93480, answer score: 3