4
\$\begingroup\$

Why reinvent the wheel?

I personally like to implement all of my JS from scratch, I try to stay away from using the likes of jQuery or any external sources/libraries/frameworks as much as possible. Why? you may ask, the answer is simple, I want to learn how to do as much as possible with plain/vanilla JavaScript.

I understand that for real world implementations, it's probably best to use frameworks or libraries when you can, just to save development time, I'm more than aware of that, as they say.


Question

I understand that AngularJS implements two way data binding very well, however, I'd like to know how Angular implements two way data binding?

Is there a more efficient way to implement two way data binding?


Research

So far the best way of implementing data binding is doing it like what I've written below in the code example, however, I can't help but feel that there's a more efficient way of implementing such a feature, I find that there's possibly a more sophisticated way to implement the callback function that I've written below.


Code

I'm not sure what you all think of the code I've just written below, but it's simple enough, maybe not the best solution in the world, but that's fine because I'm trying to find a better solution is possible.

/**
 * @description the purpose of this object is to encapsulate event handling
 */
function EventHandlerObject() {
    // basic singleton implementation
    if (EventHandlerObject.instance !== null  && typeof EventHandlerObject.instance !== "undefined") {
  	    return EventHandlerObject.instance;
    } else {
        EventHandlerObject.instance = this;
    }
}


/**
 * @description the purpose of this method is to add an event handler to a given dom object
 * @param       {Element||Array[Element]}  obj
 * @param       {String}   eventType
 * @param       {Function} callback
 * @param       {Boolean}  bool
 */
EventHandlerObject.prototype.addEvent = function (obj, eventType, callback, override) {
    if (typeof obj === "undefined" || obj === null) { return; }
    if (typeof callback !== "function") { return; }

    // a collection/aray/list of elements has been passed in
    if (obj.length || obj instanceof Array) {
        for (var i = 0, s = obj.length; i < s; i++) {
            var elm = obj[i];
            if (override === true) { // override all other events
                this.remEvent(obj, eventType, callback);
            }

            try { // try to add the event and callback
                if (elm.addEventListener) {
                    elm.addEventListener(eventType, callback, false);
                } else if (elm.attachEvent) {
                    elm.attachEvent('on' + eventType, callback);
                } else { elm["on"+eventType] = callback; }
            } catch (e) { console.log(e); }
        }


    // a single element has been passed in
    } else {
        if (override === true) { // override all other events
            this.remEvent(obj, eventType, callback);
        }

        try { // try to add the event and callback
            if (obj.addEventListener) {
                obj.addEventListener(eventType, callback, false);
            } else if (obj.attachEvent) {
                obj.attachEvent('on' + eventType, callback);
            } else {
                obj["on"+eventType] = callback;
            }
        } catch (e) {
            console.log(e);
        }
    }
};


/**
 * @description the purpose of this function is to remove an event handler from a given element
 * @param       {Element||Array[Element]}  obj
 * @param       {String}   eventType
 * @param       {Function} callback
 */
EventHandlerObject.prototype.remEvent = function (obj, eventType, callback) {
    if (typeof obj === "undefined" || obj === null) { return; }
    if (typeof callback !== "function") { return; }

    // a collection/aray/list of elements has been passed in
    if (obj.length || obj instanceof Array) {
        for (var i = 0, s = obj.length; i < s; i++) {
            var elm = obj[i];
            try { // try to remove the event
                if (elm.removeEventListener) {
                    elm.removeEventListener(eventType, callback, false);
                } else { elm[eventType + callback] = null; }
            } catch (e) { console.log(e); }
        }


    // a single element has been passed in
    } else {
        try { // try to remove the event
            if (obj.removeEventListener) {
                obj.removeEventListener(eventType, callback, false);
            } else { obj[eventType + callback] = null; }
        } catch (e) { console.log(e); }
    }
};


/**
 * @description the purpose of this function is to call the two above functions
 *              when necessary
 * @param       {Element||Array[Element]}  obj
 * @param       {Array[String]}   eventType
 * @param       {Function} callback
 * @param       {Boolean}  bool
 */
EventHandlerObject.prototype.multiEvent = function (obj, eventType, callback, override) {
    if (typeof obj === "undefined" || obj === null) { return; }
    if (typeof callback !== "function") { return; }
    if (!eventType.length) { return; }

    if (override === true) { // override all other events
        for (var i = eventType.length - 1; i > - 1; i --) {
            this.remEvent(obj, eventType[i], callback);
        }
    }

    // add multiple events
    for (var i = eventType.length - 1; i > - 1; i--) {
        this.addEvent(obj, eventType[i], callback);
    }
};


// just a demo of the above code 
var inp = document.getElementById("demoInput");
var out = document.getElementById("demoOutput");
var events = new EventHandlerObject();

// just do both as a demo
events.multiEvent(inp, ["keyup", "keydown", "keypress"], function() { out.textContent = inp.value; });
<input type="text" id="demoInput" />
<div id="demoOutput"></div>

\$\endgroup\$
  • \$\begingroup\$ Are you claiming that this code is the "cleanest way..."? If not, I suggest dropping that part of your title ("State what your code does in your title, not your main concerns about it"). \$\endgroup\$ – Toby Speight Feb 19 '18 at 15:20
  • \$\begingroup\$ @TobySpeight Oh heavens no, I originally posted this on StackOverflow, but I was told to clone the post and put it here. I just want to know if there's a cleaner way to implement it, I've even said in the post I feel there's a much better way to implement it. I literally said "not the best solution in the world", just to clarify. I'm aware this solution is not by any means the best solution around. I have edited the title though! Thanks! :) \$\endgroup\$ – JO3-W3B-D3V Feb 19 '18 at 15:24
  • 2
    \$\begingroup\$ If you were sent here from Stack Overflow, it's worth reading A guide to Code Review for Stack Overflow users. It will tell you what's expected here that's different to what you're used to. I think you'll find us a welcoming bunch, and I hope you get some useful reviews! \$\endgroup\$ – Toby Speight Feb 19 '18 at 15:30
  • 1
    \$\begingroup\$ Well, that's it, I learnt a lesson: I'll not anymore advise anybody at S.O. to post questions here. It's bloody annoying: they don't read the "how to ask" and I'm the one that get the complaints. \$\endgroup\$ – Gerardo Furtado Feb 19 '18 at 16:55
  • 1
    \$\begingroup\$ Any reason you add an event to a single element in remEvent function? \$\endgroup\$ – Blindman67 Feb 19 '18 at 17:38
5
\$\begingroup\$

A few notes:

  1. This data binding is not two way. If it was two way I should be able to update the either the view or the model and have changes propagate to the other. See this SO post. The code you have provided is really just a helper for managing events.

  2. I'm not familiar with Angular (JS or other) so can't really answer your initial question.


For the code you have provided.

  1. You really shouldn't need to support attachEvent. IE has supported addEventListener since IE 9 and all other common browsers have supported it even longer. If you must support it, you should also use detachEvent to remove events that you add with attachEvent.

  2. obj instanceof Array is safe if and only if you don't need to worry about frames. MDN recommends using Array.isArray instead.

  3. The constructor for EventHandlerObject does create a singleton. return this.instance won't work as this is not EventHandlerObject. To verify this, run new EventHandlerObject() === new EventHandlerObject().

  4. Though using strict equality everywhere is usually a good idea, it is commonly accepted (in some style guides at least) to use loose equality to compare with null in order to check if something is null or undefined.

    // Old
    EventHandlerObject.instance !== null  && typeof EventHandlerObject.instance !== "undefined"
    // New
    EventHandlerObject.instance != null
    
  5. Since arrays always have a length property, there's no need to check if something is an array if it has a length property. obj.length || obj instanceof Array should just be obj.length

  6. There is virtually no difference in speed between caching the length of the array and accessing it on each iteration of the loop (see jsperf from this SO post). It is much easier to read without caching the length, so I recommend removing this micro optimization.

    Looping backwards is also a micro optimization that makes the code harder to read. I recommend sticking with what makes most sense.

  7. It isn't clear what override is supposed to do in multiEvent. It sounds like you want to clear all other events on obj, but your current code will only remove the callback that you pass, and then re-add it, which has no effect.

  8. Instead of writing two nearly identical if statements for handling arrays, I recommend detecting if something is an array and just calling the method multiple times (or wrap your data in an array if it isn't an array).

  9. Consider using later JavaScript standards if possible. Use let instead of var, and make use of new features that make the code easier to read, like using for..of to loop over the entire array.

  10. Just checking obj.length isn't safe as obj could be an array with length 0. It is better to ensure it is a number with !isNaN(obj.length) (I would prefer !Number.isNaN(obj.length) but browser compatibility would suffer, alternatively you could use typeof obj.length === "number")

  11. addEventListener will never throw (unless the calling code passes in something they shouldn't, in which case you should throw an error anyways), so get rid of the try block.

Keeping with the same method structure, here is how I would recommend implementing this. I removed comments for brevity.

function EventHandlerObject() {
    if (EventHandlerObject.instance != null) {
  	    return EventHandlerObject.instance;
    } else {
        EventHandlerObject.instance = this;
    }
}

EventHandlerObject.prototype.addEvent = function (obj, eventType, callback) {
    if (obj == null) { return; }
    if (typeof callback !== "function") { return; }

    if (typeof obj.length === "number") {
        for (var i = 0; i < obj.length; i++) {
            this.addEvent(obj, eventType, callback, override)
        }
        return;
    }

    obj.addEventListener(eventType, callback, false);
};

EventHandlerObject.prototype.remEvent = function (obj, eventType, callback) {
    if (obj == null) { return; }
    if (typeof callback !== "function") { return; }
    
    if (typeof obj.length === "number") {
        for (var i = 0; i < obj.length; i++) {
            this.remEvent(obj, eventType, callback)
        }
    }
    
    elm.removeEventListener(eventType, callback, false);
};


EventHandlerObject.prototype.multiEvent = function (obj, eventType, callback) {
    if (typeof eventType.length !== "number") { return; }

    for (var i = 0; i < eventType.length; i++) {
        this.addEvent(obj, eventType[i], callback);
    }
};


var inp = document.getElementById("demoInput");
var out = document.getElementById("demoOutput");
var events = new EventHandlerObject();
events.multiEvent(inp, ["keyup", "keydown", "keypress"], function() { out.textContent = inp.value; });
<input type="text" id="demoInput" />
<div id="demoOutput"></div>

However this isn't how I would recommend doing things. It has a few problems.

  1. There doesn't seem to be any need to actually create the singleton. No state needs to be stored. It would be simpler to just use a plain object literal.

  2. If the passed in object does not exist, or arguments are not of the correct type, I believe this is a problem that the calling code should deal with. Thus I would drop the checks for obj == null and typeof callback === "function". It is better to be noisy about the user of a library doing something wrong than to silently swallow errors.

With this in mind, here is how I would implement an events helper with the same functionality. I have taken advantage of newer JS features here.

const toArray = something => Array.isArray(something) ? something : [something]

const EventHelper = {
  startListening(sources, events, listener) {
    for (const source of toArray(sources)) {
      for (const event of toArray(events)) {
        source.addEventListener(event, listener, false);
      }
    }
  },
  stopListening(sources, events, listener) {
    for (const source of toArray(sources)) {
      for (const event of toArray(events)) {
        source.removeEventListener(event, listener, false);
      }
    }
  }
};

const input = document.querySelector('input');
const output = document.querySelector('#out');
EventHelper.startListening(input, ['keyup', 'keydown'], function () {
  output.textContent = input.value;
})
<input>
<div id="out"/>

\$\endgroup\$
  • \$\begingroup\$ Thank you for so much useful feedback! As for the singleton thing, that was literally me being a numpty, I don't know why, I wasn't meant to type this.instance at all! :P ... That was a pure error... \$\endgroup\$ – JO3-W3B-D3V Feb 20 '18 at 20:50

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.