2
\$\begingroup\$

I'm seeking a critique of the simple program below, which displays two counters and increments them through a button, using D3.js. In real life of course no one has a need for displaying buttons that increment counters. It's a stand-in to talk about how to craft the code over the scaffold of a trivial problem.

I would like to remain within function invocations, and to generally fit in seamlessly with D3. In particular, I do not wish to use constructor invocation or prototypal patterns. More concretely, I would like to use neither class nor new.

I would also like to avoid constantly asking myself "what is this in this instant?" by initializing that, as you see in the code (let's leave arguing that this is sometimes entirely compelling for another occasion).

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

    that.init = function() {
        that.name = name;
        that.span = d3.select('body')
                      .append('span')
                      .attr('id', '#num' + name)
                      .text(value);
    }
    that.init();

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

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

d3.select('#inc')
  .on('click', double_increment);

// d3.select('#inc')
//   .on('click', incrementer1.increment);
// d3.select('#inc')
//   .on('click', incrementer2.increment);
span { margin-left: 40px; }
<script src="https://cdnjs.cloudflare.com/ajax/libs/d3/5.7.0/d3.min.js"></script>
<button id="inc">Increment</button><br />

At this point I'm lacking a nice way to avoid the function double_increment. Of course without that function—were we to use the commented out pair of on-click handlers instead—we would have a bug. Only one on-click handler can be set. We have no mechanism for adding multiple addEventListeners.

So to summarize:

  1. We define that.init as our initializer. That seems tidier than letting the initialization code loose in the body of the function. But defining it then immediately calling it seems odd. Can you improve on this?
  2. How would you avoid the ugly double_increment function within a D3 program?

that.span.text() should probably be replaced by that.span.datum, if I can find the datum in there. But we'd like to avoid duplicating the counter within Incrementer. This way we do not need to worry about the counter and its view going out of sync. The datum itself, once located, is already a bit of a (necessary) duplication.

\$\endgroup\$
2
\$\begingroup\$

The simple way of avoiding needing to define a named function like double_increment is just to use a lambda. You could simply write

d3.select('#inc').on('click',
    () => {
        incrementer1.increment();
        incrementer2.increment();
    });

You could also get fancy and loop through the incrementers too

var incrementors = [incrementer1, incrementer2]

d3.select('#inc').on('click',
    () => {
        for (inc of incrementors) {
            inc.increment();
        } 
     };

This lets you easily add new incrementors later if needed.

\$\endgroup\$
  • \$\begingroup\$ That's cool, and it nicely avoids writing a JS abstraction of the Button (one that would store the incrementers). The init business remains. Maybe that was a 2-in-1 questions. I'm happy to trim this question, mark the answer, and ask again. \$\endgroup\$ – Calaf Jun 19 at 13:58
  • \$\begingroup\$ @Calaf "That seems tidier than letting the initialization code loose in the body of the function. But defining it then immediately calling it seems odd.". In a case like this, I think loose would be fine. At the top of the function it should be well understood that that's "constructor code". The neater, modern solution though would be to make Incrementor a class, and write up an official constructor. \$\endgroup\$ – Carcigenicate Jun 19 at 14:56
  • \$\begingroup\$ You're probably right. Yielding would be simpler. Sometimes I worry that I've been much too indoctrinated by the Crockford-Bostock dogma, to the point of getting blasted for it (stackoverflow.com/q/56546011/704972). \$\endgroup\$ – Calaf Jun 19 at 15:20
0
\$\begingroup\$

A short review;

  • You are mixing model and view functionality in that one function
  • The style where you newline at every dot makes your code hard to read
  • I would suggest you look in to currying

The below splits out model and view functionality (somewhat) and reduces the amount of newlines.

function createNumberLabel(name, value) {

  return d3.select('body').append('span').attr('id', '#num' + name).text(value);
}

function incrementNumberLabel(label){

  return () => label.text(parseInt(label.text()) + 1);
}


var incrementer1 = incrementNumberLabel(createNumberLabel('one', 10));
var incrementer2 = incrementNumberLabel(createNumberLabel('two', 20));

function double_increment() {
    incrementer1();
    incrementer2();
}

d3.select('#inc').on('click', double_increment);
span { margin-left: 40px; }
<script src="https://cdnjs.cloudflare.com/ajax/libs/d3/5.7.0/d3.min.js"></script>
<button id="inc">Increment</button><br />

\$\endgroup\$

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.