1
\$\begingroup\$

Please provide a sanity check on my below JavaScript pattern to be used for a custom edit control. The goals are:

  • Centralized code
  • Avoid name collisions
  • Minimize memory use (putting functions on the prototype chain to accomplish?)
  • Ease of use for other developers

My specific question is, are there any "gotchas" in below pattern that I may have overlooked or anything that I'm doing that is just plain wrong?

var CustomEditor = function (id) {
    'use strict';
    var evnt = null,
        funct = null;
    this.id = id;
    this.init();
};

CustomEditor.prototype.onCustomBlur = function () {
    'use strict';
    // alert("blur event handler");
    this.style.backgroundColor = "#fff";
    this.style.color = "#f00";
};

CustomEditor.prototype.onCustomKeyPress = function () {
    'use strict';
    // alert("keypress event handler");
    this.style.backgroundColor = "#000";
    this.style.color = "#ff0";
};

CustomEditor.prototype.onCustomFocus = function () {
    'use strict';
    // alert("focus event handler");
    this.style.backgroundColor = "#000";
    this.style.color = "#0ff";
};

CustomEditor.prototype.addEvent = function () {
    'use strict';
    var element = document.getElementById(this.id);
    // old IE work around
    if (element.attachEvent) {
        return element.attachEvent('on' + this.evnt, this.funct);
    }
    return element.addEventListener(this.evnt, this.funct, false);
};

CustomEditor.prototype.init = function () {
    'use strict';
    // initialize a custom editor
    this.evnt = 'blur';
    this.funct = this.onCustomBlur;
    this.addEvent();

    this.evnt = 'keypress';
    this.funct = this.onCustomKeyPress;
    this.addEvent();

    this.evnt = 'focus';
    this.funct = this.onCustomFocus;
    this.addEvent();
};

This code will be JavaScript code in the HTML page to allocate and initialize:

new CustomEditor('myInput1');
new CustomEditor('myInput2');
new CustomEditor('myInput3');

See this JSFiddle to try this pattern or fork as needed to correct.

\$\endgroup\$
  • 1
    \$\begingroup\$ Are you opposed to using a framework for this functionality or do you just want a code review? You might want to check out canjs.com/docs/can.Control it does what you are doing in a pretty generic way and has community support. \$\endgroup\$ – pllee Sep 29 '14 at 18:07
  • \$\begingroup\$ @pllee, thanks for the heads up on can.Control. I'm asking for this review to improve my understanding of JavaScript. \$\endgroup\$ – Karl Sep 30 '14 at 12:49
1
\$\begingroup\$

Looks fine to me; but I think the this.evnt & this.funct abstractions are unnecessary. Would always promote readable properties over shorthand (funct -> function etc), and I think that you should change the addEvent method to accept parameters. You can then clean up the code like so:

var CustomEditor = function(id) {
    'use strict';
    this.id = id;
    this.init();
};

CustomEditor.prototype.addEvent = function(event, callback) {
    'use strict';
    var element = document.getElementById(this.id);
    // old IE work around
    if (element.attachEvent) {
        return element.attachEvent('on' + event, callback);
    }
    return element.addEventListener(event, callback, false);
};

CustomEditor.prototype.init = function() {
    'use strict';
    this.addEvent('blur', this.onCustomBlur);
    this.addEvent('keypress', this.onCustomKeyPress);
    this.addEvent('focus', this.onCustomFocus);
};

See the fiddle.

\$\endgroup\$
  • \$\begingroup\$ FYI, this pattern is not working with IE10. The events fire, but the this.style.XXX fail due to them being NULL. \$\endgroup\$ – Karl Sep 30 '14 at 12:58
  • \$\begingroup\$ Instead of ataching the id to the object. I would actually cache the DOM look up. this is the part that is really heavy. So instead of calling document.getElementById everytime. I would have the construct accept an element. \$\endgroup\$ – Pinoniq Sep 30 '14 at 14:34
  • \$\begingroup\$ Yeah, this.style.XXX is null makes sense; after all given your code you would need to look up the element in the DOM from the cached this.id. So... onCustomFocus for example should probably look like: function () { var el = document.getElementById(this.id); el.style.backgroundColor = "#000"; el.style.color = "#0ff"; };. Better yet would be to cache the look up as per Pinoniq's comment. So that instead of having this.id = id you could do this.element = document.getElementById(id). Hope this helps. \$\endgroup\$ – Ben Sep 30 '14 at 17:49
  • \$\begingroup\$ The IE 10 issue I've fixed. I'll post an update later, but basically, the logic test in the addEvent() method has to be flipped. However, what I did not realize when I posted the question is that I do not have access to the CustomEditor variables from within the event listeners. The issue is this references the input not CustomEditor. I've tried to use bind() but I've not gotten it to work. How do I give access to CustomEditor variables from the event handler methods? \$\endgroup\$ – Karl Oct 3 '14 at 10:34
  • \$\begingroup\$ You can try this.addEvent('focus', this.onCustomFocus.bind(this)); - or if using underscore/lodash this.addEvent('focus', _.bind(this.onCustomFocus, this));. developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/… \$\endgroup\$ – Ben Oct 3 '14 at 10:46

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.