2
\$\begingroup\$

This is the piece of code I'd like to refactor. Basically, when the credit card option is selected, then do I want the credit card number, zip code, and Cvv inputs to be validated.

validateForm=(e)=> {
  Array.from(inputElements).forEach(element=> {
    let value=element.value;
    let input=element;
    if (input===name) {
      errorMessage(isValidName, input, value, e)
    } else if (element===email) {
      errorMessage(isValidEmail, input, value, e);
    } else if (element===creditNumber) {
      if (creditOptionChecked()) {
        errorMessage(isValidCreditCard, input, value, e);
      }
    } else if (element===zip) {
      if (creditOptionChecked()) {
        errorMessage(isValidZipCode, input, value, e);
      }
    } else if (element===cvv) {
      if (creditOptionChecked()) {
        errorMessage(isValidCvv, input, value, e);
      }
    } else if (element==fieldSetActivity) {
      validateCheckboxes(e)
    }
  });
}


form.addEventListener('submit', (e)=> {
  validateForm(e)
  console.log("hello")
})

I've been at this for a couple hours, but I'm still a bit new to be able to implement something that is modular. Here is a link to my JS fiddle, so you can see the rest of the javascript code: https://jsfiddle.net/apasric4/4xfL9utv/2/

\$\endgroup\$
0

1 Answer 1

1
\$\begingroup\$

Incomplete code

Your question's code is missing so much information there is not much to review.

Because of this I can only review the code in regard to exactly the logic your function performs.

Style points

  • Don't add code that is not needed. Eg

    • The variable input is a copy of element having two names for the same reference will lead to confusion, mistakes and bugs.

    • You add a function to form submit, and all it does is call validateForm. Set the listener to validateForm eg form.addEventListener('submit', validateForm) and avoid the middle man.

  • Good code does not repeat AKA is DRY (Don't Repeat Yourself).

    Examples of repeated code;

    • You call errorMessage, with the same 3 last arguments five times. Create a function and just pass the first argument, let the function call errorMessage with the rest of the arguments.

    • You repeat the same statement if (creditOptionChecked()) { three times. Again hand the repetitive source code to a function..

  • Variables that do not change and/or should not change should be declared as constants. eg const value = element.value; rather than let value = element.value;

  • Array.from requires an array like argument. Array like arguments are all iterable thus you do not need to create an array before you iterate. The line Array.from(inputElements).forEach(element=> { can be written as inputElements.forEach(element => { or for (const element of inputElements) {

    Array iterators (example Array.forEach) are not as efficient as for or while loops. Using for or while loops when you can also gives you a better sense of the code complexity, while using a little less memory and power.

Example

Rewriting your function to be compatible with the original under all possible unknown states using the above points.

form.addEventListener("submit", e => {
    var input;
    const error = valid => errorMessage(valid, input, input.value, e);
    const optError = valid => creditOptionChecked() && error(valid);
    for (input of inputElements) {
        if (input === name) { error(isValidName) }
        else if (input === email) { error(isValidEmail) }
        else if (input === creditNumber) { optError(isValidCreditCard) }
        else if (input === zip) { optError(isValidZipCode) }
        else if (input === cvv) { optError(isValidCvv) }
        else if (input == fieldSetActivity) { validateCheckboxes(e) }
    }
});

There are some improvements that can be made depending on the unknowns, maybe needing no if statements at all, but I likely waste both of our times if I speculate.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.