0

I'm working on a project for school and I'm kinda having problems with the code.I want the code to do the following: If the user press the submit button it must give the value of the choices they made. Like if the choose question 1 letter a (with a value of 10) and question 2 letter b (with a value of 20), it will give the total of 30. I already gave the values to each answer. But when I run the code I get NaN.

Here's the HTML code:

   var numo, numt, nump, numf, numv, x;
    
    var questiontype = document.querySelectorAll('input[name=type]');
    
    questiontype.forEach((radio)=>{
      radio.addEventListener('change',(e)=>{
        var numo = e.target.value;
        
      });
    }); 
    
    var questionage = document.querySelectorAll('input[name=age]');
    
    questionage.forEach((radio)=>{
      radio.addEventListener('change',(e)=>{
        var numt = e.target.value;
        
      });
    }); 
    
    var questionanatomical = document.querySelectorAll('input[name=anatomical]');
    
    questionanatomical.forEach((radio)=>{
      radio.addEventListener('change',(e)=>{
        var nump= e.target.value;
        
      });
    }); 
    
    var questionpatient = document.querySelectorAll('input[name=patient]');
    
    questionpatient.forEach((radio)=>{
      radio.addEventListener('change',(e)=>{
        var numf = e.target.value;
        
      });
    }); 
    
    var questionaccuracy = document.querySelectorAll('input[name=accuracy]');
    
    questionaccuracy.forEach((radio)=>{
      radio.addEventListener('change',(e)=>{
        var numv = e.target.value;
       
      });
    });
    
    function getValues(numo, numt, nump, numf, numv) {
    var x = numo + numt + nump + numf + numv;
    document.getElementById("answer").innerHTML = x;
    } 
 <article>
		
			<h1 class="headings">New Mask</h1>
			
			<section>
			
			<fieldset>
			<form action="" method="POST" id="nwmask">
			
			<label>Type of Base Plate</label> </br>
			
			<input type="radio" name="type" value=10000 checked>	High Precision 	<br/>
			<input type="radio" name="type" value=20000>		Push-Pin 		<br/>
			</fieldset>
			
			<br/>
			
			
			<fieldset>
			<label>Age</label> </br>
			
			<input type="radio" name="age" value=1000 checked>			Adult 	<br/>
			<input type="radio" name="age" value=2000>		Paediatric 		<br/>
			</fieldset>
			
			<br/>
			
			<fieldset>
			<label>Anatomical Region</label> </br>
			
			<input type="radio" name="anatomical" value=100 checked>				Brain 				<br/>
			<input type="radio" name="anatomical" value=200>		Head, Neck and Shoulders	<br/>
			</fieldset>
			<br/>
			
			<fieldset>
			<label>Type of Patient</label> </br>
				
			<input type="radio" name="patient" value=10 checked>			Curative 			<br/>
			<input type="radio" name="patient" value=20>			Palliative			<br/>
			<input type="radio" name="patient" value=30>		Claustrophobic		<br/>
			</fieldset>
			<br/>
			
			
			<fieldset/>
			<label>Accuracy</label> </br>
			
			<input type="radio" name="accuracy" value=1 checked>		&lt;1mm 		<br/>
			<input type="radio" name="accuracy" value=2>		&lt;2mm		<br/>
			</fieldset>
			
			<br/>
			
			</form>
		
			<br/>
			<button type="submit" value="Submit" onclick="getValues();">				Submit	</button>
			<!--<button type="reset" value="Reset" id="resetBtn">		Reset		</button>-->
			
			</section>
			
			<p id="answer"></p>
		</article>

3
  • By doing var ... on each of your functions, you are declaring new variables whose scopes are only inside your functions, so when calling getValuesall your variables are undefined, remove all your var from your functions, and try again Commented Oct 10, 2017 at 18:22
  • @Lixus Did you mean like this? Though I still receive a Nan Commented Oct 10, 2017 at 18:34
  • @c... - I rolled back the edit since it changed the substance of the question quite a bit. Editing questions (especially your own) on SO is perfectly fine as long as there are no answers. When answers are posted, altering the question to a point where answers become irrelevant can be quite confusing for future readers and should thus be avoided ;) Commented Oct 10, 2017 at 21:19

4 Answers 4

2

Your first problem:

When you write

var nump; // <------ this line

// ...

questionanatomical.forEach((radio)=>{
      radio.addEventListener('change',(e)=>{
        var nump= e.target.value; // <-------- is "hidden" by this line

The line var nump = e.target.value is simultaneously a variable declaration and a variable assignment that could just as well be written like this:

questionanatomical.forEach((radio)=>{
  radio.addEventListener('change',(e)=>{
    var nump; // <--------- declaration
    nump = e.target.value; // <-------- assignment

The declaration part means three things:

  • You just created a brand new variable named nump in the function scope. This variable has nothing to do with the original nump variable declared on line 1 (in the global scope).
  • Since your locally scoped nump variable is named the same as the original global scope nump variable, the original nump is "hidden" from the function.
  • Therefor, the assignment on the next line is to your local nump, not the global nump variable. Nothing outside of the function scope can see the value you just assigned - meaning that you could just as well have written var noOneWillEverSeeThis = e.target.value;.

The solution to this would be losing the var, so, instead of writing

var nump = e.target.value;

you would write

nump = e.target.value;

Now, instead of declaring a brand new variable in the inner scope and then setting it to e.target.value, you set the original nump variable (in the global scope) to the desired value.

To further your understanding of this issue, I suggest googling "understanding JavaScript scopes".

Your second problem:

function getValues(numo, numt, nump, numf, numv) { 

This is a slightly different way to make the same mistake as in Problem #1: numo, numt, nump, etc. are declared as function arguments - this hides the previous declarations from the function scope.

Since your onclick callback does not provide those variables to the function, the effective call that happens is: getValues(undefined, undefined, undefined, undefined, undefined).

Now try typing this into your console: undefined + undefined. See what's happening?

I'm sure you can figure out how to solve this second problem on your own, given the solution to the first ;)

Sign up to request clarification or add additional context in comments.

4 Comments

I've tried it and changed it by removing all the var keywords inside the functions but I still get a NaN answer : (
Oh! @vzwick So I must get the onclick event provide the variables to the function. Can I remove the param words like this: function getValues() { and the put that like on top of the whole code after the var declarations?
@c... Exactly ;) You already have var declarations for numo, numt, etc. in the global scope - and you are setting those variables in the addEventListener callbacks. So getValues can do without arguments.
@c... - Here are the answers to your next 2 questions: 1) try changing all the options once before clicking the button! 2) parseInt("100", 10) === 100 ;) Don't give up learning and, most importantly, try to understand why things are happening!
1

It seems to me that you are overcomplicating your solution, why are you bothering with updating globally scoped variables on every change of the form when you could simply collect and process that data on form submit?

Scope management is a fairly large part of making JS work for you so I've wrapped my solution inside an IIFE that keeps variables scoped within itself rather than polluting the global space.

For your HTML I made a few small changes: removed the onclick in the code for submission (for simplicity of the example), moved your submit inside the form (always a best practice), and titled your form so it was easier to get to with the document.forms attribute.

    (function(d) {
      const init = () => {
        const form = d.forms.testform;
        const answer = d.getElementById('answer');
        const submitHandler = e => {
          e.preventDefault();

          const total = Object.keys(form.elements)
            .filter(key => isNaN(key))
            .reduce((acc, cur) => acc + Number(form.elements[cur].value), 0);
    
          answer.textContent = total;          
        }
    
        form.addEventListener('submit', submitHandler, false);
      };
    
      d.addEventListener('DOMContentLoaded', init, false);
    })(document)
    <article>
      <h1 class="headings">New Mask</h1>
    
      <section>
    
        <form action="" method="POST" id="nwmask" name="testform">
          <fieldset>
            <label>Type of Base Plate</label> </br>
    
            <input type="radio" name="type" value=10000 checked>	High Precision 	<br/>
            <input type="radio" name="type" value=20000>		Push-Pin 		<br/>
          </fieldset>
    
          <br/>
    
    
          <fieldset>
            <label>Age</label> </br>
    
            <input type="radio" name="age" value=1000 checked>			Adult 	<br/>
            <input type="radio" name="age" value=2000>		Paediatric 		<br/>
          </fieldset>
    
          <br/>
    
          <fieldset>
            <label>Anatomical Region</label> </br>
    
            <input type="radio" name="anatomical" value=100 checked>				Brain 				<br/>
            <input type="radio" name="anatomical" value=200>		Head, Neck and Shoulders	<br/>
          </fieldset>
    
          <br/>
    
          <fieldset>
            <label>Type of Patient</label> </br>
    
            <input type="radio" name="patient" value=10 checked>			Curative 			<br/>
            <input type="radio" name="patient" value=20>			Palliative			<br/>
            <input type="radio" name="patient" value=30>		Claustrophobic		<br/>
          </fieldset>
    
          <br/>
    
          <fieldset>
            <label>Accuracy</label> </br>
    
            <input type="radio" name="accuracy" value=1 checked>		&lt;1mm 		<br/>
            <input type="radio" name="accuracy" value=2>		&lt;2mm		<br/>
          </fieldset>
    
          <br/>
    
          <button type="submit" value="Submit">				Submit	</button>
    
        </form>
    
        <br/>
    
        <!--<button type="reset" value="Reset" id="resetBtn">		Reset		</button>-->
    
      </section>
    
      <p id="answer"></p>
    </article>

4 Comments

Gotta love how this essentially "homework" question is becoming a playground for elegant JS solutions ;) Just out of curiosity: has Number(x) become the new parseInt(x, 10)?
Also: total can be const, answer can be const in the init scope.
I use both pretty often tbh, but Number('123hi') will return NaN which for this use case makes it a stronger filter.
ahh, I totally should have changed those ;p. Good catch
0

The primary issue is that you are re-declaring your variables inside the .forEach() loops, which causes the outer-scoped version of the variable not to get the value. If you simply remove the var from the .forEach() lines, then references to the variables will match the higher-scoped ones.

In your getValues() function, you are expecting values to be passed in and you are using the same names as your variables. This won't cause the variables to be passed in, it will create newly scoped variables that will hide the outer scoped ones. You can remove them and just access the higher scoped variables.

You also have to know/remember that HTML has just one data type...text. When you read the values of HTML elements into JavaScript, all those values will be strings. To do math with those values, you have to convert the strings to numbers.

Also, you have quite a few HTML errors....

While technically, <br/> is legal, it is only useful in a very few cases and should be avoided because unnecessary use of it will invariably lead to bugs, which you have because you've written </br> several times (which is never valid). In addition, you've written <fieldset/>, which isn't allowed. Also, don't use br for visual styling (adding blank lines). br is a semantic tag which indicates content continues on the next line. For styling, use CSS.

Next, you were using <label> elements inside of your fieldset elements as titles for the field sets, but that's the job of <legend>. <label> is for associating content with a form field, it's not to title fieldsets.

You've also got a submit button, when you aren't actually submitting data anywhere. In those cases, use type=button. Additionally, since you aren't actually submitting data, you don't even need the <form> element.

Now, lastly, you are repeating the same code over and over again with the only difference being that each loop totals up a different variable. In programming, the moment you write the same code twice, you should stop and think about how to not do that. It's called the DRY (Don't Repeat Yourself) principle.

If you take a look at the comments in the working code below, you will see how all those loops and functions can be reduced.

// Get reference to the HTML elements that we'll need individual access to:
var answer = document.getElementById("answer");
var submit = document.querySelector("button[type=button]");

// Set up event binding in JavaScript, not HTML
submit.addEventListener("click", getValues);

// Will store final answer:
var result = null;

// This object has properties that will actually store
// the radio button values as needed. By initializing these
// properties at the same values as their corresponding
// radio button values, we ensure that a valid answer is 
// reported even if the user doesn't do any initial changes
// to the radio button selections and just clicks submit.
var scopeObject = {
  basePlate:10000, 
  age: 1000,
  region: 100,
  patientType: 10,
  accuracy: 1
};

// Since all your event handlers do pretty much the same thing (set the value of the
// clicked element into a particular variable, we can simplify things greatly by setting
// up two arrays: one for the varaibles to store data and one for the question groups.
// Then, we can just match the index of the question group to the index of the variable
// and we'll only need one function:
var questionGroups = ["Type of Base Plate", "Age", "Anatomical Region", "Type of Patient", "Accuracy"];
var vars = ["basePlate", "age", "region", "patientType", "accuracy"];

// Next, we'll set up just one callback function for all radio buttons, but
// not all browsers will treat the HTML Collection returned from
// .querySelectorAll() as an array, which means that .forEach() 
// won't work on questions unless you explicitly convert it 
// to an array. 

// Also, the convention for capitalization of identifiers is to use 
// PascalCase for constructor functions, ALLCAPS for constants and
// camelCase for everything else.
var questions = Array.prototype.slice.call(document.querySelectorAll('input[type=radio]'));

questions.forEach((radio)=>{
  radio.addEventListener('change',(e)=>{
    // Get the text of the legend of the current radio button's fieldset
    var leg = e.target.parentElement.querySelector("legend").textContent;
    
    // Look up the index number of that text in the questionGroups array:
    var idx = questionGroups.indexOf(leg);
    
    // Set the corresponding variable in the variables array to the value of the
    // radio button that just got changed:
    scopeObject[vars[idx]] = e.target.value;
  });
}); 
    
function getValues() {
  // Throw out old result
  result = null;
  // Loop through all the properties in our scope object and sum all the values
  for(var prop in scopeObject){
    // The + in front of scopeObject is needed to explicitly convert the string
    // values from the radio buttons into numbers so that the addition works
    result += +scopeObject[prop];
  }
  answer.textContent = result;
}
/* Don't use <br> as a styling tool. That's what CSS is for. */
fieldset {
  margin-bottom:1em; /* This puts one line of space below each fieldset. No <br> needed. */
}
<article>
	<h1 class="headings">New Mask</h1>
		<fieldset>		
      <!-- Fieldsets get <legend>, not <label> -->
		  <legend>Type of Base Plate</legend> <br>
			
		  <input type="radio" name="type" value="10000" checked>High Precision<br>
		  <input type="radio" name="type" value="20000"> Push-Pin<br>
		</fieldset>
			
		<fieldset>
      <legend>Age</legend>
			
	    <input type="radio" name="age" value="1000" checked>			Adult 	<br>
		  <input type="radio" name="age" value="2000">		Paediatric 		<br>
		</fieldset>
					
		<fieldset>
		  <legend>Anatomical Region</legend>
			
		  <input type="radio" name="anatomical" value="100" checked> Brain<br>
		  <input type="radio" name="anatomical" value="200"> Head, Neck and Shoulders<br>
		</fieldset>
			
		<fieldset>
		  <legend>Type of Patient</legend>
				
		  <input type="radio" name="patient" value="10" checked> Curative<br>
		  <input type="radio" name="patient" value="20"> Palliative<br>
		  <input type="radio" name="patient" value="30"> Claustrophobic<br>
		</fieldset>
						
		<fieldset>
		  <legend>Accuracy</legend>
		
		  <input type="radio" name="accuracy" value="1" checked> &lt; 1mm<br>
		  <input type="radio" name="accuracy" value="2"> &lt; 2mm<br>
		</fieldset>

    <!-- Don't use a "submit" button when you aren't submitting data anywhere. 
         And, don't use inline HTML event attribtues. Do event binding in JavaScript. -->
		<button type="button" value="Submit">Submit</button>
			
	<p id="answer"></p>
</article>

Now, having shown that example, if you really think about it, why do you need to get the total calculated each time the user changes a radio button value, when you aren't actually asking for that value until the submit button is clicked? Essentially, if I click away on those buttons, but never click submit, I will have executed the code many times for no reason. That scenario also uses more memory that it needs to because each and every radio button has to now store its own change event callback function. The better approach is to simply get the sum of all the checked radio button values when the user clicks submit. That solution is even simpler:

// Get reference to the HTML elements that we'll need individual access to:
var answer = document.getElementById("answer");
var submit = document.querySelector("button[type=button]");

// Set up event binding in JavaScript, not HTML
submit.addEventListener("click", getValues);
   
function getValues() {
  // Get only the checked radio buttons (notice the ":checked" in the selector)
  // into an HTML collection and then convert that collection into an array:
  var questions = Array.prototype.slice.call(document.querySelectorAll('input[type=radio]:checked'));

  // Will store final answer:
  var result = null;
  
  questions.forEach((radio)=>{
    // Remember: HTML values are strings. To do math, you need to 
    // convert the strings to numbers. The "+" right in front of "radio.value" 
    // does this.
    result += +radio.value;
  });

  // Output the answer into the HTML
  answer.textContent = result;
}
/* Don't use <br> as a styling tool. That's what CSS is for. */
fieldset {
  margin-bottom:1em; /* This puts one line of space below each fieldset. No <br> needed. */
}
<article>
	<h1 class="headings">New Mask</h1>
		<fieldset>		
      <!-- Fieldsets get <legend>, not <label> -->
		  <legend>Type of Base Plate</legend> <br>
			
		  <input type="radio" name="type" value="10000" checked>High Precision<br>
		  <input type="radio" name="type" value="20000"> Push-Pin<br>
		</fieldset>
			
		<fieldset>
      <legend>Age</legend>
			
	    <input type="radio" name="age" value="1000" checked>			Adult 	<br>
		  <input type="radio" name="age" value="2000">		Paediatric 		<br>
		</fieldset>
					
		<fieldset>
		  <legend>Anatomical Region</legend>
			
		  <input type="radio" name="anatomical" value="100" checked> Brain<br>
		  <input type="radio" name="anatomical" value="200"> Head, Neck and Shoulders<br>
		</fieldset>
			
		<fieldset>
		  <legend>Type of Patient</legend>
				
		  <input type="radio" name="patient" value="10" checked> Curative<br>
		  <input type="radio" name="patient" value="20"> Palliative<br>
		  <input type="radio" name="patient" value="30"> Claustrophobic<br>
		</fieldset>
						
		<fieldset>
		  <legend>Accuracy</legend>
		
		  <input type="radio" name="accuracy" value="1" checked> &lt; 1mm<br>
		  <input type="radio" name="accuracy" value="2"> &lt; 2mm<br>
		</fieldset>

    <!-- Don't use a "submit" button when you aren't submitting data anywhere. 
         And, don't use inline HTML event attribtues. Do event binding in JavaScript. -->
		<button type="button" value="Submit">Submit</button>
			
	<p id="answer"></p>
</article>

10 Comments

+1 for the DRY metaprogramming approach; -1 for it being way above OPs level of comprehension. I appreciate the good intentions but this breeds cargo cult programmers :(
Also, while we're at it, why not var vars = Object.keys(scopeObject)? ;)
@vzwick I appreciate your feedback. As a professional IT trainer for over 20 years, I have found that tackling core programming patterns, methods and best practices right from the start raises the level of the learner's comprehension from day 1. The only thing that I would categorize as perhaps beyond introduction level is the use of the object to store properties that can be looked up as strings later. Also, why not Object.keys? No particular reason. Why not for(var props in scopeObject)?
@vzwick Let's not forget that the OP is using arrays, .forEach and arrow functions, so he can't be too much of a beginner (needs a good tutorial on scope though). Lastly, I've added sufficient comments in the code to explain what is happening, but we've got to leave the OP some investigation of his own.
A propos for ... in - I find it a good habit to include a hasOwnProperty() check on every for ... in loop. YMMV.
|
-1

The problem is that you have change event that means it doesnt count them as checket but you also must return a value to the variable like this

    questiontype.forEach((radio)=>{
       radio.addEventListener('change',(e)=>{
      return numo = e.target.value;
    });
   });

1 Comment

This is plain wrong. Try for yourself, remove the return and it will work just as well.

Your Answer

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