4
\$\begingroup\$

I implemented a simple voting widget like the one used on Stack Exchange sites, to use for other purposes (see live on bashoneliners.com), as a reusable package, dubbed UpvoteJS.

Here's how it works:

Upvote.create('topic-123');
<link rel="stylesheet" href="https://janosgyerik.github.io/upvotejs/dist/upvotejs/upvotejs.css">

<script src="https://janosgyerik.github.io/upvotejs/dist/upvotejs/upvotejs.vanilla.js"></script>

<div id="topic-123" class="upvotejs upvotejs-serverfault">
    <a class="upvote upvote-on"></a>
    <span class="count">101</span>
    <a class="downvote"></a>
    <a class="star star-on"></a>
</div>

How to use

As you can see in the above snippet, the typical use has the following elements:

  • Include the stylesheet upvotejs.css with a <link> tag. The companion upvotejs.svg is expected in the same directory on the web server.

  • Add HTML markup with the fields you need. With the stylesheet included, this is enough to get a nicely rendered widget, read-only, since without JavaScript, the clicks will have no effect.

  • To make the widget interactive, responding to user clicks, include the JavaScript package upvotejs.vanilla.js with a <script> tag, and activate the widget using the provided Upvote.create(topicId) function in JavaScript (to happen after the DOM is loaded). The topicId parameter is mandatory, and it must refer to a unique id in the DOM.

  • To make the widget save state to a backend (not featured in the snippet), pass to Upvote.create as second parameter a JSON object, with a field named callback, which must be a JavaScript function, for example: Upvote.create(topicId, {callback: your_callback_function}). It is up to the developer to implement your_callback_function to persist its payload. The function will get called on any state change.

The above should be enough for typical use cases.

Additional notes:

  • All sub-elements in the HTML markup are optional, for example it's possible to have a widget without a star button, or any other element.

  • The initial state of a widget can be set either with HTML markup (recommended), or using basic markup (without upvote-on, downvote-on, star-on classes) and setting values in the second parameter of the Upvote.create call, for example: Upvote.create(topicId, {count: 123, upvoted: true})

  • Upvote.create does some sanity checks, and throws an exception when it detects illegal state, for example (not an exhaustive list):

    • The specified ID doesn't exist in the DOM
    • The parameter types in the JSON object don't match what's expected (count must be integer, and so on)
    • upvoted and downvoted are both true
  • Upvote.create returns an object that represents the widget, and can be used to inspect and control its state. In the typical use case, this is probably not necessary.

Implementation

I'm most interested in a review of the JavaScript part (upvotejs.vanilla.js (2.1.0)), the complete project is available on GitHub.

const Upvote = function() {
  const upvoteClass = 'upvote';
  const enabledClass = 'upvotejs-enabled';
  const upvoteOnClass = 'upvote-on';
  const downvoteClass = 'downvote';
  const downvoteOnClass = 'downvote-on';
  const starClass = 'star';
  const starOnClass = 'star-on';
  const countClass = 'count';

  const Utils = {
    combine: function() {
      const combined = {};
      for (let i = 0; i < arguments.length; i++) {
        Object.entries(arguments[i])
          .filter(e => e[1] !== undefined)
          .forEach(e => combined[e[0]] = e[1]);
      }

      return combined;
    },
    isBoolean: v => typeof v === "boolean",
    isFunction: v => typeof v === "function",
    classes: dom => dom.className.split(/ +/).filter(x => x),
    removeClass: (dom, className) => {
      dom.className = dom.className.split(/ +/)
        .filter(x => x)
        .filter(c => c !== className)
        .join(' ');
    },
    noop: () => {}
  };

  const Model = function() {
    const validate = params => {
      if (!Number.isInteger(params.count)) {
        throw 'error: parameter "count" must be a valid integer';
      }
      if (!Utils.isBoolean(params.upvoted)) {
        throw 'error: parameter "upvoted" must be a boolean';
      }
      if (!Utils.isBoolean(params.downvoted)) {
        throw 'error: parameter "downvoted" must be a boolean';
      }
      if (!Utils.isBoolean(params.starred)) {
        throw 'error: parameter "starred" must be a boolean';
      }
      if (params.callback && !Utils.isFunction(params.callback)) {
        throw 'error: parameter "callback" must be a function';
      }
      if (params.upvoted && params.downvoted) {
        throw 'error: parameters "upvoted" and "downvoted" must not be true at the same time';
      }
    };

    const create = params => {
      validate(params);

      const data = Utils.combine(params);

      const upvote = () => {
        if (data.upvoted) {
          data.count--;
        } else {
          data.count++;
          if (data.downvoted) {
            data.downvoted = false;
            data.count++;
          }
        }
        data.upvoted = !data.upvoted;
      };

      const downvote = () => {
        if (data.downvoted) {
          data.count++;
        } else {
          data.count--;
          if (data.upvoted) {
            data.upvoted = false;
            data.count--;
          }
        }
        data.downvoted = !data.downvoted;
      };

      return {
        count: () => data.count,
        upvote: upvote,
        upvoted: () => data.upvoted,
        downvote: downvote,
        downvoted: () => data.downvoted,
        star: () => data.starred = !data.starred,
        starred: () => data.starred,
        data: () => Utils.combine(data)
      };
    };

    return {
      create: create
    };
  }();

  const View = function() {
    const create = id => {
      const dom = document.getElementById(id);
      if (dom === null) {
        throw 'error: could not find element with ID ' + id + ' in the DOM';
      }

      if (Utils.classes(dom).includes(enabledClass)) {
        throw 'error: element with ID ' + id + ' is already in use by another upvote controller';
      }
      dom.className += ' ' + enabledClass;

      const firstElementByClass = className => {
        const list = dom.getElementsByClassName(className);
        if (list === null) {
          throw 'error: could not find element with class ' + className + ' within element with ID ' + id + ' in the DOM';
        }
        return list[0];
      };

      const createCounter = className => {
        const dom = firstElementByClass(className);

        if (dom === undefined) {
          return {
            count: () => undefined,
            set: Utils.noop
          };
        }

        return {
          count: () => parseInt(dom.innerHTML || 0, 10),
          set: value => dom.innerHTML = value
        };
      };

      const createToggle = (className, activeClassName) => {
        const createClasses = () => {
          const classes = {
            [className]: true,
            [activeClassName]: false,
          };
          item.className.split(/ +/)
            .filter(x => x)
            .forEach(className => classes[className] = true);
          return classes;
        };

        const formatClassName = () => {
          return Object.entries(classes)
            .filter(e => e[1])
            .map(e => e[0])
            .join(' ');
        };

        const item = firstElementByClass(className);
        if (item === undefined) {
          return {
            get: () => false,
            set: Utils.noop,
            onClick: Utils.noop
          };
        }

        const classes = createClasses();

        return {
          get: () => classes[activeClassName],
          set: value => {
            classes[activeClassName] = value;
            item.className = formatClassName();
          },
          onClick: fun => item.onclick = fun
        };
      };

      const render = model => {
        counter.set(model.count());
        upvote.set(model.upvoted());
        downvote.set(model.downvoted());
        star.set(model.starred());
      };

      const parseParamsFromDom = () => {
        return {
          count: counter.count(),
          upvoted: upvote.get(),
          downvoted: downvote.get(),
          starred: star.get()
        };
      };

      const destroy = () => {
        Utils.removeClass(dom, enabledClass);
        upvote.onClick(null);
        downvote.onClick(null);
        star.onClick(null);
      };

      const counter = createCounter(countClass);
      const upvote = createToggle(upvoteClass, upvoteOnClass);
      const downvote = createToggle(downvoteClass, downvoteOnClass);
      const star = createToggle(starClass, starOnClass);

      return {
        render: render,
        parseParamsFromDom: parseParamsFromDom,
        onClickUpvote: fun => upvote.onClick(fun),
        onClickDownvote: fun => downvote.onClick(fun),
        onClickStar: fun => star.onClick(fun),
        destroy: destroy
      };
    };

    return {
      create: create
    };
  }();

  const create = (id, params = {}) => {
    var destroyed = false;
    const view = View.create(id);
    const domParams = view.parseParamsFromDom();
    const defaults = {
      id: id,
      count: 0,
      upvoted: false,
      downvoted: false,
      starred: false,
      callback: () => {}
    };
    const combinedParams = Utils.combine(defaults, domParams, params);
    const model = Model.create(combinedParams);
    const callback = combinedParams.callback;

    const throwIfDestroyed = () => {
      if (destroyed) {
        throw "fatal: unexpected call to destroyed controller";
      }
    };

    const upvote = () => {
      throwIfDestroyed();
      model.upvote();
      view.render(model);
      callback(model.data());
    };

    const downvote = () => {
      throwIfDestroyed();
      model.downvote();
      view.render(model);
      callback(model.data());
    };

    const star = () => {
      throwIfDestroyed();
      model.star();
      view.render(model);
      callback(model.data());
    };

    const destroy = () => {
      throwIfDestroyed();
      destroyed = true;
      view.destroy();
    };

    view.render(model);
    view.onClickUpvote(upvote);
    view.onClickDownvote(downvote);
    view.onClickStar(star);

    return {
      id: id,
      count: () => {
        throwIfDestroyed();
        return model.count();
      },
      upvote: upvote,
      upvoted: () => {
        throwIfDestroyed();
        return model.upvoted();
      },
      downvote: downvote,
      downvoted: () => {
        throwIfDestroyed();
        return model.downvoted();
      },
      star: star,
      starred: () => {
        throwIfDestroyed();
        return model.starred();
      },
      destroy: destroy
    };
  };

  return {
    create: create
  };
}();

Unit tests

I'm also interested in a review of the unit tests (live tests of current version):

const create = (id, params) => {
  return Upvote.create(id, params);
};

QUnit.test('throw exception if destroy is called twice', assert => {
  const obj = gen();
  obj.destroy();
  assert.throws(() => obj.destroy());
});

const gen = function() {
  var idcount = 0;
  return (params = {}) => {
    ++idcount;
    const id = params.id || ('u' + idcount);
    const jqdom = $('#templates div.upvotejs').clone();
    jqdom.attr('id', id);
    $('#tests').append(jqdom);
    params.callback = params.callback || (data => {});
    return create(id, params, jqdom);
  };
}();

const uiTester = obj => {
  const widget = $('#' + obj.id);
  const count = widget.find('.count');
  const upvote = widget.find('.upvote');
  const downvote = widget.find('.downvote');
  const star = widget.find('.star');

  return {
    count: () => parseInt(count.text(), 10),
    upvoted: () => upvote.hasClass('upvote-on'),
    downvoted: () => downvote.hasClass('downvote-on'),
    starred: () => star.hasClass('star-on'),
    upvote: () => upvote.click(),
    downvote: () => downvote.click(),
    star: () => star.click()
  };
};

QUnit.test('initialize from params', assert => {
  const obj = gen();
  assert.equal(obj.count(), 0);
  assert.equal(obj.upvoted(), false);
  assert.equal(obj.downvoted(), false);
  assert.equal(obj.starred(), false);

  assert.equal(gen({count: 17}).count(), 17);
  assert.equal(gen({upvoted: true}).upvoted(), true);
  assert.equal(gen({downvoted: true}).downvoted(), true);
  assert.equal(gen({starred: true}).starred(), true);

  assert.throws(() => gen({count: 'foo'}), 'throw if count param is not an integer');
  assert.throws(() => gen({upvoted: 'foo'}), 'throw if upvoted param is not a boolean');
  assert.throws(() => gen({downvoted: 'foo'}), 'throw if downvoted param is not a boolean');
  assert.throws(() => gen({starred: 'foo'}), 'throw if starred param is not a boolean');
  assert.throws(() => gen({callback: 'foo'}), 'throw if callback param is not a function');
  assert.throws(() => gen({upvoted: true, downvoted: true}), 'throw if upvoted=true and downvoted=true');
});

QUnit.test('initialize from dom', assert => {
  const v1 = Upvote.create('count-1');
  assert.equal(v1.count(), 1);
  assert.equal(v1.upvoted(), false);
  assert.equal(v1.downvoted(), false);
  assert.equal(v1.starred(), false);

  const v2 = Upvote.create('count-2-upvoted');
  assert.equal(v2.count(), 2);
  assert.equal(v2.upvoted(), true);
  assert.equal(v2.downvoted(), false);
  assert.equal(v2.starred(), false);

  const v3 = Upvote.create('count-3-upvoted-starred');
  assert.equal(v3.count(), 3);
  assert.equal(v3.upvoted(), true);
  assert.equal(v3.downvoted(), false);
  assert.equal(v3.starred(), true);

  const v4 = Upvote.create('count-4-downvoted');
  assert.equal(v4.count(), 4);
  assert.equal(v4.upvoted(), false);
  assert.equal(v4.downvoted(), true);
  assert.equal(v4.starred(), false);

  const v5 = Upvote.create('count-5-downvoted-starred');
  assert.equal(v5.count(), 5);
  assert.equal(v5.upvoted(), false);
  assert.equal(v5.downvoted(), true);
  assert.equal(v5.starred(), true);

  const vLarge = Upvote.create('count-456789');
  assert.equal(vLarge.count(), 456789);
  assert.equal(vLarge.upvoted(), false);
  assert.equal(vLarge.downvoted(), false);
  assert.equal(vLarge.starred(), false);

  const vNegativeLarge = Upvote.create('count-minus-456789');
  assert.equal(vNegativeLarge.count(), -456789);
  assert.equal(vNegativeLarge.upvoted(), false);
  assert.equal(vNegativeLarge.downvoted(), false);
  assert.equal(vNegativeLarge.starred(), false);

  assert.throws(() => Upvote.create('upvoted-downvoted'));
});

QUnit.test('UI updated from params', assert => {
  const obj = uiTester(gen());
  assert.equal(obj.count(), 0);
  assert.equal(obj.upvoted(), false);
  assert.equal(obj.downvoted(), false);
  assert.equal(obj.starred(), false);

  assert.equal(uiTester(gen({count: 17})).count(), 17);
  assert.equal(uiTester(gen({upvoted: true})).upvoted(), true);
  assert.equal(uiTester(gen({downvoted: true})).downvoted(), true);
  assert.equal(uiTester(gen({starred: true})).starred(), true);
});

QUnit.test('upvote non-downvoted non-upvoted', assert => {
  const count = 5;
  const obj = gen({count: count});
  assert.equal(obj.count(), count);
  obj.upvote();
  assert.equal(obj.count(), count + 1);
});

QUnit.test('upvote downvoted', assert => {
  const count = 6;
  const obj = gen({count: count, downvoted: true});
  assert.equal(obj.count(), count);
  obj.upvote();
  assert.equal(obj.count(), count + 2);
});

QUnit.test('upvote upvoted', assert => {
  const count = 7;
  const obj = gen({count: count, upvoted: true});
  assert.equal(obj.count(), count);
  obj.upvote();
  assert.equal(obj.count(), count - 1);
});

QUnit.test('downvote non-downvoted non-upvoted', assert => {
  const count = 5;
  const obj = gen({count: count});
  assert.equal(obj.count(), count);
  obj.downvote();
  assert.equal(obj.count(), count - 1);
});

QUnit.test('downvote upvoted', assert => {
  const count = 6;
  const obj = gen({count: count, upvoted: true});
  assert.equal(obj.count(), count);
  obj.downvote();
  assert.equal(obj.count(), count - 2);
});

QUnit.test('downvote downvoted', assert => {
  const count = 7;
  const obj = gen({count: count, downvoted: true});
  assert.equal(obj.count(), count);
  obj.downvote();
  assert.equal(obj.count(), count + 1);
});

QUnit.test('star non-starred', assert => {
  const obj = gen();
  obj.star();
  assert.ok(obj.starred(), 'should be starred');
});

QUnit.test('star starred', assert => {
  const obj = gen({starred: true});
  obj.star();
  assert.ok(!obj.starred(), 'should not be starred');
});

QUnit.test('upvote indepently', assert => {
  const count1 = 5;
  const v1 = gen({count: count1});
  const count2 = 5;
  const v2 = gen({count: count2});
  v1.upvote();
  assert.equal(v1.count(), count1 + 1);
  assert.equal(v2.count(), count2);
});

QUnit.test('downvote indepently', assert => {
  const count1 = 5;
  const v1 = gen({count: count1});
  const count2 = 5;
  const v2 = gen({count: count2});
  v1.downvote();
  assert.equal(v1.count(), count1 - 1);
  assert.equal(v2.count(), count2);
});

QUnit.test('star indepently', assert => {
  const v1 = gen();
  const v2 = gen();
  v1.star();
  assert.equal(v1.starred(), true);
  assert.equal(v2.starred(), false);
});

QUnit.test('call callback on value changes', assert => {
  var receivedPayload;
  const callback = payload => receivedPayload = payload;

  const obj1_id = 100;
  const obj1_origCount = 10;
  const obj1 = gen({id: obj1_id, count: obj1_origCount, callback: callback});

  const obj2_id = 200;
  const obj2_origCount = 20;
  const obj2 = gen({id: obj2_id, count: obj2_origCount, callback: callback});

  obj1.upvote();
  assert.deepEqual(receivedPayload, {
    id: obj1_id,
    action: 'upvote',
    newState: {
      count: obj1_origCount + 1,
      downvoted: false,
      upvoted: true,
      starred: false
    }
  });

  obj2.upvote();
  assert.deepEqual(receivedPayload, {
    id: obj2_id,
    action: 'upvote',
    newState: {
      count: obj2_origCount + 1,
      downvoted: false,
      upvoted: true,
      starred: false
    }
  });

  obj1.upvote();
  assert.deepEqual(receivedPayload, {
    id: obj1_id,
    action: 'unupvote',
    newState: {
      count: obj1_origCount,
      downvoted: false,
      upvoted: false,
      starred: false
    }
  });

  obj2.star();
  assert.deepEqual(receivedPayload, {
    id: obj2_id,
    action: 'star',
    newState: {
      count: obj2_origCount + 1,
      downvoted: false,
      upvoted: true,
      starred: true
    }
  });

  obj2.star();
  assert.deepEqual(receivedPayload, {
    id: obj2_id,
    action: 'unstar',
    newState: {
      count: obj2_origCount + 1,
      downvoted: false,
      upvoted: true,
      starred: false
    }
  });

  obj2.downvote();
  assert.deepEqual(receivedPayload, {
    id: obj2_id,
    action: 'downvote',
    newState: {
      count: obj2_origCount - 1,
      downvoted: true,
      upvoted: false,
      starred: false
    }
  });

  obj2.downvote();
  assert.deepEqual(receivedPayload, {
    id: obj2_id,
    action: 'undownvote',
    newState: {
      count: obj2_origCount,
      downvoted: false,
      upvoted: false,
      starred: false
    }
  });
});

QUnit.test('update model updates UI', assert => {
  const obj = gen();
  const ui = uiTester(obj);

  obj.upvote();
  assert.equal(ui.count(), 1);
  assert.equal(ui.upvoted(), true);

  obj.downvote();
  assert.equal(ui.count(), -1);
  assert.equal(ui.upvoted(), false);
  assert.equal(ui.downvoted(), true);

  obj.upvote();
  assert.equal(ui.count(), 1);
  assert.equal(ui.upvoted(), true);
  assert.equal(ui.downvoted(), false);

  obj.star();
  assert.equal(ui.starred(), true);
  obj.star();
  assert.equal(ui.starred(), false);
});

QUnit.test('update UI updates model', assert => {
  const obj = gen();
  const ui = uiTester(obj);

  ui.upvote();
  assert.equal(obj.count(), 1);
  assert.equal(obj.upvoted(), true);

  ui.downvote();
  assert.equal(obj.count(), -1);
  assert.equal(obj.upvoted(), false);
  assert.equal(obj.downvoted(), true);

  ui.upvote();
  assert.equal(obj.count(), 1);
  assert.equal(obj.upvoted(), true);
  assert.equal(obj.downvoted(), false);

  ui.star();
  assert.equal(obj.starred(), true);
  ui.star();
  assert.equal(obj.starred(), false);
});

QUnit.test('cannot associate multiple models to the same id', assert => {
  const orig = gen();
  assert.throws(() => gen({id: orig.id}));
});

QUnit.test('widget stops responding to clicks after destroyed', assert => {
  const obj = gen({count: 99});
  const ui = uiTester(obj);

  ui.upvote();
  assert.equal(ui.count(), 100);
  ui.upvote();
  assert.equal(ui.count(), 99);

  obj.destroy();
  ui.upvote();
  assert.equal(ui.count(), 99);
  assert.throws(() => obj.upvote());
  assert.throws(() => obj.downvote());
  assert.throws(() => obj.star());
  assert.throws(() => obj.count());
  assert.throws(() => obj.upvoted());
  assert.throws(() => obj.downvoted());
  assert.throws(() => obj.starred());

  const reused = gen({id: obj.id});
  assert.equal(reused.count(), 99);
  ui.upvote();
  assert.equal(reused.count(), 100);
});

QUnit.test('all sub-elements (upvote/downvote/count/star) are optional in the HTML markup', assert => {
  ['upvote', 'downvote', 'count', 'star'].forEach(cls => {
    const obj0 = gen();
    obj0.destroy();
    const jqdom = $('#' + obj0.id);
    jqdom.find('.' + cls).remove();
    const obj = create(obj0.id, {}, jqdom);

    assert.equal(obj.count(), 0);
    obj.upvote();
    assert.equal(obj.count(), 1);
    assert.equal(obj.upvoted(), true);
    obj.downvote();
    assert.equal(obj.count(), -1);
    assert.equal(obj.downvoted(), true);
    assert.equal(obj.upvoted(), false);
    obj.downvote();
    assert.equal(obj.count(), 0);
    assert.equal(obj.downvoted(), false);
    obj.star();
    assert.equal(obj.starred(), true);
    obj.star();
    assert.equal(obj.starred(), false);
  });
}

Code review

I'm looking for a review of the posted code in any and all aspects.

Some particular areas do come to mind I'm especially curious about:

  • Bad practices: is there anything to fix
  • Usability: is there anything that looks hard to use, and how to make it better
  • Are the tests sufficient? Did I miss any important cases?
  • Are the tests overcomplicated? Do you see ways to simplify?
  • I'm used to using QUnit for testing JavaScript packages. Do you think I could benefit greatly by using something else?
\$\endgroup\$
1
  • \$\begingroup\$ Some comments wouldn't go amiss. \$\endgroup\$ Commented Feb 10, 2019 at 23:22

1 Answer 1

5
\$\begingroup\$

Foreword

There is quite a bit of code here and it has taken a bit of time to process - especially the unit tests. I feel like the thoughts below will be incomplete but if I think of other ideas in the future, I could add those later.

General Feedback

I know you have made various versions of this in the past - including a jQuery version posted in 2014 and this vanilla JS version appears to use features like the let and const keywords, as well as arrow functions. Did you consider using the ES-6 class syntax? I know they are really just "primarily syntactical sugar over JavaScript's existing prototype-based inheritance"1 and "there’s no way to define private methods, getters and setters"2 so perhaps the revealing module pattern is best here for concealing private methods.

The test code is very lengthy and I can't think of any cases not covered, but there may still be other scenarios that should be considered. I don't see any obvious simplifications for that code.

To DRY out some of that code, you could consider throwing the initialization strings in QUnit.test('initialize from dom', ...) for v1 through v5 into an array and iterating over them. Perhaps something like below would work (*untested):

const initStrings = [
  'count-1', 
  'count-2-upvoted', 
  'count-3-upvoted-starred', 
  'count-4-downvoted',
  'count-5-downvoted-starred', 
];
const adjMethods = ['upvoted', 'downvoted', 'starred'];
initStrings.forEach(initStr => {
  const voteObj = Upvote.create(initStr);
  const parts = initStr.split('-'); // could also destructure here
  assert.equal(voteObj.count(), parts[1]);
  adjMethods.forEach(adjMethod => assert.equal(voteObj[adjMethod](), parts.includes(adjMethod))); 
});

Targeted Feedback

  • for...of could be used to simplify some for loops - e.g. in Utils.combine(). That way there wouldn't be a need to make a variable like i, increment it, and use it to access each element.
  • Shorthand property names - can be used to simplify assignment - e.g. return objects from:

    • Model.create()
    • View.create()
    • Model
    • View

    as well as the defaults object used in Upvote.create()

  • Element.classList.add() could be used instead of manually altering className property and dealing with spaces to separate names in View.create(), and Element.classList.remove() could likely eliminate the need for Utils.removeClass().
  • Excess filter loop - in Utils.removeClass() - simplify to x => x && x !== className
  • Repeated Utils.Noop() - appears duplicated within defaults declared in View.create()
  • Excess wrapper function in unit tests

    const create = (id, params) => {
      return Upvote.create(id, params);
    };
    

    could be simplified to:

    const create = Upvote.create;
    

    unless for some reason there is a difference in arguments passed to that function....


It appears that the unit test helper gen() has changed in the github repo since you posted this, but I was curious about the idcount variable in the code above:

const gen = function() {
  var idcount = 0;
  return (params = {}) => {
    ++idcount;
    const id = params.id || ('u' + idcount);

I considered suggesting that the ++idcount get moved into the usage on the line below, where id is assigned. Maybe you always want that incremented whenever the function is called, but that variable doesn't appear to be used anywhere else in that function or elsewhere, which leads me to believe that it could be incremented only when used and it would still work the same.


And even though they are "constant variables" declared with const, you might consider using all uppercase numbers for the constant string values, just like you use in Java and other languages - e.g. instead of:

const upvoteClass = 'upvote';
const enabledClass = 'upvotejs-enabled';

use uppercase:

const UPVOTED_CLASS = 'upvote';
const ENABLED_CLASS = 'upvotejs-enabled';

1https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes

2https://www.sitepoint.com/javascript-private-class-fields/#privateclassfields

\$\endgroup\$

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.