Prevent rewriting multiple vars

Question

Taking a look at the code, is there a better way in JavaScript to prevent so much repetition. In particular, re declaring the variables like moduleOptions, removeBtn, moveUp?

I tried putting them in their own key values pairs. Unfortunately, that causes them to be called right away, and the selectors that I'm querying are not ready until the toggleModuleOptions.show() and toggleModuleOptions.hide() functions are called.

 var toggleModuleOptions = {
 show: function() {
 var moduleOptions = visualModules.activeModuleContainer.querySelector('.module-options');
 moduleOptions.classList.add('module-options--active');
 removeBtn = visualModules.activeModuleContainer.querySelector('.js-remove');
 removeBtn.addEventListener('click', deleteModule);
 moveUp = visualModules.activeModuleContainer.querySelector('.js-index-up');
 moveUp.addEventListener('click', move);
 moveUp.direction = 'up';
 moveDown = visualModules.activeModuleContainer.querySelector('.js-index-down');
 moveDown.addEventListener('click', move);
 moveDown.direction = 'down';
 },
 hide: function() {
 var moduleOptions = visualModules.activeModuleContainer.querySelector('.module-options');
 if(moduleOptions) {
 moduleOptions.classList.remove('module-options--active');
 }
 removeBtn = visualModules.activeModuleContainer.querySelector('.js-remove');
 removeBtn.removeEventListener('click', deleteModule);
 moveUp = visualModules.activeModuleContainer.querySelector('.js-index-up');
 moveUp.removeEventListener('click', move);
 moveDown = visualModules.activeModuleContainer.querySelector('.js-index-down');
 moveDown.removeEventListener('click', move);
 }
 }

Answer 1

This version tries not to change functionality. Added two functions that reduce code duplication, both methods make use of javascript's [](square bracket) property accessor syntax which allows the functions to call different functions based on the parameter passed in.

var addListener = function(selector, method, listener) {
 var selected = visualModules.activeModuleContainer.querySelector('.js-' + selector);
 selected[method + 'EventListener']('click', listener);
 return selected;
};
var showOrHide = function (method, direction) {
 var moduleOptions = visualModules.activeModuleContainer.querySelector('.module-options');
 if (moduleOptions) {
 moduleOptions.classList[method]('module-options--active');
 }
 removeBtn = addListener('remove', method, deleteModule);
 moveUp = addListener('index-up', method, move);
 moveDown = addListener('index-down', method, move);
 if (direction) {
 moveUp.direction = 'up';
 moveDown.direction = 'down';
 }
};
var toggleModuleOptions = {
 show: function() {
 showOrHide('add', true);
 },
 hide: function() {
 showOrHide('remove', false);
 }
};

This version could change some things but very minor. This one always sets the direction properties even on the one that wasn't being set before to avoid needing conditional logic for setting those properties. It also does not assign values to variables from outside the scope of the code shown which was only left in above in case it was necessary.

var addListener = function(selector, method, listener, direction) {
 var selected = visualModules.activeModuleContainer.querySelector('.js-' + selector);
 selected[method + 'EventListener']('click', listener);
 selected.direction = direction;
};
var showOrHide = function (method) {
 var moduleOptions = visualModules.activeModuleContainer.querySelector('.module-options');
 if (moduleOptions) {
 moduleOptions.classList[method]('module-options--active');
 }
 addListener('remove', method, deleteModule);
 addListener('index-up', method, move, 'up');
 addListener('index-down', method, move, 'down');
};
var toggleModuleOptions = {
 show: function() {
 showOrHide('add');
 },
 hide: function() {
 showOrHide('remove');
 }
};

Answer 2

You might be able to do something like this:

var toggleModuleOptions = (function () {
 var moduleOptions, removeBtn, moveUp, moveDown;
 function getElements() {
 moduleOptions = visualModules.activeModuleContainer.querySelector('.module-options');
 removeBtn = visualModules.activeModuleContainer.querySelector('.js-remove');
 moveUp = visualModules.activeModuleContainer.querySelector('.js-index-up');
 moveDown = visualModules.activeModuleContainer.querySelector('.js-index-down');
 }
 return {
 show: function () {
 getElements();
 moduleOptions.classList.add('module-options--active');
 removeBtn.addEventListener('click', deleteModule);
 moveUp.addEventListener('click', move);
 moveUp.direction = 'up';
 moveDown.addEventListener('click', move);
 moveDown.direction = 'down';
 },
 hide: function () {
 getElements();
 if (moduleOptions) {
 moduleOptions.classList.remove('module-options--active');
 }
 removeBtn.removeEventListener('click', deleteModule);
 moveUp.removeEventListener('click', move);
 moveDown.removeEventListener('click', move);
 }
 }
}());

The API is still toggleModuleOptions.show() and toggleModuleOptions.hide() but the common task of finding the elements is handled by a "private" function.

Ideally, the getElements function wouldn't be necessary (or at least shouldn't have side-effects), and you could just do:

var toggleModuleOptions = (function () {
 var moduleOptions = visualModules.activeModuleContainer.querySelector('.module-options'),
 removeBtn = visualModules.activeModuleContainer.querySelector('.js-remove'),
 moveUp = visualModules.activeModuleContainer.querySelector('.js-index-up'),
 moveDown = visualModules.activeModuleContainer.querySelector('.js-index-down');
 return { ... }

i.e. find the elements once, and skip the getElements function. However, this won't necessarily work if the JS is evaluated before the page has loaded or the page changes so much that the elements do have to found and re-found, to avoid stale references.

Alternatively, you could perhaps handle more of this in the move/deleteModule event handlers themselves. E.g. if the container element has or lacks a certain class name, just ignore the event. That way you won't have to add and remove the handlers all the time.

Or, of course, if the elements are actually hidden, you won't receive events on them anyway.

Our of curiosity: In the hide function you check whether moduleOptions exists before trying to remove a class name, but in show you don't check. I'd imagine that the two functions should be more symmetrical: If you need a check in one of them, you probably need it in both. Or, conversely, you don't need the check, period.