simple die roll application issue

Question

I am learning javascript and I have written a short die roll app but it does not run when the button is clicked. I can't see where the problem is. Can somebody help me with this.

    <!DOCTYPE HTML>
<html>
    <head>
        <script type=text/javascript>

        var face0= new Image() face0.src="d1.gif"
        var face1= new Image() face1.src="d2.gif"
        var face2= new Image() face2.src="d3.gif"
        var face3= new Image() face3.src="d4.gif"
        var face4= new Image() face4.src="d5.gif"
        var face5= new Image() face5.src="d6.gif"

        function rollDice(){
        var randomdice= Math.round(Math.random() *5)document.images["mydice"].src=eval("face" * randomdice+".src")
        }
        </script>
    </head>
    <body>
    <img src="d1.gif" name="mydice">
    <form>
    <input type="button" value="Roll" onclick="rollDice()">
    </form>

    </body>
</html>

WTF is `Math.round(Math.random() *5)document.images["mydice"].src=eval("face" * randomdice+".src")` supposed to do? Fix your syntax errors, and do not use `eval` (but an array for your images). — Bergi, Nov 27 '12 at 14:59
@Bergi `eval` is perfectly fine in this situation. Although I do agree an array makes much more sense. — Ian, Nov 27 '12 at 15:01
its a function that is supposed to randomly display the 6 images I have stored. — user1810449, Nov 27 '12 at 15:04

score 2 · Answer 1 · answered Nov 27 '12 at 14:59

Semicolons are not optional. The browser will try to insert them as best it can, but it can be wrong a lot of the time.

Additionally, use a @!#$ array! That's what they're there for!

Try this:

(function() {
    var i, faces = [];
    for( i=0; i<6; i++) {
        (faces[i] = new Image()).src = "d"+(i+1)+".gif";
    }
    window.rollDice = function() {
        var side = Math.floor(Math.random()*6);
        document.getElementById('mydice').src = faces[side].src;
    }
})();

...

<img src="d1.gif" id="mydice" />

And remove the form. You are not submitting anything, so you don't need a form.

Could the downvoter please explain what I did wrong so I might rectify it? — Niet the Dark Absol, Nov 27 '12 at 15:13

Joshua Dwire · Accepted Answer · 2012-11-27T15:12:57.250

1

Please don't use eval. Instead do something like this:

var faces=[]
var numFaces=6;
for(var i=1;i<=numFaces;i++){
    var img=new Image();
    img.src='d'+i+'.gif';
    faces.push(img);
}
window.rollDice=function(){
    var randomdice= Math.floor(Math.random()*numFaces);
    document.images["mydice"].src=faces[randomdice].src;
}

jsFiddle: http://jsfiddle.net/jdwire/fZY3u/

edited Nov 27 '12 at 15:12

answered Nov 27 '12 at 15:01

Joshua Dwire

5,415
5
29
50

I would +1 this, but I'd hate to see what your code would be for a 100-sided die... – Niet the Dark Absol Nov 27 '12 at 15:03
@Kolink I would love to see a 100-sided die – Ian Nov 27 '12 at 15:04
@Kolink Maybe I should change it to a for loop... – Joshua Dwire Nov 27 '12 at 15:04
2

@Ian [Wish granted](http://unclesgames.com/images/products/N00388_big.jpg) - I was amazed when I saw this thing for real. – Niet the Dark Absol Nov 27 '12 at 15:05
@Kolink Oh no! Haha I didn't expect there to be one for real – Ian Nov 27 '12 at 15:06
@Kolink My (revised) code for a 100-sided die complete with (404) image preloading: http://jsfiddle.net/jdwire/3JjHK/ – Joshua Dwire Nov 27 '12 at 15:19
If I was to get the value of the die which was rolled and display it, would I need to follow a different solution?? @jdwire. – user1810449 Nov 27 '12 at 15:48
@user1810449 Do you mean you want to display the text of the number rolled? If so, add 1 to the `randomDice` variable in the `rollDice` function, and that will be the number rolled. Use that to display the number rolled somewhere as text. – Joshua Dwire Nov 27 '12 at 15:50
Yes display the text of the number rolled. Ok I shall try this Thanks. – user1810449 Nov 27 '12 at 15:53
So with in the roll dice function, add 1 to randomdice variable like so>> var randomdice= Math.floor(Math.random()*numFaces)+1; – user1810449 Nov 27 '12 at 16:04
@user1810449 Only do that if you don't want to display the image otherwise, it will cause null reference errors. If you still want display the image, just add one to the value when you display it: `element.innerHTML='You rolled a '+(randomdice+1);` – Joshua Dwire Nov 27 '12 at 16:06
Oh okay, I have done it by adding the following code within the roll dice function and displayed it in a textbox on the form var results=randomdice+1; document.getElementById("res").value = results; – user1810449 Nov 27 '12 at 16:16

score 0 · Answer 3 · edited May 23 '17 at 12:27

0

You are almost there - you just need semi colons between your statements, and you need to concatenate strings with '+', not '*'

Edit

As Kolink points out, eval is dangerous.

You don't actually need to instantiate Image()s at all - given that you have 6 gifs for the sides of the die, all you need to do is change the src filename on your image, like so:

 function rollDice(){
     var randomdice= Math.round(Math.random() *5) + 1; // +1 since your gifs are 1-6
     document.images["mydice"].src = "d" + randomdice.toString() + ".gif";
 }

Updated Fiddle here

Edit Be aware of the caveats noted by the points below that this answer doesn't preload the images, which will have a performance hit the first time each dice face gif is triggered for the first time, before it is cached.

edited May 23 '17 at 12:27

Community

1
1

answered Nov 27 '12 at 14:59

StuartLC

104,537
17
209
285

-1 for encouraging the horrific use of `eval`. – Niet the Dark Absol Nov 27 '12 at 15:02
Well, I didn't explicitly discourage it, but OP seemed to have his mind set on `eval` already :) – StuartLC Nov 27 '12 at 15:05
I think Kolink's point is not that it's evil...it's perfectly fine in this situation, but it's being used as an attempt to replace an array, which isn't the best approach. – Ian Nov 27 '12 at 15:12
And instantiating `Image`s isn't necessary as you've pointed out, but it preloads them, so they are cached for faster loading later. Not that faster loading is necessary for a simple situation like this – Ian Nov 27 '12 at 15:14
Worst part is, you can even use `window['face'+randomdice]` and it's better than `eval`. – Niet the Dark Absol Nov 27 '12 at 15:14
@Kolink Huh, didn't think of that. I guess that makes sense since everything's in the global scope. That's what makes your solution nice, nothing extra is exposed – Ian Nov 27 '12 at 15:21
This code is good, but it won't preload the images like the OP's code will. – Joshua Dwire Nov 27 '12 at 15:23

Muthu Kumaran · Answer 4 · 2012-11-27T15:05:15.873

-1

You missed semi-colons ;

    var face0= new Image(); face0.src="d1.gif";
    var face1= new Image(); face1.src="d2.gif";
    var face2= new Image(); face2.src="d3.gif";
    var face3= new Image(); face3.src="d4.gif";
    var face4= new Image(); face4.src="d5.gif";
    var face5= new Image(); face5.src="d6.gif";

    function rollDice(){
       var randomdice= Math.round(Math.random() *5); 
       document.images["mydice"].src=eval("face" + randomdice+".src");
    }

Note: Always format your code neatly. Don't write all codes in one line, it will confuse you.

edited Nov 27 '12 at 15:05

answered Nov 27 '12 at 14:57

Muthu Kumaran

17,682
5
47
70

Thanks, Ive just add the semi colons but it still does not run... – user1810449 Nov 27 '12 at 14:59
-1 for not correcting the horrific misuse of `eval`. – Niet the Dark Absol Nov 27 '12 at 15:02
@Kolink How is this a misuse? – Ian Nov 27 '12 at 15:02
The other problem is `eval("face" * randomdice+".src")` - it needs to be `eval("face"+randomdice+".src")` as someone else already pointed out – Ian Nov 27 '12 at 15:03
I didn't notice `eval` there. `eval` is not even required there. – Muthu Kumaran Nov 27 '12 at 15:04
If the code is refactored, then it isn't required, that's correct. But in its current state, the "*" character needs to be changed to a "+", was my point. – Ian Nov 27 '12 at 15:05
@Ian I changed it to `+` – Muthu Kumaran Nov 27 '12 at 15:07
@Kolink I mean, I definitely agree an array would be the better solution, but using `eval` isn't evil in this case. It's not like the user is `eval`ing user input or database-stored information. And it's only for 6 items. Nonetheless, it's better to teach the preferred method. These answers are solutions to the short-term problem, not the long-term – Ian Nov 27 '12 at 15:12
wOW this question engaged alot of interest, Id like to thank you guys and I will learn alot from this. :) – user1810449 Nov 27 '12 at 15:39

simple die roll application issue

4 Answers4