patternjavascriptMinor
Roll the dice - to give a random output with a die
Viewed 0 times
randomthediewithgiveoutputrolldice
Problem
I was playing a game with my family and we lost the dice that went with the game, so I created this small program and I am now trying to create a website with it.
```
mouseClicked = function()
{
// change to change max and minimum numbers
var number= random(0.5, 6.4);
// makes number into intiger
var integer = round(number);
// creates background
fill(255, 255, 255);
// makes the dice face be 1
if(integer === 1)
{
fill(245, 242, 242);
rect(100, 100, 200, 200);
fill(0, 0, 0);
ellipse(200,200, 100, 100);
}
// makes the dice face 4
if(integer === 2)
{
fill(245, 242, 242);
rect(100, 100, 200, 200);
fill(0, 0, 0);
ellipse(150,150, 70, 70);
ellipse(250,250, 70, 70);
}
// makes the dice face be 3
if(integer === 3)
{
fill(245, 242, 242);
rect(100, 100, 200, 200);
fill(0, 0, 0);
ellipse(150,150, 60, 60);
ellipse(200, 200, 60, 60);
ellipse(250, 250, 60, 60);
}
// make the dice face be 4
if(integer === 4)
{
fill(245, 242, 242);
rect(100, 100, 200, 200);
fill(0, 0, 0);
ellipse(150, 150, 60, 60);
ellipse(150, 250, 60 ,60);
ellipse(250, 250, 60, 60);
ellipse(250, 150, 60, 60);
}
// make the dice face be 5
if(integer === 5)
{
fill(245, 242, 242);
rect(100, 100, 200, 200);
fill(0, 0, 0);
ellipse(150, 150, 60, 60);
ellipse(150, 250, 60 ,60);
ellipse(250, 250, 60, 60);
ellipse(250, 150, 60, 60);
ellipse(200, 200, 60, 60);
}
// make the dice face be 6
if(integer === 6)
{
fill(245, 242, 242);
rect(100, 100, 200, 200);
fill(0, 0, 0);
ellipse(150, 150, 45, 45);
ellipse(150, 250, 45, 45);
ellipse(250, 250, 45, 45);
ellipse(250, 150, 45, 45);
ellipse(250, 200, 45, 45);
ellipse(150, 200, 45, 45);
```
mouseClicked = function()
{
// change to change max and minimum numbers
var number= random(0.5, 6.4);
// makes number into intiger
var integer = round(number);
// creates background
fill(255, 255, 255);
// makes the dice face be 1
if(integer === 1)
{
fill(245, 242, 242);
rect(100, 100, 200, 200);
fill(0, 0, 0);
ellipse(200,200, 100, 100);
}
// makes the dice face 4
if(integer === 2)
{
fill(245, 242, 242);
rect(100, 100, 200, 200);
fill(0, 0, 0);
ellipse(150,150, 70, 70);
ellipse(250,250, 70, 70);
}
// makes the dice face be 3
if(integer === 3)
{
fill(245, 242, 242);
rect(100, 100, 200, 200);
fill(0, 0, 0);
ellipse(150,150, 60, 60);
ellipse(200, 200, 60, 60);
ellipse(250, 250, 60, 60);
}
// make the dice face be 4
if(integer === 4)
{
fill(245, 242, 242);
rect(100, 100, 200, 200);
fill(0, 0, 0);
ellipse(150, 150, 60, 60);
ellipse(150, 250, 60 ,60);
ellipse(250, 250, 60, 60);
ellipse(250, 150, 60, 60);
}
// make the dice face be 5
if(integer === 5)
{
fill(245, 242, 242);
rect(100, 100, 200, 200);
fill(0, 0, 0);
ellipse(150, 150, 60, 60);
ellipse(150, 250, 60 ,60);
ellipse(250, 250, 60, 60);
ellipse(250, 150, 60, 60);
ellipse(200, 200, 60, 60);
}
// make the dice face be 6
if(integer === 6)
{
fill(245, 242, 242);
rect(100, 100, 200, 200);
fill(0, 0, 0);
ellipse(150, 150, 45, 45);
ellipse(150, 250, 45, 45);
ellipse(250, 250, 45, 45);
ellipse(250, 150, 45, 45);
ellipse(250, 200, 45, 45);
ellipse(150, 200, 45, 45);
Solution
@janos already covered this a little bit, but if you want to use the built-in
But, we need to do one more thing. Since the
Now you should be able to generate random numbers from 1-6 in a cleaner, and more readable manner.
I've also noticed that you're repeating a lot of code. @Caridorc already touched on this a little bit, but I'm going to expand on it a bit further. Rather than keeping it in the
This is a good thing to remember when you're doing graphics programming. If there's no need to re-draw something every frame, or click, then don't. In this case, you don't need to re-draw and fill the rectangle, so it can be moved entirely out of the
Expanding on the subject of code repetition, you're also repeating a lot of
To use this function, you supply a list of
Finally, rather than using a chain of
But, we can get better than a chain of
The first value of each list is the diameter of the circles, and the rest are positions. The nice thing about this is that it's easily expandable, rather than having to add a new
In addition, thanks to the tips from @Jonah, I'd also recommend the following things:
After making all these changes, plus a few more, I ended up with the following, improved (hopefully) code:
Finally, I'd just like to nitpick a few things:
random(x, y) used in Processing.JS, I'd do something like this:var numberOfDots = random(1, 6);But, we need to do one more thing. Since the
random(x, y) function in Processing.JS can return non-integer results, we need to use the floor function to round the returned value down, and then add one. This means that your calculation simply becomes this:var numberOfDots = floor(random(1, 6)) + 1;Now you should be able to generate random numbers from 1-6 in a cleaner, and more readable manner.
I've also noticed that you're repeating a lot of code. @Caridorc already touched on this a little bit, but I'm going to expand on it a bit further. Rather than keeping it in the
mouseClicked override, you can simply put it at the top of your code like this:fill(245, 242, 242);
rect(100, 100, 200, 200);
mouseClicked = function() {
...
}This is a good thing to remember when you're doing graphics programming. If there's no need to re-draw something every frame, or click, then don't. In this case, you don't need to re-draw and fill the rectangle, so it can be moved entirely out of the
mouseClicked override. In the case of this program though, I felt that the rectangle wasn't needed, so I just removed it altogether.Expanding on the subject of code repetition, you're also repeating a lot of
ellipse calls. I'd highly suggest implementing a function that automatically draws x amount of circles for you. This is what I came up with:var drawDiceDots = function(dotPositions, dotDiameter) {
for(var i = 0; i <= dotPositions.length - 1; ++i) {
ellipse(dotPositions[i][0], dotPositions[i][1], dotDiameter, dotDiameter);
}
};To use this function, you supply a list of
x-y positions in the format [[x, y], ...]. For example, in order to render the dots on the 6-dot side of a die, you'd do this:drawDiceDots([
[150, 150],
[150, 250],
[250, 250],
[250, 150],
[250, 200],
[150, 200]
], 45);Finally, rather than using a chain of
if statements, I'd recommend using a switch/case construct. A typical switch/case construct looks something like this:switch(someVariable) {
case someValue:
...
break;
...
}But, we can get better than a chain of
if statements. We can either use a JavaScript object or a list. In this case, I'm going to use a list. To do this, you'd first create a list that looks like this:var possibleDiceFaces = [
[100, [200, 200]],
[70, [150, 150], [250, 250]],
[60, [150, 150], [200, 200], [250, 250]],
[60, [150, 150], [150, 250], [250, 250],[250, 150]],
[60, [150, 150], [150, 250], [250, 250], [250, 150], [200, 200]],
[45, [150, 150], [150, 250], [250, 250], [250, 150], [250, 200], [150, 200]]
];The first value of each list is the diameter of the circles, and the rest are positions. The nice thing about this is that it's easily expandable, rather than having to add a new
case or if statement each time you want to add a "side".In addition, thanks to the tips from @Jonah, I'd also recommend the following things:
- Separate responsibilities, e.g, simply call a dice drawing function in the
mouseClickoverride, rather than having a huge pile ofcasestatements.
- Use more domain-oriented names for things, rather than implementation-oriented.
After making all these changes, plus a few more, I ended up with the following, improved (hopefully) code:
/**
* Draw x amount of dots given a list of positions
* for a dice.
* @param {list[][]} dotPositions - A list of dot positions in the form [x, y].
* @param {number} dotDiameter - The diameter of the dots.
*/
var drawDiceDots = function(dotPositions, dotDiameter) {
for(var i = 1; i <= dotPositions.length - 1; ++i) {
fill(0, 0, 0);
ellipse(dotPositions[i][0], dotPositions[i][1], dotDiameter, dotDiameter);
}
};
/**
* Draw the fact of a dice. By default, there are 6 different faces,
* but this number can be changed to add more.
*/
var drawDiceFace = function(rolledNumber) {
var possibleDiceFaces = [
[100, [200, 200]],
[70, [150, 150], [250, 250]],
[60, [150, 150], [200, 200], [250, 250]],
[60, [150, 150], [150, 250], [250, 250],[250, 150]],
[60, [150, 150], [150, 250], [250, 250], [250, 150], [200, 200]],
[45, [150, 150], [150, 250], [250, 250], [250, 150], [250, 200], [150, 200]]
];
if(rolledNumber in possibleDiceFaces) {
drawDiceDots(possibleDiceFaces[rolledNumber], possibleDiceFaces[rolledNumber][0]);
}
else {
throw "The rolled number specified is not valid.";
}
};
mouseClicked = function() {
background(255, 255, 255);
var rolledNumber = floor(random(1, 6)) + 1;
drawDiceFace(rolledNumber);
};Finally, I'd just like to nitpick a few things:
- Generally you should wri
Code Snippets
var numberOfDots = random(1, 6);var numberOfDots = floor(random(1, 6)) + 1;fill(245, 242, 242);
rect(100, 100, 200, 200);
mouseClicked = function() {
...
}var drawDiceDots = function(dotPositions, dotDiameter) {
for(var i = 0; i <= dotPositions.length - 1; ++i) {
ellipse(dotPositions[i][0], dotPositions[i][1], dotDiameter, dotDiameter);
}
};drawDiceDots([
[150, 150],
[150, 250],
[250, 250],
[250, 150],
[250, 200],
[150, 200]
], 45);Context
StackExchange Code Review Q#113933, answer score: 6
Revisions (0)
No revisions yet.