0
\$\begingroup\$

I hope you can get me out of my current predicament which involves coding style when it comes to appending many elements in a pretty flexible and typesafe way.

Now I see my code below and I feel that it can be written in a more compact and shorter manner, especially in my set function. Also, I'm not sure if I should merge the arrays and keep it in one line.

What makes me wonder most however, is how to append all those divs to a document fragment before placing them to the real DOM as it is fairly complex.

Would you say I'm right about the possible improvements that can be made? If so, then how can I improve it?

jsFiddle

var superContainer = document.getElementsByClassName('superContainer');
    var article = document.getElementsByTagName('article');
    var obj = {};

    var wrappers = ['a', 'b', 'c'];

    var a1 = ['fruits', 'meat', 'milk', 'soup', 'sweets'];
    var a2 = ['troll', 'gollum', 'imp', 'ork', 'dolly'];
    var a3 = ['foobar'];

    var misc = ['boring', 'fun', 'annoying', 'great'];

    function Constructor() {
        function set() {
            for(var i = 0; i < article.length; i++) {
                var tempSC = document.createElement('div');

                tempSC.appendChild(article[i].parentNode.replaceChild(tempSC, article[i]));
                tempSC.className = 'superContainer';

                create(superContainer[i], wrappers, 0, 0);
                create(obj.a[i], a1, 0, 0);
                create(obj.b[i], a2, 0, 0);
                create(obj.c[i], a3, 0, 0);
                create(obj.milk[i], misc, 1, 2);
                create(obj.sweets[i], misc, 0, 1);
                create(obj.milk[i].children[0], misc, 3, 0);
                create(obj.sweets[i].children[1], misc, 3, 0);
            }
        }

        set();

        function create(parent, elem, loopStart, loopEnd) {
            for(var i = loopStart; i < elem.length - loopEnd; i++) {
                var result = document.createElement('div');
                result.className = elem[i];

                parent.appendChild(result);
                obj[elem[i]] = document.getElementsByClassName(elem[i]);
            }
        }
    }

    Constructor();
}
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Indentation

I don't understand the indentation. The second line should be indented the same as the first line.

Multiple var declarations

If you declare multiple variables, you can use the var keyword only once, and separate variables using the comma operator, for example:

var article = document.getElementsByTagName('article')
   ,obj = {}
   ,wrappers = ['a', 'b', 'c']

Semicolons

From another answer of mine:

You're using many semicolons. JavaScript has a nice feature called automation semicolon insertion, which means you almost don't need to use semicolons at all, with just a few exceptions. From npm style guide:

Don't use them [semicolons] except in four situations:

  • for (;;) loops. They're actually required.
  • null loops like: while (something) ; (But you'd better have a good reason for doing that.)
  • case 'foo': doSomething(); break
  • In front of a leading ( or [ at the start of the line. This prevents the expression from being interpreted as a function call or property access, respectively.

Note that it's only a matter of style. You don't have to follow it if you don't like it, but personally I think that if something is not required, there's no reason to use it.

Meaningless identifiers

Identifiers like a1, a2, a3 doesn't tell much about the purpose of the variable. Try to name them in a way that explain what they are used for.

Only class names should begin with an uppercase letter

It's recommended to use identifiers starting with an uppercase letter (like Constructor in your case) only to declare classes. As you don't use new operator when calling that function, I assume it's not a class and therefore should have a name starting with a lowercase letter.

Use const instead of var to declare variables that should never be changed

You don't intend to change most of the variables, especially these that refer to the DOM elements. You could use const keyword instead of var to make sure don't override them accidentally.

Using variables or functions before they are declared

You're using create function before you define it. It is recommended not to use variables or functions before declaring them. Therefore, you should move your create function declaration to the top.

Use for...of loop

If you don't need array indexes, you can use the for...of loop instead of the traditional loop.

Example:

for (let element of array) {
  console.log(element)
}

Miscellaneous

I don't see a point in declaring the set function and immediately calling it. If you want to create a separate scope, use IIFE, but I don't think you need it here. The same is with the Constructor function.

a1, a2, and a3 should definitely merged in one array.

\$\endgroup\$
7
  • \$\begingroup\$ Thats nice but some of those features like const and for of are not so well supported. Do you think the programm is as small as it can get or do you think one can remove more lines of code? \$\endgroup\$
    – Asperger
    Commented May 7, 2016 at 9:04
  • \$\begingroup\$ @Asperger Lack of support in browsers is not an excuse to not use those features. You should use a JavaScript compiler like Babel, which compiles code that uses latest ECMAScript features into ECMAScript 5 code. \$\endgroup\$ Commented May 7, 2016 at 9:13
  • \$\begingroup\$ I did check and the es5 code generated is so bad compared to what it looks like if I write it from scratch. I mean I know all those functions from java and they are useful but...I mean im not sure if thats good \$\endgroup\$
    – Asperger
    Commented May 7, 2016 at 10:06
  • \$\begingroup\$ @Asperger What do you mean by "bad"? If you mean readability -- that's perfectly fine, the generated code isn't intended to be read by humans. If you're still not convinced, see who's using Babel. \$\endgroup\$ Commented May 7, 2016 at 10:10
  • \$\begingroup\$ you are right but im just trying babel online and it cant convert lets or const, I keep getting errors \$\endgroup\$
    – Asperger
    Commented May 7, 2016 at 10:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.