2
\$\begingroup\$

I have a myDiv.mousedown, window.mousemove, and window.mouseup event. I'm trying to organize my code, and have it clean. So I created a function and returned the 3 event handlers.
The mousedown event adds a mousemove event, and the mouseup event removes the mousemove event.

Since I need to pass the exact same object to removeEventListener() as the one I used with addEventListener(), I have to create a variable for each event handler.

This doesn't seem to be organized and clean. Does this follow common practice? If not what is the 'correct', organized and clean code to perform that?

JSFiddle

var myDiv = document.getElementById('myDiv');

var VeryUsefullClass = (function() {

  function VeryUsefullClass(parentElem, options) {
    var _this = this;
    _this.index = 0;

    _this.mouseDownHandler = _this.mouseHandler().mouseDown;
    _this.mouseMoveHandler = _this.mouseHandler().mouseMove;
    _this.mouseUpHandler = _this.mouseHandler().mouseUp;

    myDiv.addEventListener('mousedown', _this.mouseDownHandler);
    window.addEventListener('mouseup', _this.mouseUpHandler);
  }

  VeryUsefullClass.prototype.mouseHandler = function() {
    var _this = this;
    var obj = {
      mouseDown: function(e) {
        console.log('mouseDown');
        window.addEventListener('mousemove', _this.mouseMoveHandler);
      },
      mouseMove: function(e) {
        myDiv.innerHTML = 'mouseMove ' + _this.index++;
      },
      mouseUp: function(e) {
        console.log('mouseUp');
        _this.index = 0;
        window.removeEventListener('mousemove', _this.mouseMoveHandler);
      }
    }
    return obj;
  }

  return VeryUsefullClass;
})();

console.clear();

var hello = new VeryUsefullClass();
#myDiv {
  position: absolute;
  background-color: orange;
  width: 200px;
  height: 100px;
}
<div id="myDiv"></div>

\$\endgroup\$
  • \$\begingroup\$ No class? Not up on the es6 yet? \$\endgroup\$ – Naftali aka Neal Sep 23 '16 at 15:12
  • \$\begingroup\$ Also you do not have any dependency injection. myDiv is global? \$\endgroup\$ – Naftali aka Neal Sep 23 '16 at 15:14
  • \$\begingroup\$ @Neal No, I'm not using es6 yet. In the code above myDiv is global, but not in my actual project. \$\endgroup\$ – Steve Sep 23 '16 at 15:24
  • \$\begingroup\$ See my answer using dependency injection (with es6) It can be modified (easily) to use es5 if you want. \$\endgroup\$ – Naftali aka Neal Sep 23 '16 at 15:35
  • \$\begingroup\$ In the code above myDiv is global, but not in my actual project - this is why we ask for actual, real code. There's little use in putting up your code for review if the advice you're getting isn't about your actual/real code. Not to mention the effort reviewers typically put in their answers. \$\endgroup\$ – Mathieu Guindon Sep 23 '16 at 16:39
3
\$\begingroup\$
  • Consider using dependency injection

    • What that means is "injecting" what you want to manipulate into your class (instead of relying on "global" variable), for example:

      let a = 42;
      function manipulateA () {
          return a = a + 24;
      }
      console.log(manipulateA(), a); // 66, 66
      

      versus:

      let a = 42;
      function manipulate (a) {
          return a = a + 24;
      }
      console.log(manipulate(a), a); // 66, 42
      

      This allows for much easier to test code, and you know exactly what will happen with everything instead of relying on something to magically happen in the global scope.

Consider this instead (using es6 and dependency injection):

class MoveOver {
  // dependency injection
  constructor ({element = document.body}) {
    this.element = element;
    this.index = 0;
  }
  
  startListening () {
    const mouseMove = () => {
      this.element.textContent = `mouseMove_${this.index++}`;
    }
    this.element.addEventListener('mousedown', () => {
      console.log('mouseDown');
      this.element.addEventListener('mousemove', mouseMove, false);
    }, false);
    window.addEventListener('mouseup', () => {
      console.log('mouseUp');
      this.element.removeEventListener('mousemove', mouseMove, false);
    }, false);
  }
}

const move1 = new MoveOver({element: document.getElementById('mouse')});
const move2 = new MoveOver({element: document.getElementById('mouse2')});

move1.startListening();
move2.startListening();
.cover {
  height: 200px; width: 200px;
  border: red solid 1px;
  float: left; margin: 15px;
}
<div id="mouse" class="cover"></div>
<div id="mouse2" class="cover"></div>

In the above example we are able to add the same events to two (2) different elements, but yet still yield the same results for both

\$\endgroup\$
  • \$\begingroup\$ Thanks for the detailed and explained answer! Isn't it more efficient to call a named function to an event listener, or does it not make a difference? \$\endgroup\$ – Steve Sep 23 '16 at 16:11
  • \$\begingroup\$ What do you mean @Jessica ? \$\endgroup\$ – Naftali aka Neal Sep 23 '16 at 16:17
  • \$\begingroup\$ Meaning something like this: element.addEventListener('click', namedFunction); function namedFunction(e) {...} \$\endgroup\$ – Steve Sep 23 '16 at 16:19
  • \$\begingroup\$ Sorry, can you make a fiddle or something? it is hard to convey meaning in code in a comment. \$\endgroup\$ – Naftali aka Neal Sep 23 '16 at 16:19
  • \$\begingroup\$ jsfiddle.net/2syuevz3 \$\endgroup\$ – Steve Sep 23 '16 at 16:24

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.