0
\$\begingroup\$

I just recently started with Javascript and jQuery. My background is in Python. I would be more than happy to get some feedback if this coding style of mine is optimal, or if in JS people generally write things differently.

The current task is to build different elements in a website that are always hosted in the same container. I have written a container creation function like this:

  function create_container(type, counter) {
      var ret = $('<div />').attr({
          class: 'container-box',
      }).data({
          type: type,
          counter: counter,
      }).append(
          $('<span />').attr({
              class: 'remove'
          }).append('&times;')
      );
      return ret
  }

  var b = create_container('text', 0);
  console.log(create_container('text', 0));

This function will then be reused in different objects to create the same containing container. Type and counter will be set when the object will be initiated.

\$\endgroup\$
1

3 Answers 3

1
\$\begingroup\$

The small change I'd suggest would be to rename the function. In JavaScript it is custom to use camel case: createContainer.


The bigger problem in my opinion is hard coding the HTML template into the function. This makes maintenance more difficult, especially if a second person who many not be familiar with JS needs to change the design.

At the very least you should move the static HTML to a string. Something like this:

var createContainer = (function() {
  var template = $("<div class=\"container-box\"><span class=\"remove\">&times;</span></div>");
  return function(type, counter) {
    return template.clone().data({
      type: type,
      counter: counter,
    });
  }
})();

Alternatively you even could use the template element to move the template to the HTML file:

<template id="container-template">
  <div class="container-box">
    <span class="remove">&times;</span>
  </div>
</template>

var createContainer = (function() {
  var template = $($("#container-template").prop("content"));
  return function(type, counter) {
    return template.clone().data({
      type: type,
      counter: counter,
    });
  }
})();
\$\endgroup\$
1
  • \$\begingroup\$ I wasn't thinking about the template tag! This just made my week, you probably saved me a lot of time now. I had been stuck with this code design for quiet a while now. \$\endgroup\$
    – Claude
    Nov 17, 2017 at 12:51
1
\$\begingroup\$

First of all, In terms of readability, when chaining jquery operations, I like to put the operation at the start of the line. When I create an Object with only one attribute, a one liner with no trailing comma is also good. And naming your jquery objects with a heading $ makes anyone understand that you're using a jquery object here :

function create_container(type, counter) {
      var $ret = $('<div />')
          .attr({class: 'container-box'})
          .data({
              type: type,
              counter: counter,
          })
          .append(
              $('<span />')
                  .attr({class: 'remove'})
                  .append('&times;')
          );
      return $ret
  }

  var $b = create_container('text', 0);
  console.log(create_container('text', 0));

Then you coud use the jquery method addClass instead of attr({class: 'foo'} :

function create_container(type, counter) {
  var $ret = $('<div />')
      .addClass('container-box')
      .data({
          type: type,
          counter: counter,
      })
      .append(
          $('<span />')
              .addClass('remove')
              .append('&times;')
      );
  return $ret
}

var $b = create_container('text', 0);
console.log(create_container('text', 0));

Finally, if you get to use es6, you could get this kind of code, which I find a lot more readable :

function create_container(type, counter) {
  const $ret = $('<div />')
      .addClass('container-box')
      .data({type, counter})
      .append(
          $('<span />')
              .addClass('remove')
              .append('&times;')
      );
  return $ret
}

const $b = create_container('text', 0);
console.log(create_container('text', 0));
\$\endgroup\$
1
  • \$\begingroup\$ This is great! Thank you for your feedback. This really does improve readability. Coming from Python I was just stuck with my JS. \$\endgroup\$
    – Claude
    Nov 17, 2017 at 12:50
0
\$\begingroup\$

I would use html template tag to generate elements, because it is a lot more readable. All you need is a template, and the following format function. What the function does is that it takes a string (the first argument), and then numbers wrapped in brackets {}, e.g., {0}, {1} and replaces them with the arguments from 1 and onwards.

<script id="container-template" type="text/template">
  <div class="container-box" data-type="{1}" data-counter="{2}">
    <span class="remove">{0}</span>
  </div>
</script>

<script type="text/javascript">
  function format(formatArgs) {
        var args = Array.prototype.slice.call(arguments, 1);
        return formatArgs.replace(
            /{(\d+)}/g, function (match, number) {
                return typeof args[number] != 'undefined'
                    ? args[number]
                    : match
                    ;
            });
    }

    var containerTemplate = $('#container-template').html();
    var containerDiv = $(format(containerTemplate, "text", "div", 7));
</script>

This would result in the following structure

<div class="container-box" data-type="div" data-counter="7">
  <span class="remove">text</span>
</div>

When you are ready, you should move onto frameworks like Angular, React or Vue. Because then it becomes even more readable and the flow is much better, especially because format has some restrictions for loops.

\$\endgroup\$

Your Answer

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

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