Trying to improve my javascript code

JavaScript
1

I have the following function where I am separately checking for if(document.getElementById("affiliatedPrimary5") !== null){ in the first if and then going after looping over checkboxes.

Is there a way I could improve my checking process since I’m displaying same alert message three times in three scenarios ?

function affiliationcarKitCodechecks() {
	    
	 if(document.getElementById("affiliatedPrimary5") !== null){
	     	let isPrimcarKitCodeSelected = document.getElementById("affiliatedPrimary5").checked;
		   	 if (isPrimcarKitCodeSelected) {
		        //check if partOne is empty or not
		        let partOne = document.getElementById("partOne").value;
		        let partTwo = document.getElementById("partTwo").value;
		        partOne=partOne.trim();
		        partTwo=partTwo.trim();
		        if (partOne.length==0 && partTwo.length==0) {
		            alert("Please complete the partOne or partTwo fields. This information is required ");
		            return false;
		        }
		    }


		    
	    }
	    
	   var checkboxes =  document.getElementsByName('affiliatedSecondary');
      
	    for (var i = 0; i < checkboxes.length; i++) {
	        if (checkboxes[i].checked) {
	         
	             if(checkboxes[i].value === "CED"){
	            	 	 let partOne = document.getElementById("partOne").value;
	 		         let partTwo = document.getElementById("partTwo").value;
	 		         partOne=partOne.trim();
	 		         partTwo=partTwo.trim();
	 		         if (partOne.length==0 && partTwo.length==0) {
	 		            alert("Please complete the partOne or AltId/Tattoo fields. This information is required ");
	 		            return false;
	 		         }
	            	
	             }
	        }
	    }
	
	    
	    let doescarKitCodeExists = document.getElementById("carKitCode");
	     //check for empty string
	    if(doescarKitCodeExists.value !== ""){
	    	//check if the string equals carKitCode
	    	if(doescarKitCodeExists.value === "carKitCode") {
	    	    let partOne = document.getElementById("partOne").value;
		        let partTwo = document.getElementById("partTwo").value;
		        partOne=partOne.trim();
		        partTwo=partTwo.trim();
		        if (partOne.length==0 && partTwo.length==0) {
		            alert("Please complete the partOne or partTwo fields. This information is required ");
		            return false;
		        }
	    	}
	    }
	    


	    
	}
2

I think you are alluding to this, but for starters you could address some of the repetition. In particular this block of code which you have used 3 times.

  let partOne = document.getElementById("partOne").value;
  let partTwo = document.getElementById("partTwo").value;
  partOne=partOne.trim();
  partTwo=partTwo.trim();
  if (partOne.length==0 && partTwo.length==0) {
      alert("Please complete the partOne or partTwo fields. This information is required ");
      return false;
  }

A simple way to do this is to wrap the code inside of a function. This will make it re-usable.

function checkEmptyFields () {
  let partOne = document.getElementById("partOne").value;
  let partTwo = document.getElementById("partTwo").value;
  
  partOne=partOne.trim();
  partTwo=partTwo.trim();
  
  if (partOne.length==0 && partTwo.length==0) {
    alert("Please complete the partOne or partTwo fields. This information is required ");
    return false;
  }
}

So your original block of code

if(doescarKitCodeExists.value !== ""){
  //check if the string equals carKitCode
  if(doescarKitCodeExists.value === "carKitCode") {
      let partOne = document.getElementById("partOne").value;
      let partTwo = document.getElementById("partTwo").value;
      partOne=partOne.trim();
      partTwo=partTwo.trim();
      if (partOne.length==0 && partTwo.length==0) {
          alert("Please complete the partOne or partTwo fields. This information is required ");
          return false;
      }
  }
}

can be refactored to

if(doescarKitCodeExists.value !== ""){
  //check if the string equals carKitCode
  if(doescarKitCodeExists.value === "carKitCode") {
    checkEmptyFields();
  }
}

This also raises the question to me (I am still half asleep), do we need to check if doescarKitCodeExists is empty? Is that redundant?

Won’t just checking for a match to carKitCode work?

if(doescarKitCodeExists.value === "carKitCode") {
  checkEmptyFields();
}

Note: if you are using camelCase, doescarKitCodeExists should be doesCarKitCodeExist. These inconsistencies can be a cause of frustrating bugs. Maybe carKitCode would suffice as a name.

Refactoring the partOne, partTwo field checks into a re-usable function would certainly be a start.

To improve on the checkEmptyFields function, I would want to make it more generic. A function that possibly takes an arbitrary number of inputs, checks if they are all empty and returns true or false. This way it is not so tightly coupled to ‘partOne’ and ‘partTwo’ and a set error message.

// can select multiple elements separated by a comma with querySelectorAll
const fields = document.querySelectorAll('#partOne, #partTwo');
// slight rename to the checkEmptyFields function
if (allEmptyFields(fields)) {
  alert("Please complete the partOne or partTwo fields. This information is required ");
  return false;
}

This would go back to some repetition, but would offer you more flexibility.

The above is just a start. It would be helpful to see your form’s HTML as well.

2 Likes
3

Thank you for the detailed explanation.

For some reason, putting it in a separate function is not stopping the form from submitting so I had to keep it as it is. Is there anything I need to be careful of or might be causing this unexpected behavior? Basically, the whole this is inside a form on a JSP page and the form keeps submitting if I put the repetitative code inside a function.

4

I confess I have not worked with JSP, but logically having the code block wrapped in a function shouldn’t make any difference to the outcome. I will have another look through your existing code, to see if there is anything I have missed.

I think we could still benefit from seeing the HTML for your form, so we can see how it all ties together. At least that way, we can maybe run some tests.

5

In this refactored version, I was wondering if I should be checking for true or false since checkEmptyFields() function would return false in case of failure. So should the above code be like this? Because what I’m noticing is that my form keeps on submitting even if the checkEmptyFields() function returns false.

if(doescarKitCodeExists.value !== ""){
  //check if the string equals carKitCode
  if(doescarKitCodeExists.value === "carKitCode") {
           if(!checkEmptyFields()){
              return false;
           } else {
           return true ;
            }
  }
}
6

My html form is defined like this:


		<form id="orderForm" action="/mywebsite/car.htm" method="POST" onsubmit="return validateOrderForm(this) && affiliationCEDchecks()" data-gtm-form-interact-id="0">
            </form>

Here is my javascript code which works fine.By fine, I mean, if an alert is shown, my form won’t submit.

function affiliationcarKitCodechecks() {
	    
	 if(document.getElementsByName("affiliatedPrimary") !== null){
	     	let checkboxes = document.getElementsByName("affiliatedPrimary");
			for (var i = 0; i < checkboxes.length; i++) {
	        if (checkboxes[i].checked) {
	         
	             if(checkboxes[i].value === "CED"){
	            	 	 let partOne = document.getElementById("partOne").value;
	 		         let partTwo = document.getElementById("partTwo").value;
	 		         partOne=partOne.trim();
	 		         partTwo=partTwo.trim();
	 		         if (partOne.length==0 && partTwo.length==0) {
	 		            alert("Please complete the partOne or AltId/Tattoo fields. This information is required ");
	 		            return false;
	 		         }
	            	
	             }
	        }
	    }


		    
	    }
	    
	   var checkboxes =  document.getElementsByName('affiliatedSecondary');
      
	    for (var i = 0; i < checkboxes.length; i++) {
	        if (checkboxes[i].checked) {
	         
	             if(checkboxes[i].value === "CED"){
	            	 	 let partOne = document.getElementById("partOne").value;
	 		         let partTwo = document.getElementById("partTwo").value;
	 		         partOne=partOne.trim();
	 		         partTwo=partTwo.trim();
	 		         if (partOne.length==0 && partTwo.length==0) {
	 		            alert("Please complete the partOne or AltId/Tattoo fields. This information is required ");
	 		            return false;
	 		         }
	            	
	             }
	        }
	    }
	
	    
	    let doescarKitCodeExists = document.getElementById("carKitCode");
	     //check for empty string
	    if(doescarKitCodeExists.value !== ""){
	    	//check if the string equals carKitCode
	    	if(doescarKitCodeExists.value === "carKitCode") {
	    	    let partOne = document.getElementById("partOne").value;
		        let partTwo = document.getElementById("partTwo").value;
		        partOne=partOne.trim();
		        partTwo=partTwo.trim();
		        if (partOne.length==0 && partTwo.length==0) {
		            alert("Please complete the partOne or partTwo fields. This information is required ");
		            return false;
		        }
	    	}
	    }
	    


	    
	}

I have refactored it like this (based on the suggestion from my earlier question here) by separating out the repetitive code into a separate function:
Now, the problem is, when an alert is shown, my form would still submit which I don’t want. My understanding it that checkEmptyFields is returning a false when an alert is shown and I’m not returning it inside my if blocks below? So do I need to return something like return checkEmptyFields(); wherever I’m calling this function?

function checkEmptyFields () {
  let partOne = document.getElementById("partOne").value;
  let partTwo = document.getElementById("partTwo").value;
  
  partOne=partOne.trim();
  partTwo=partTwo.trim();
  
  if (partOne.length==0 && partTwo.length==0) {
    alert("Please complete the partOne or partTwo fields. This information is required ");
    return false;
  }
}


if(document.getElementsByName("affiliatedPrimary") !== null){
	     	let checkboxes = document.getElementsByName("affiliatedPrimary");
			for (var i = 0; i < checkboxes.length; i++) {
	        if (checkboxes[i].checked) {
	         
	             if(checkboxes[i].value === "CED"){
	            	 checkEmptyFields();
	            	
	             }
	        }
	    }


		    
	    }
	    
	   var checkboxes =  document.getElementsByName('affiliatedSecondary');
      
	    for (var i = 0; i < checkboxes.length; i++) {
	        if (checkboxes[i].checked) {
	         
	             if(checkboxes[i].value === "CED"){
	            	 checkEmptyFields();
	            	
	             }
	        }
	    }
	
	    
	    let doescarKitCodeExists = document.getElementById("carKitCode");
	     //check for empty string
	    if(doescarKitCodeExists.value !== ""){
	    	//check if the string equals carKitCode
	    	if(doescarKitCodeExists.value === "carKitCode") {
	    	   checkEmptyFields();
	    	}
	    }
	    


	    
	}
7

Hi @Jack_Tauson_Sr, please don’t start anymore new threads for this. It is only confusing matters.

I have merged two of them.

It seems to me looking at the two threads there is a lot of refactoring to do. Possibly quite a bit of work.

Back to what you have posted, this does not seem to tie in with the javascript you have supplied here at all.

onsubmit="return validateOrderForm(this) && affiliationCEDchecks()"

It actually seems to relate to your other post. For instance, where is validateOrderForm?

I asked you to supply your html, but you have only supplied the form tags with none of the form elements e.g. inputs, checkboxes etc. So again we are working blind. We really need to see the form in it’s entirety.

That said, I think what you are looking for is event.preventDefault(). You would call this at the top of your validation function. If everything passes you would then call submit on your form.

This will give you a bit of an example of the usage.

I am not about today, so will have to come back to this.