There are definitely improvements. Let us start with some basics.
Begin your file with
'use strict';
This avoids a class of nasty bugs that easily creep into JavaScript programs.
Do not use global variables. Parameterization is generally the best option but in this case we can avoid polluting the global namespace by simply wrapping our code in an immediately invoked function expression (IIFE).
(function () {
var elements;
var i = 0;
var isError = false;
var errorMessage;
var correctMessage = '✓';
var errorSpan;
...
}());
Continuing, there is indeed a lot of repetition as you have correctly recognized. Fortunately, JavaScript makes it easy to remove this kind of duplication while at the same time making our code more declarative. Let's start with some simple refactoring
function hide(element) {
element.classList.add('hidden');
}
function show(element) {
element.classList.remove('hidden');
}
Now let us extract the code under // check adventure dropdown choice
gets us to this so far
function checkAdventureDropdown(option) {
if (option.id !== 'adventure') {
return;
}
if (adventureForm.selectedIndex === 0) {
dropDowns.classList.add('hidden');
} else if (adventureForm.selectedIndex === 1) {
dropDowns.classList.remove('hidden');
charterDrop.classList.remove('hidden');
mountainDrop.classList.add('hidden');
trampDrop.classList.add('hidden');
photoDrop.classList.add('hidden');
} else if (adventureForm.selectedIndex === 2) {
dropDowns.classList.remove('hidden');
charterDrop.classList.add('hidden');
mountainDrop.classList.remove('hidden');
trampDrop.classList.add('hidden');
photoDrop.classList.add('hidden');
} else if (adventureForm.selectedIndex === 3) {
dropDowns.classList.remove('hidden');
charterDrop.classList.add('hidden');
mountainDrop.classList.add('hidden');
trampDrop.classList.remove('hidden');
photoDrop.classList.add('hidden');
} else if (adventureForm.selectedIndex === 4) {
dropDowns.classList.remove('hidden');
charterDrop.classList.add('hidden');
mountainDrop.classList.add('hidden');
trampDrop.classList.add('hidden');
photoDrop.classList.remove('hidden');
}
}
Note that we can factor out the common test into an early return.
There is a lot more we can do here. We could simplify with a switch statement, but I think a cleaner approach would be to use an object.
Let us convert our imperative conditional code into a mapping that declares which elements should be visible and which elements need to be hidden for each specific option.
var states = {
0: { hide: [dropDowns], show: [] },
1: {
hide: [trampDrop, mountainDrop, photoDrop],
show: [dropDowns, charterDrop]
},
2: {
show: [mountainDrop],
hide: [charterDrop, trampDrop, photoDrop]
},
3: {
show: [trampDrop],
hide: [charterDrop, mountainDrop, photoDrop]
},
4: {
show: [dropDowns, photoDrop],
hide: [charterDrop, mountainDrop, trampDrop]
}
}
Not only is this more readable, as we can see clearly which states include and exclude which components. Now checkAdventureDropdown becomes quite simple, with very few places for bugs to hide.
function checkAdventureDropdown(option, states) {
if (option.id !== 'adventure') {
return;
}
var state = states[adventureForm.selectedIndex];
state.show.forEach(show);
state.hide.forEach(hide);
}
Here is what we end up with
'use strict';
(function () {
var elements;
var i = 0;
var isError = false;
var errorMessage;
var correctMessage = '✓';
var errorSpan;
var adventureForm;
var charterDrop;
var mountainDrop;
var trampDrop;
var photoDrop;
var dropDowns;
var states = {
0: { hide: [dropDowns], show: [] },
1: {
hide: [trampDrop, mountainDrop, photoDrop],
show: [dropDowns, charterDrop]
},
2: {
show: [mountainDrop],
hide: [charterDrop, trampDrop, photoDrop]
},
3: {
show: [trampDrop],
hide: [charterDrop, mountainDrop, photoDrop]
},
4: {
show: [dropDowns, photoDrop],
hide: [charterDrop, mountainDrop, trampDrop]
}
};
if (option.id !== 'adventure') {
return;
}
function checkAdventureDropdown() {
var state = states[adventureForm.selectedIndex];
state.show.forEach(show);
state.hide.forEach(hide);
}
checkAdventureDropdown();
}());
There is definitely room for improvement, but I would argue that this is a step in the right direction.
Looking over the linked html, each possible value of adventureForm.selectedIndex corresponds to a meaningful string value. We can leverage this value, which is exposed via adventureForm.value, to further increase readability by giving out states meaningful names instead of numeric indeces.
var states = {
'': { hide: [dropDowns], show: [] },
charter: {
hide: [trampDrop, mountainDrop, photoDrop],
show: [dropDowns, charterDrop]
},
mountain: {
show: [mountainDrop],
hide: [charterDrop, trampDrop, photoDrop]
},
tramping: {
show: [trampDrop],
hide: [charterDrop, mountainDrop, photoDrop]
},
photograph: {
show: [dropDowns, photoDrop],
hide: [charterDrop, mountainDrop, trampDrop]
}
};
We can make this even cleaner by encapsulating our hide and show into a single function which understands our states and how to toggle them.
'use strict'
(function() {
if (option.id !== 'adventure') {
return;
}
var states = {
'': { hide: [dropDowns], show: [] },
charter: {
hide: [trampDrop, mountainDrop, photoDrop],
show: [dropDowns, charterDrop]
},
mountain: {
show: [mountainDrop],
hide: [charterDrop, trampDrop, photoDrop]
},
tramping: {
show: [trampDrop],
hide: [charterDrop, mountainDrop, photoDrop]
},
photograph: {
show: [dropDowns, photoDrop],
hide: [charterDrop, mountainDrop, trampDrop]
}
};
function applyState(state) {
hide(state.hide);
show(state.show);
function hide(elements) {
elements.forEach(function (element) {
element.classList.add('hidden');
});
}
function show(elements) {
elements.forEach(function (element) {
element.classList.add('hidden');
});
}
}
var state = states[adventureForm.value];
if (!state) {
throw Error('invalid index');
}
applyState(state);
}());
Clarity is increasing fast. We can leverage hoisting to improve it still further.
'use strict';
(function() {
if (option.id !== 'adventure') {
return;
}
var states = {
'': { hide: [dropDowns], show: [] },
charter: {
hide: [trampDrop, mountainDrop, photoDrop],
show: [dropDowns, charterDrop]
},
mountain: {
show: [mountainDrop],
hide: [charterDrop, trampDrop, photoDrop]
},
tramping: {
show: [trampDrop],
hide: [charterDrop, mountainDrop, photoDrop]
},
photograph: {
show: [dropDowns, photoDrop],
hide: [charterDrop, mountainDrop, trampDrop]
}
};
var state = states[adventureForm.value];
if (!state) {
throw Error('invalid index');
}
applyState(state);
function applyState(state) {
hide(state.hide);
show(state.show);
function hide(elements) {
elements.forEach(function (element) {
element.classList.add('hidden');
});
}
function show(elements) {
elements.forEach(function (element) {
element.classList.add('hidden');
});
}
}
}());
Here, we leverage the fact that JavaScript hoists function definitions to the top of their enclosing lexical scope to allow our code to read more naturally and declaratively. but we can use this technique to go further.
function getStates() {
return {
'': { hide: [dropDowns], show: [] },
charter: {
hide: [trampDrop, mountainDrop, photoDrop],
show: [dropDowns, charterDrop]
},
mountain: {
show: [mountainDrop],
hide: [charterDrop, trampDrop, photoDrop]
},
tramping: {
show: [trampDrop],
hide: [charterDrop, mountainDrop, photoDrop]
},
photograph: {
show: [dropDowns, photoDrop],
hide: [charterDrop, mountainDrop, trampDrop]
}
};
}
leaving us with the quite elegant
'use strict'
(function() {
if (option.id !== 'adventure') {
return;
}
var states = getStates();
var state = states[adventureForm.value];
if (!state) {
throw Error('invalid state');
}
applyState(state);
function getStates { ... }
function applyState(state) { ... }
}());
I would like to add that while the addition of the document.querySelector API is a big improvement, there are libraries such as jQuery that you can leverage to make your html itself more maintainable.
