5
\$\begingroup\$

I was wondering if someone could please point me in the right direction with this bit of JavaScript I have been working on.

I have made this form that currently has onblur JavaScript validation and additionally I am to make it dynamic so that when a user selects an option from a dropdown option form a div is shown with corresponding options. But at the moment that code I have written for this is looking pretty repetitive and suffering from 'code smell'.

Is there a better way to do this? I am thinking a for loop that iterates through the dropdown options but am not 100% sure how to put this in place?

adventureForm = document.querySelector('#adventure');
       charterDrop = document.querySelector('.charter-options');
       mountainDrop = document.querySelector('.mountain-lake-options');
       trampDrop = document.querySelector('.tramp-options');
       photoDrop = document.querySelector('.photo-options');
       dropDowns =  document.querySelector('#dropdowns');

      // check adventure dropdown choice
      if (field.id === "adventure" && adventureForm.selectedIndex === 0) {
      dropDowns.classList.add('hidden');

          } else if (field.id === "adventure" && 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 (field.id === "adventure" && 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 (field.id === "adventure" && 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 (field.id === "adventure" && adventureForm.selectedIndex === 4)  {
           dropDowns.classList.remove('hidden');
           charterDrop.classList.add('hidden');
           mountainDrop.classList.add('hidden');
           trampDrop.classList.add('hidden');
           photoDrop.classList.remove('hidden');
         }

You can view the JSBin link here.

\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$

Your JSBin contains more interesting code than the excerpt you have embedded in the question.

This JavaScript code appears in your onblur handler for the "Adventure Type" field. You should be using an onchange handler instead, since it is annoying that the UI update is delayed.

You shouldn't be mingling code that changes the form with the code that validates the choices. The two tasks should be done in separate handlers.

The dependent sets of selections for the various types of adventures have a lot in common with each other. For example, most of them require a Date and a Time of Day. You should reuse the same field rather than writing multiple lookalike versions of the field. Not only would that reduce the HTML, it would also improve the user experience, since changing the Adventure Type wouldn't clobber any data the user may have entered for the Date.

You're relying too much on JavaScript to manipulate specific elements in the form. You can define the field dependencies in the HTML form, use CSS to hide irrelevant fields, and use just a touch of JavaScript to set one class to trigger the desired CSS effect.

document.getElementById('adventure').addEventListener('change', function(event) {
    var adventure = event.target.value;
    var detailsContainer = document.getElementById('adventure-details');
    detailsContainer.className = detailsContainer.className.replace(/\s*for-adventure-\S*/g, '') + ' for-adventure-' + adventure;
    console.log(detailsContainer.className);
});
#adventure-details .form__group {
    display: none;
}

#adventure-details.for-adventure-charter .form__group.for-adventure-charter,
#adventure-details.for-adventure-mountain .form__group.for-adventure-mountain,
#adventure-details.for-adventure-tramping .form__group.for-adventure-tramping,
#adventure-details.for-adventure-photograph .form__group.for-adventure-photograph {
    display: block;
}
<div>
  <div class="form__group">
    <label class="adventure__title" for="adventure">Adventure Type</label>
    <select class="form__option" title="adventure" id="adventure" required>
      <option value=""></option>
      <option value="charter">Charter Flight</option>
      <option value="mountain">Mountain &amp; Lake Viewing</option>
      <option value="tramping">High Country Tramping</option>
      <option value="photograph">Photographic Flight</option>
    </select>
    <span class="correct" id="adventure-error"></span>
  </div>
</div>

<div id="adventure-details" class="container">
  <div class="form__group for-adventure-charter for-adventure-mountain for-adventure-photograph">
    <label class="form__label" for="date">Date</label>
    <input class="form__input" name="date" id="date" placeholder="22/10/2016" required>
    <span class="correct" id="date-error"></span>
  </div>

  <div class="form__group for-adventure-tramping">
    <label class="form__label" for="trampdate">Departure Date</label>
    <input class="form__input" name="trampdate" id="trampdate" placeholder="22/10/2016" required>
    <span class="correct" id="trampdate-error"></span>
  </div>

  <div class="form__group for-adventure-tramping">
    <label class="form__label" for="return">Return Date</label>
    <input class="form__input" name="return" id="return" placeholder="28/10/2016" required>
    <span class="correct" id="return-error"></span>
  </div>

  <div class="form__group for-adventure-tramping">
    <label class="form__label" for="catering">Catering Required</label>
    <select class="form__option" title="catering" id="catering" required>
      <option value=""></option>
      <option value="yes">Yes</option>
      <option value="no">No</option>
    </select>
    <span class="correct" id="catering-error"></span>
  </div>

  <div class="form__group for-adventure-charter for-adventure-mountain for-adventure-photograph">
    <label class="time__title" for="time">Time Of Day</label>
    <select class="form__option" title="time" id="time" required>
      <option value=""></option>
      <option value="morning">Morning</option>
      <option value="mid-day">Mid-Day</option>
      <option value="afternoon">Afternoon</option>
      <option value="sunset">Sunset</option>
    </select>
    <span class="correct" id="time-error"></span>
  </div>

  <div class="form__group for-adventure-charter">
    <label class="adventure__title" for="flight-type">Flight Type</label>
    <select class="form__option" title="flight-type" id="flight-type" required>
      <option value=""></option>
      <option value="acrobatic">Acrobatic</option>
      <option value="peaceful">Peaceful</option>
    </select>
    <span class="correct" id="flight-type-error"></span>
  </div>

  <div class="form__group for-adventure-charter for-adventure-mountain for-adventure-photograph">
    <label class="length__title" for="length">Flight Length</label>
    <input class="form__number" id="length" type="number" title="length" min="1" max="5" required>
    <span class="correct" id="length-error"></span>
  </div>

  <div class="form__group for-adventure-photograph">
    <label class="form__label" for="doors">Doors Removed?</label>
    <select class="form__option" title="doors" id="doors" required>
      <option value=""></option>
      <option value="yes">Yes</option>
      <option value="no">No</option>
    </select>
    <span class="correct" id="doors-error"></span>
  </div>
</div>

\$\endgroup\$

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.