JavaScript form validation: improve on 'good, but not good enough'

I recently received a 'good, but not good enough' rating on a coding exercise I was asked to perform. It was to validate that if "Product complaint" option was selected that Product name, Product size, Use-by date and Batch code fields all needed to have values. What can I do to improve the JS to go from 'good, but not good enough' to 'great'?

JS:

document.querySelector("form").addEventListener("submit", function(e) {

  var enquiryType = document.querySelector("#enquirytype");

  if (enquiryType.value === "pc") {

      validateProductDetails();
      e.preventDefault();

  }

  });

  function validateProductDetails() {

  var requiredFields = document.querySelectorAll('[id^="product-"]'),
      requiredLabel = document.querySelectorAll('[for^="product-"]'),
      formSubmit = document.forms[0],
      requiredIsEmpty = [],
      i;

  for (i = 0; i < requiredFields.length; i += 1) {

      if (requiredFields[i].value.length === 0) {

          alert('Please enter the ' + requiredLabel[i].innerHTML + ".");
          requiredFields[i].focus();
          requiredIsEmpty.push(requiredFields[i]);
          return false;

      }

  }

  if (requiredIsEmpty.length === 0) {

      formSubmit.submit();
      return true;
  }

}

HTML:

  Enquiry type
  
    Choose
    General enquiry
    Product feedback or enquiry
    Product complaint
  

  Product name
  

  Product size
  

  Use-by date
  

  Batch code
  

  

 

Separation of Concerns

So I agree with rolfl, but I also want to expand a bit, and provide some small bit of skeleton code for you to go on. When I do validation on the client, I usually set up variables for errors, conditions, and dependencies. This allows me to use a simple handling structure that can be used if needed (as @rolfl suggested), and ignored if not, that way you aren't validating fields that don't need validation, or if they do, you're only performing exactly as much validation as needed.

This simple "boilerplate" that I've just created will also allow you to inject functionality to any of your variables by adding objects to your arrays. As long as your validate function executes the conditions and dependencies injected, you should have a very nice reusable client validation boilerplate.

Then, you just attach an event listener to whatever form element you want to validate, and run a validate function on that callback. It'll efficiently go through conditions that exist on elements, instead of the other way around, and you can be as flexible you want with your view code without tying it to your validation function.

One of the key principles of coding is Separation of Concerns. Any of your code that performs app logic (the validation) shouldn't have any sort of view code in it if at all possible. I believe this is the biggest single concept that would have made the difference in the feedback you received . If you maintain SOC in this case, no matter how the form changes, your validate function won't have to change, or will be at least very flexible, and if you all of a sudden needed to use the same form but on a mobile page, you could just change the renderErrors function to alter your view code to fit the new display without changing anything else.

That's what I would consider great. See my fiddle: http://jsfiddle.net/LongLiveCHIEF/sqwYT/

I'll update it a bit, but wanted to give you a head start on my line of thought.

StackExchange Code Review Q#36102, answer score: 2