4
\$\begingroup\$

In the following code we use the function Incrementer (capitalized for clarity, despite that it's to be used without new), to instantiate a number.

Incrementer provides a method increment to augment a counter in a span. We have two such counters, and so we define Incrementer to avoid code duplication (although it would be nice to program always like this, even in the absence of the worry of duplication).

function Incrementer(name, value) {
    let that = {};

    // Constructor
    that.name = name;
    let mydiv = document.getElementById('mydiv');
    mydiv.appendChild(document.createElement('br'));
    that.span = document.createElement('span');
    that.span.id = '#num' + name;
    that.span.innerHTML = value;
    mydiv.appendChild(that.span);

    that.increment = function() {
        var i = parseInt(that.span.textContent) + 1;
        that.span.innerHTML = i;
    }
    return that;
}
var incrementer1 = Incrementer('one', 10);
var incrementer2 = Incrementer('two', 20);

function double_increment() {
    incrementer1.increment();
    incrementer2.increment();
}
<div id="mydiv">
    <button id="inc" onclick="double_increment()">Increment</button><br />
</div>

The function double_increment() is then used to increment both numbers.

I'm seeking critique of this code, in particular:

  1. The block marked "Constructor" is directly inside the function Incrementer rather than in a function called Constructor. What is the right basis for cleaner code?
  2. The function double_increment() seems awkward. It exists at the global scope merely to respond to events (from user input in general, whether a button or otherwise). Is this an adequate basis for a (larger) code base?
\$\endgroup\$
2
\$\begingroup\$

disclaimer: "bad" is simply a way of saying it's not the best way.

If we were in the early 2000s this would probably be relatively clean code, however javascript has advanced a lot since then (current version: )

Take some time to check what's new.


Best practices today is to completely separate HTML / JS / CSS (unless you're using , , )

  • Cleaner code
  • Logic separation
  • Reusability
  • Easier to maintain

So the following should not appear in your HTML.

/// bad
onclick="double_increment()"
/// good
const button = document.getElementById("some_id");
button.addEventListener("click", double_increment);

There is always a better alternative than <br/> and it's not semantic

Don't use it.


const before let and let before var

Always declare your variables with const by default. If you're ok with the variable mutating then use let. Don't use var unless you know what you're doing and you actually need it.

note: pushing an element to an array (declared with const) does not mutate the variable, just the array. Thus no error.


Do not hardcode your html element id in your function.

function Incrementer(name, value) {
   /// bad
   let mydiv = document.getElementById('mydiv'); 
}

It either needs to be sent via the parameter or accessed statically using class

class Incrementer {
   /// still bad though
   static target = "mydiv"
}

Object methods should do what their name infers.

Here, the increment function is supposed to increment the variable, but it does more than that.

  1. Gets the text value of the span
  2. Parses the text value
  3. Increases by one
  4. Updates the UI

(1) is hacky (2) completely unneeded (3) the actual thing your function is supposed to do (4) updates UI ? not what your function says it does

/// bad
that.increment = function() {
        var i = parseInt(that.span.textContent) + 1;
        that.span.innerHTML = i;
}
/// better
that.i = value;

//increments value
that.increment = function() {
        that.i++;
}

//updates UI
that.update = function(){
        that.span.innerHTML = that.i;
}

// Method that should be called on button click
that.handleOnClick = function(){
   that.increment();
   that.update();
}

A double_increment function doesn't need to exist. Each Incrementer should add an event listener to the button click.

/// bad
function double_increment() {
    incrementer1.increment();
    incrementer2.increment();
}

See next section add event listener for solution


Using class and working example

has introduced class

JavaScript classes, introduced in ECMAScript 2015, are primarily syntactical sugar over JavaScript's existing prototype-based inheritance. The class syntax does not introduce a new object-oriented inheritance model to JavaScript.

Thus your Incremental function should be a class instead.

class Incrementer {

  constructor(incrementId, options) {
  
    const {
      name  = "N/A",
      value = 0,
      target
    } = options;
  
    this.name     = name;
    this.counter  = value;
    this.view     = document.createElement("span");
    
    this.view.id = `#num_${name}`
    
    const button = document.getElementById(incrementId);
    button.addEventListener("click", ()=>this.handleOnClick())

    this.update();        

    const container = document.getElementById(target);
    container.appendChild(this.view);
  }
  
  handleOnClick(){
    this.increment();
    this.update();
  }

  increment() {
    this.counter++;
  }

  update() {
    this.view.innerText = this.counter;
  }
}

const incrementer1 = new Incrementer(
  "increment", 
  {name: "Hello World", value: 3, target: "container"}
);

const incrementer2 = new Incrementer(
  "increment", 
  {name: "Hello World", value: 0, target: "container"}
)
#container {
  display: flex;
  flex-direction: row;
  justify-content: space-evenly;
  align-items: center;
  background-color: darkblue;
}

#container > span {
  padding: 10px;
  background-color: lightblue;
}
<button id="increment">Increment</button>
<div id="container"></div>


\$\endgroup\$
5
  • \$\begingroup\$ Would you prefer Incrementer inject an element in the dom or work on an existing element? \$\endgroup\$ – dfhwze Jun 18 '19 at 18:06
  • \$\begingroup\$ @dfhwze depends on the use case, but in the OP's situation it would appear that it's dynamic content so inject an element in the dom. \$\endgroup\$ – kemicofa ghost Jun 18 '19 at 18:08
  • 1
    \$\begingroup\$ my case? I'm just an editor of the OP, not the poster :) \$\endgroup\$ – dfhwze Jun 18 '19 at 18:09
  • \$\begingroup\$ Nice reply that covers a lot of ground, but I wanted to remain within function invocations, not constructor invocation (whether new or old styles). I'm loath to amend the question to specify this after you put so much effort. It's probably worth a distinct question. \$\endgroup\$ – Calaf Jun 19 '19 at 0:02
  • \$\begingroup\$ My only reservation to your answer is that you introduced strong coupling between Incrementer and the button. At the outset we want Incrementer to know nothing about what triggers it. We may change our mind and experiment with the event coming in from one of many sources until we're happy with the UI, and we'd like to wrap Incrementer and let it be, while we move forward with the experimentations. \$\endgroup\$ – Calaf Jun 19 '19 at 0:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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