0

I've done FizzBuzz several times but never had this problem. Perhaps, it is something fundamental about for-loops that I don't understand. For some reason, the code below runs 10x longer than it should (well, than I think it should). If the user enters 20, it runs to 200. I fixed the problem by setting i = 0; i < num and then printing i+1 to my div, but I still don't understand why the original code does not work as expected. And while I'm at it, I might as well admit that I still can't set up JSFiddle properly. http://jsfiddle.net/nngrey/hA4pg/ (This does not run at all.) So any thoughts on that would also be appreciated. Thanks!

<head>
  <title>Fizz Buzz</title>
  <script>
    function fizzbuzz(){
      var num = prompt("Please enter a number between 1 and 100: ");
      for(var i=1; i<num+1; i++){
        if (i%3===0 && i%5===0){
          document.getElementById("div1").innerHTML = div1.innerHTML+"<p>Fizz Buzz</p>";
        }else if (i%3===0){
          document.getElementById("div1").innerHTML = div1.innerHTML+"<p>Fizz</p>";
        }else if (i%5===0){
          document.getElementById("div1").innerHTML = div1.innerHTML+"<p>Buzz</p>";
        }else{
          document.getElementById("div1").innerHTML = div1.innerHTML+"<p>"+i+"</p>";
        }
      }
    }
  </script>
</head>

<body onLoad = "fizzbuzz()">
  <div id = "div1">
    <h1>Fizz Buzz</h1>
  </div> 
</body>
Improve this question
7
  • to fix your fiddle put fizzbuzz(); at the bottom of the script window and take it out the body tag Commented Oct 25, 2013 at 4:08
  • If you choose "no wrap," on the left hand menu of jsfiddle, your code will run. The problem is that jsfiddle is adding all the javascript into the onLoad. You are calling it from onLoad in your HTML, so the function needs to be declared in <head> or in <body>. Commented Oct 25, 2013 at 4:08
  • Where are you declaring the div1 variable? If this works for you then you're running in IE which is the only browser that auto-creates global variables based on element's ids. Don't depend on it. Always use getElementById. Also, you really need to do getElementById once and save a reference to the DOM element in a variable. Named div1 perhaps? Commented Oct 25, 2013 at 4:09
  • @slebetman: Chrome has done this too for a long time, and now Firefox as well. Commented Oct 25, 2013 at 4:12
  • foobarbeque - You are right! I tried going through those options but I must have been too impatient. I probably did not notice it was still processing. JSFiddle seems to be running slow tonight. Snowburnt - I will try your suggestion too. Commented Oct 25, 2013 at 4:24

4 Answers 4

Reset to default
4

In your code, prompt() returns a string. Javascript will evaluate this line:

      for(var i=1; i<num+1; i++){

with num as a string. i.e num+1 becomes "20"+"1" (note the quotes) which is "201". the comparison is then evaluated numerically, so your loop runs ten times linger than it should.

In your revised version i < num is evaluated numerically, so the loop runs for the correct period.

You can force num to be a number like this:

      var num = Number(prompt("Please enter a number between 1 and 100: "));

num is now a number, so 20 + 1 = 21 (note - no quotes) and both versions of your loop should operate correctly

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

2 Comments

So num+1 becomes the string "201" and the string "201" is converted to the int 201 when it is compared to i the first time? Is num+1 executed before the first iteration of the for-loop or immediately before it is compared to i?
@Nathan I don't know for sure when the test is evaluated. In this instance it could be evaluated before the loop, but code inside the loop could theoretically change the condition, so I suspect the test is completely re-evaluated on every iteration.
0

You need to do:

var num = parseInt(prompt("Please enter a number between 1 and 100: "), 0);

prompt returns a string, so if you enter 20, num+1 is the string "201", not the number 21.

Improve this answer

Comments

0

here num is a string you have to use parseInt to convert it to int

for(var i=1; i<parseInt(num)+1; i++){
}
Improve this answer

Comments

0

The prompt() returns a string.

Simple use +prompt() instead. That should make it a number. Updated code demo.

Improve this answer

3 Comments

That's handy - thanks. So "+" or "Number" or "parseInt" would all work. I don't suppose one is vastly better than the other? "+" saves on keystrokes! ;-)
Actually there are quite a few differences than just saving keystrokes... Have a look: stackoverflow.com/questions/17106681/…
Ah...looks like "+" and "Number" will both preserve the decimal spaces in a float. Maybe I should used parseInt for my project to correct for non-int submissions.