2
\$\begingroup\$

Background

I'm interested in implementing a small script to display multiple independent instances of a Canvas-based control on one page. The ultimate intention is to add many similar controls to the same page, so efficiency is helpful.

Inspiration Code

Here is the original source code that I'm working from that creates a Canvas-based UI control (additional pureknob.js code omitted here for brevity). This code block creates a single UI control. (The inspiration code isn't mine and I'm not seeking advice on how to improve it.)

// Create knob element, 300 x 300 px in size.
const knob = pureknob.createKnob(300, 300);

// Set properties.
knob.setProperty('angleStart', -0.75 * Math.PI);
knob.setProperty('angleEnd', 0.75 * Math.PI);
knob.setProperty('colorFG', '#88ff88');
knob.setProperty('trackWidth', 0.4);
knob.setProperty('valMin', 0);
knob.setProperty('valMax', 100);

// Set initial value.
knob.setValue(50);

/*
 * Event listener.
 *
 * Parameter 'knob' is the knob object which was
 * actuated. Allows you to associate data with
 * it to discern which of your knobs was actuated.
 *
 * Parameter 'value' is the value which was set
 * by the user.
 */
const listener = function(knob, value) {
  console.log(value);
};

knob.addListener(listener);

// Create element node.
const node = knob.node();

// Add it to the DOM.
const elem = document.getElementById('some_element');
elem.appendChild(node);

HTML:

The following snippet is used to display the controls (through a Django server, if that's relevant).

...

<td style="width:150px; text-align: center;">
<span id="some_element_1"></span>
</td>
<td style="width:150px; text-align: center;">
<span id="some_element_2"></span>
</td>

...

Generating Multiple Controls

I'm able to produce multiple UI objects by simply duplicating every element of the script, i.e.,

const knob1 = pureknob.createKnob(300, 300);
const knob2 = pureknob.createKnob(300, 300);

...

knob1.setProperty('angleStart', -0.75 * Math.PI);
knob2.setProperty('angleStart', -0.75 * Math.PI);

which works, but obviously isn't DRY.

Output: Example of Multiple Control Objects

Example of Multiple Control Objects

Code to Produce Multiple Independent Canvas Objects

Therefore, I've chosen to create a list of objects and iterate over them. Is there a more efficient or safer way to create multiple control object instances?

<script type="text/javascript">
    // <![CDATA[

    let i = 0;

    // Create knob elements, 100 x 100 px in size.
    const knob1 = pureknob.createKnob(100, 100);
    const knob2 = pureknob.createKnob(100, 100);
    const knobs = [knob1, knob2];
    knobs.forEach(demoKnob)

    /*
     * Demo code for knob element.
     */
    function demoKnob(k) {
        ++i

        // Set properties.
        k.setProperty('angleStart', -0.75 * Math.PI);
        k.setProperty('angleEnd', 0.75 * Math.PI);
        k.setProperty('colorFG', '#245166');
        k.setProperty('trackWidth', 0.4);
        k.setProperty('valMin', 0);
        k.setProperty('valMax', 100);

        // Set initial value.
        k.setValue(40);

        /*
         * Event listener.
         *
         * Parameter 'knob' is the knob object which was
         * actuated. Allows you to associate data with
         * it to discern which of your knobs was actuated.
         *
         * Parameter 'value' is the value which was set
         * by the user.
         */
        let listener = function(k, value) {
            console.log(value);
        };

        k.addListener(listener);

        // Create element node.
        let node = k.node();

        // Add it to the DOM.
        document.getElementById('some_element_' + String(i)).appendChild(node);
    }

    /*
     * This is executed after the document finished loading.
     */
    function ready() {
        demoKnob();
    }

    document.addEventListener('DOMContentLoaded', ready, false);
    // ]]>
</script>

\$\endgroup\$
1
\$\begingroup\$

Review

There are bugs and the code is accessibly noisy.

Style

Some source code style points

  • Avoid unneeded code as it is just noise. Code noise makes code harder to read amd maintain.

    • The script tag default type is "text/javascript" and thus is not required.

    • JavaScript does type conversion automatically. Thus 'some_element_' + String(i) is the same as 'some_element_' + i

    • The function listener does nothing but log to the console. This is debug/development code and should not be in release code (You may not be aware but Code Review question are considered to be release code)

    • Avoid redirection functions (Functions that only call another function). Eg you have the event listener ready that just calls demoKnob

    • Avoid single use variables when they do not improve readability. EG shorten very long lines, reduces complex statement clauses.

    • Comments are just noise, good code does not need comments.

  • There are some poor and inappropriate variable names

    • Avoid naming a variable in global scope i,j,or k. In this case i can be idCount, or not needed at all as you can get the id from the iterating callback's second argument.

    • Take care to correctly capitalize camelCase names. pureknob should be pureKnob

    • listener is a very poor function name. Yes it is a listener but that gives no clue as to what it does.

BUG!

The DOMContentLoaded will cause a error to be thrown as when it indirectly calls demoKnob there are no arguments passed. I do not understand why you have the listener there? As I see it it is not required, all the setup has already beeen executed before you add the listener.

Question

You ask...

"Is there a more efficient or safer way to create multiple control object instances?"

From your point of view and apart from the points above and considering that there is some information missing the answer is No and No.

That said (and I have not looked at any of the 3rd party code as we only review the OP's code) the use of the property Knob.colorFG indicates that the object is very old school in its design and as such may not be optimal for what it does.

The best way to know if it is efficient is to test it.

  • Degradation limit: How many can you add to a page before it degrades the page's performance metrics?
  • Are you planing on using significantly less than the degradation limit?

If you answer yes to the second then its safe to use. However do keep an eye out for more efficient components.

Rewrite

Removes noise and buggy code. Moves default properties to a constant for easier maintenance and readability.

<script>
    const KNOB_DEFAULTS = {
        angleStart: -0.75 * Math.PI,
        angleEnd: 0.75 * Math.PI,
        colorFG: '#245166',
        trackWidth: 0.4,
        valMin: 0,
        valMax: 100,
        value: 40,
    };
    [pureknob.createKnob(100, 100), pureknob.createKnob(100, 100)].forEach(demoKnob);

    function demoKnob(knob, i) {
        for (const [name, val] of Object.entries(KNOB_DEFAULTS)) { knob.setProperty(name, val) }
        document.getElementById('some_element_' + (i + 1)).appendChild(knob.node());
    }
</script>
\$\endgroup\$
1
  • \$\begingroup\$ Thank you for a thorough and thoughtful review. I will definitely apply your elegant revisions. As an amateur, I clearly have much to learn. My initial concerns revolved around efficient iteration. While, I can't answer a number of your questions regarding design decision-making--as those choices were made by the original author of the inspiration code I'm using--my decisions were inspired by other languages I'm more familiar with. I'm aware that code for review should be considered final and that was my intention. Cheers! \$\endgroup\$
    – DaveL17
    Aug 2 at 1:17

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.