3
\$\begingroup\$

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:

<form action="/" method="get">

  <label for="enquirytype">Enquiry type</label>
  <select name="enquiry" id="enquirytype" required>
    <option value="">Choose</option>
    <option value="ge">General enquiry</option>
    <option value="pf">Product feedback or enquiry</option>
    <option value="pc">Product complaint</option>
  </select>

  <label for="product-name">Product name</label>
  <input type="text" id="product-name">

  <label for="product-size">Product size</label>
  <input type="text" id="product-size">

  <label for="product-use-by">Use-by date</label>
  <input type="text" id="product-use-by">

  <label for="product-batch">Batch code</label>
  <input type="text" id="product-batch">

  <input type="submit" value="Submit" id="form-submit">

 </form>
\$\endgroup\$
  • \$\begingroup\$ were you given the html as-is, or were you asked to create those fields as well? \$\endgroup\$ – Brian Vanderbusch Nov 26 '13 at 3:42
  • \$\begingroup\$ @Brian asked to create that as well. Also instructed not to use jQuery or another library, and it only had to work in latest Chrome. The HTML was marked as 'perfect' (this is a subset of all HTML), so I'd prefer to concentrate on the JS. \$\endgroup\$ – K Groll Nov 26 '13 at 3:49
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ That boilerplate looks great. I still have to read through more carefully but I see what you're getting at. \$\endgroup\$ – K Groll Nov 26 '13 at 4:27
  • \$\begingroup\$ I updated it to make it more 'modular`, but keep in mind I didn't run any of it through a validator. I'm used to using other libs for this stuff, so my javascript DOM api usage is a bit rusty. When in doubth, it's the theory that counts. \$\endgroup\$ – Brian Vanderbusch Nov 26 '13 at 5:12
  • \$\begingroup\$ ok, I'm done editing everything now, time for bed. Good luck, reach out to me for questions. (info in my profile) \$\endgroup\$ – Brian Vanderbusch Nov 26 '13 at 5:23
  • \$\begingroup\$ Cheers for the fiddle. I'll try filling it all out and if I have any issues will get in touch. \$\endgroup\$ – K Groll Nov 26 '13 at 5:33
1
\$\begingroup\$

Searching for all the labels is a waste of time/resources. You only ever need a label if the validation fails on an entry, and only then should you locate the matching label for the failed validation.

That way, when all the fields validate, then no labels need to be searched at all.

Additionally, I would (personal preference) prefer to have a strict name-based set of values that need validation. Searching for those fields that match a pattern in the id is something that may lead to issues further down the road.

Still, if you want to use patterns for finding the entry fields, at least limit the search to values which are entry fields.... you have no validation that checks to ensure that there's not some other node that has an 'id' starting with 'product-'.

Also, you assume that the labels for the entry fields are in the same order as the fields. This is dangerous.

When it comes down to it, if it were me reviewing the code, I would think: Sure, it does the standard stuff OK, but it does not have any robustness.


EDIT

When I wrote my review I assumed the HTML was supplied (I did not realize that you wrote the HTML as part of the process). This makes some of what I suggested a little less relevant... Still, you ask about the third paragraph: ...have a strict name-based set of values that need validation...

Consider the following:

 var tovalidate=["product-name","product-size","product-use-by","product-batch"];

Then loop on that array and pull the relevant details for each of them as needed.

\$\endgroup\$
  • \$\begingroup\$ OK that's mostly fair enough regarding the labels. I only bothered with them to provide a reasonably meaningful alert message. Thanks. Can you explain your 3rd paragraph please? I don't quite get it. Also anything regarding structure of the code? Cheers. \$\endgroup\$ – K Groll Nov 26 '13 at 4:03
  • \$\begingroup\$ That explains things. I thought by giving the to be validated fields the same prefix it would provide flexibility, but I can see why it might be brittle. \$\endgroup\$ – K Groll Nov 26 '13 at 4:24

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.