Javascript exiting Loop Early

Question

I have a loop for a coin flipping program that I'm making. the problem is the it seems to be exiting early. Take a look.

$(function() {
    $('#rollDice').click(function() {

        var e = document.getElementById("diceSides");
        var diceSides = e.options[e.selectedIndex].text;
        var diceRolls = document.getElementById('rollCount').value;

        if (diceRolls.match(/^[\d]*$/ )) {      
            if (diceRolls == "")  {
                alert ("Please fill out all forms then try again.");

            } else {

                $('#diceRollContainer').slideDown('slow');

                for (i=0;i<diceRolls;i++) {
                    var randNum = Math.floor(Math.random()*diceSides)+1;
                    var rolls = ("You rolled a " + diceSides + " sided die " + diceRolls + " times, and got the numbers ");

                    rollMinOne = rolls - 1;
                    if (i == rollMinOne) {
                        var rolls = (rolls + randNum + ".");
                    }
                    var rolls = (rolls + randNum + ", ");

                }
                alert (rolls);
            }
        } else {
            alert ("Make sure you only enter numbers and no spaces, then try again.");
        }
    });
});

The problem is that the program is alerting rolls before the for loop seems to be completed. Why is it doing this?

What is it alerting, and what did you expect? Looking at that, I'd expect something like You rolled a 4 sided die 3 times, and got the numbers 1, 2, 3.3, — Simon Buchan
– Simon Buchan, Commented Apr 12, 2012 at 3:15
I'm not sure I understand. Would could I do to fix this? Also, why can't I comment on the answer? — Jack Davis
– Jack Davis, Commented Apr 12, 2012 at 3:22

Simon Buchan · Accepted Answer · 2012-04-12 03:50:01Z

2

You have several bugs in that code, but the one that explains the behavior you're seeing is that you reset the value of rolls every time through the loop to the initial string.

Once you move that line out and you get a closer value, but you are also computing rollsMinOne from rolls, rather than diceRolls, as you intended (this is why choosing good names is so important), meaning that if statement is never true (since a string minus a number is the value NaN "Not a Number", which is not equal to anything [even itself!]).

Then the only functional (rather than style or design) problem is that you are then appending a value with a comma on the end even if you already added it with a period.

Putting it all together:

    var rolls = ("You rolled a " + diceSides + " sided die " + diceRolls + " times, and got the numbers ");
    for (i=0;i<diceRolls;i++) {
        var randNum = Math.floor(Math.random()*diceSides)+1;

        rollMinOne = diceRolls - 1;
        if (i == rollMinOne) {
            rolls = (rolls + randNum + ".");
        } else {
            rolls = (rolls + randNum + ", ");
        }

Though as the other answers mention, there are easier and faster ways to get the same result, I feel it's important to understand why code doesn't work.

answered Apr 12, 2012 at 3:50

Simon Buchan

13.3k2 gold badges55 silver badges57 bronze badges

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

1 Comment

Jack Davis Over a year ago

Thanks, that is what I'm looking for, I want to understand it. Besides, it's just practice right now anyway.

user238638 · Accepted Answer · 2012-04-12 03:29:51Z

i got bored and implemented your code, this seems to work with minimal testing

<script>
    $(function() {
        $('#rollDice').click(function() {

            var diceSides = $('#dice-sides').val();
            var diceRolls = $('#roll-count').val();

            if (diceRolls.match(/^[\d]*$/ )) {      
                if (diceRolls == "")  {
                    alert ("Please fill out all forms then try again.");

                } else {
                    $('#output').text(
                        "You rolled a " + diceSides + 
                        " sided die " + diceRolls + 
                        " times, and got the numbers ");

                    for (i=0; i<diceRolls; i++) {

                        var randNum = Math.floor(Math.random()*diceSides)+1;

                        $('#output').append(randNum);
                    }
                }
            } else {
                alert ("Make sure you only enter numbers and no spaces, then try again.");
            }
        });
    });
</script>
<form onsubmit="return false;">
    <label>Sides</label>
    <input id="dice-sides" type="text" value="6">
    <label>Count</label>
    <input id="roll-count" type="text" value="1">
    <button id="rollDice">Roll</button>
</form>
Rolls
<div id="output"> 

</div>

Collectives™ on Stack Overflow

Javascript exiting Loop Early

2 Answers 2

1 Comment

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

1 Comment

Comments

Your Answer

Sign up or log in

Post as a guest

Related