Create multiple divs inside a containing div

Question

The below function is used to create multiple content division elements (i.e. <div>) inside a containing <div>. The function is working correctly and is as expected, I just feel the quality of the code itself is a bit poor.

Therefore, can the following function be, in any way, simplified or improved?

generate(chartData) {
 var container = document.createElement("div");
 $(container).attr("class", "container2");
 var myNode = document.getElementById(chartData.bindto.replace("#", "")).appendChild(container);
 for(var i = 0; i < 10; i++) {
 var div = document.createElement('div');
 $(div).attr("class", "block");
 var span = document.createElement('span');
 var text = "Hello World!";
 span.append(text);
 div.append(span);
 myNode.append(div);
 }
 }

• michael.zech: What is 'chartdata.bindTo'?

Answer 1

Descriptive names

As a global function name, generate is too vague: it should tell what it's generating, something like generateChart. As a method name it would be ok because the object itself would be what's being generated or doing the generation (e.g. chart.generate()).

(I'm aware that the function doesn't actually generate a chart as it currently is, but I assume that the "hello world" part is later replaced with the actual code that does it.)

Class names like "container2" and "block" aren't descriptive enough to tell what their meaning is. "Block" might be descriptive in context, but "container2" certainly isn't.

Too broad parameters

You're passing an object chartData to the function, but only use its bindto property. You could pass the property directly to the function, as it seems to only tell where to put the chart.

Mixing jQuery and native DOM methods

As a general rule when doing low-level element manipulation you should either always use jQuery or never use it, but not mix the two. In the code you're using jQuery only to add class names, which could easily be done with plain JavaScript.

Browser compatibility

.append() isn't supported by any version of Microsoft or Android browsers so if you're targeting any of those browsers you'll have to use other methods, add a polyfill, or use jQuery. Note that .append() on native DOM elements is a different method than jQuery's .append().

Miscellaneous

myNode points to container (because .appendChild() returns the child element). Having two variables for the same element is confusing, so you should only use container.

Variable declarations should be at the top of the function if possible. Variables shouldn't be declared inside a loop, unless you're using let.

The variable text should be declared outside the loop. It's always the same so re-declaring it 10 times is unnecessary.

The number 10 in the for loop is a magic number - you should replace it with a descriptive constant, especially if it's used somewhere else too to refer to the number of blocks in the chart.

Single and double quotes should be used consistently to delimit strings, so either use single quotes everywhere or double quotes everywhere.

Revised code

Putting it all together, plain JavaScript version:

var BLOCKS_PER_CHART = 10;
function generateChart(chartContainer) {
 var container = document.createElement("div");
 var text = "Hello World!";
 var blockDiv, textSpan; // used in the for loop
 container.className = "container2";
 document.getElementById(chartContainer.replace("#", "")).appendChild(container);
 for(var i = 0; i < BLOCKS_PER_CHART; i++) {
 blockDiv = document.createElement("div");
 blockDiv.className = "block";
 textSpan = document.createElement("span");
 textSpan.append(text); // see note about browser compatibility
 blockDiv.append(textSpan);
 container.append(blockDiv);
 }
}

jQuery version:

var BLOCKS_PER_CHART = 10;
function generateChart(chartContainer) {
 var container = $("<div>").addClass("container2").appendTo(chartContainer);
 var text = "Hello World!";
 var blockDiv; // used in the for loop
 for(var i = 0; i < BLOCKS_PER_CHART; i++) {
 blockDiv = $("<div>").addClass("block").appendTo(container);
 $('<span>').text(text).appendTo(blockDiv);
 }
}

Both of these assume that the function is called with what used to be chartData.bindto. Also, I don't know the purpose of "container2" so I've left its name for you to change.

Note that this could be further improved if latest ECMAScript features like const can be used.

Answer 2

Not an expert but the simplest working code I have seen is:

<script>
 for (var i = 0; i <= 16; i++) {
 document.write('<div>' + i.toString() + '</div>');
 }
</script>

You can declare an array with your text instead of using i.toString(). Above code works fantastic when used in forms that have a range input and datalists are involved.

• Sᴀᴍ Onᴇᴌᴀ: Beware the note about document.write: "Note: as document.write writes to the document stream, calling document.write on a closed (loaded) document automatically calls document.open, which will clear the document." So any existing HTML will likely be cleared...

Answer 3

I agree with everything in Guy Incognito's answer. I also see another possible improvement:

DocumentFragment

One possible optimization is adding the child elements to a DocumentFragment to avoid reflows after each element is added.

The key difference is that because the document fragment isn't part of the active document tree structure, changes made to the fragment don't affect the document, cause reflow, or incur any performance impact that can occur when changes are made.

Though be aware that it is experimental technology and the constructor has limited browser compatibility (e.g. not supported by IE or Edge).

Alternative to DocumentFragment

If you didn't end up using a DocumentFragment, you could still minimize reflows by adding the container2 element after adding all the child elements - something similar to the code below:

Warning: untested code:

generateChart(chartData) {
 var container = document.createElement("div");
 container.className = "container2";
 for(var i = 0; i < 10; i++) {
 var div = document.createElement('div');
 div.className = "block";
 var span = document.createElement('span');
 span.innerHTML = "Hello World!";
 div.appendChild(span);
 container.appendChild(div);
 }
 document.getElementById(chartData.bindto.replace("#", "")).appendChild(container);
}