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\$
6
  • \$\begingroup\$ No class? Not up on the es6 yet? \$\endgroup\$
    – Naftali
    Commented Sep 23, 2016 at 15:12
  • \$\begingroup\$ Also you do not have any dependency injection. myDiv is global? \$\endgroup\$
    – Naftali
    Commented Sep 23, 2016 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
    Commented Sep 23, 2016 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
    Commented Sep 23, 2016 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\$ Commented Sep 23, 2016 at 16:39

1 Answer 1

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\$
8
  • \$\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
    Commented Sep 23, 2016 at 16:11
  • \$\begingroup\$ What do you mean @Jessica ? \$\endgroup\$
    – Naftali
    Commented Sep 23, 2016 at 16:17
  • \$\begingroup\$ Meaning something like this: element.addEventListener('click', namedFunction); function namedFunction(e) {...} \$\endgroup\$
    – Steve
    Commented Sep 23, 2016 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
    Commented Sep 23, 2016 at 16:19
  • \$\begingroup\$ jsfiddle.net/2syuevz3 \$\endgroup\$
    – Steve
    Commented Sep 23, 2016 at 16:24

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.