3
\$\begingroup\$

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);
    }
};

Ideally, I'd like to know the following things:

  • Is there a way I could use an array or something to simplify this code?
  • Are there any changes I should make?
  • How can I make the code more efficient?

Demo can be found here

\$\endgroup\$
6
\$\begingroup\$

@janos already covered this a little bit, but if you want to use the built-in 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 mouseClick override, rather than having a huge pile of case statements.
  • 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 write braces in JavaScript on the same line as the declaration, due to evil things like automatic semicolon insertion.
  • You should properly indent everything, including comments.
  • Don't use comments when your code is self-explanatory.

Other than that, nice work! :)

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ If my assumption is correct that random(1,6) generates a random decimal from 1 to 6, doesn't rounding actually lead to only 1/2 odds of 1 and 6, compared to the numbers in the middle? 1-1.5, 1.5-2.5, ... , 5.5-6. How about Ceiling (0-6)? \$\endgroup\$ – Kaz Dec 14 '15 at 19:55
  • \$\begingroup\$ @Zak Hmm. You have a good point there. I'm going to change it to floor and just add 1. \$\endgroup\$ – Ethan Bierlein Dec 14 '15 at 19:59
  • \$\begingroup\$ random(1,6) generates a float from 1 through 6, flooring and adding 1 gives you a value of 1 when? flooring and adding 1 only gives 1 when the random value is >=0 and < 1. ceiling(random(0, 6)) seems more correct, although it isn't clear to me from the documentation if there's a chance it could return exactly 0, in which case some strategy would need to be employed to avoid that. @janos answer is a good way to go about this, as well. \$\endgroup\$ – PeterL Dec 14 '15 at 23:29
  • 1
    \$\begingroup\$ @Jonah Noted. I will add some more information to my answer when I have time. \$\endgroup\$ – Ethan Bierlein Dec 15 '15 at 17:26
  • 1
    \$\begingroup\$ Much nicer! I'm Jonah, and I endorse this answer. \$\endgroup\$ – Jonah Dec 15 '15 at 19:59
4
\$\begingroup\$

Extract code out of the conditional branches

You always draw the containing rectangle, so you can move the code to draw it:

    fill(245, 242, 242);
    rect(100, 100, 200, 200);
    fill(0, 0, 0);

outside the if statements, just once at the top to avoid repetition.

Define a circle function

You are only drawing circles and everytime you call:

ellipse(a, b, c, c);

where the last two values are equal. A circle function makes the statement clearer and avoids repeating the same value twice so many times.

| improve this answer | |
\$\endgroup\$
4
\$\begingroup\$

The way you generate a random number seems odd, and biased. I believe this is closer to your real intention:

var num = Math.floor(Math.random() * 6) + 1;

Your chain of if conditions are mutually exclusive. Instead of multiple separate if, it would be better to connect them together with else if.

| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

One of element to improve code is to apply the DRY principle, that is Don't Repeat Yourself. In your code as pointed out by Caridorc you do repeat the clearing of the die face, which should be moved outside of the if statements.

But a die is very repetitive by nature, so there is more to extract. If you look at the die face for 2 and 3, the only difference is the middle dot, and when going to 4 and 5, you add two more dots. And finally for 6 you add yet another two dots in the middle. This can be formalised as shown here:

var LEFT_X = 150;
var MIDDLE_X = 200;
var RIGHT_X = 250;
var TOP_Y = 150;
var MIDDLE_Y = 200;
var BOTTOM_Y = 250;
var EYE_DIAMETER = 40;

var circle = function(x, y, diameter)
{
    ellipse(x, y, diameter, diameter);
};

mouseClicked = function()
{
    // change to change max and minimum numbers
    var number = floor(random() * 6) + 1;

    // creates background
    fill(255, 255, 255);
    rect(0, 0, 400, 400);

    fill(245, 242, 242);
    rect(100, 100, 200, 200);
    fill(0, 0, 0);
    text(number, 285, 285);

    // For all odd dices fill middle
    if(number % 2 === 1)
    {
        circle(MIDDLE_X, MIDDLE_Y, EYE_DIAMETER);
    }

    // From 2 and up fill upper left, and lower right
    if(number > 1)
    { 
        circle(LEFT_X, TOP_Y, EYE_DIAMETER);
        circle(RIGHT_X, BOTTOM_Y, EYE_DIAMETER);
    }

    // From 4 and up, fill upper right and lower left
    if(number > 3)
    {
        circle(RIGHT_X, TOP_Y, EYE_DIAMETER);
        circle(LEFT_X, BOTTOM_Y, EYE_DIAMETER);
    }
    // For 6 (and up?), fill middle left and right    
    if(number > 5)
    {
        circle(LEFT_X, MIDDLE_Y, EYE_DIAMETER);
        circle(RIGHT_X, MIDDLE_Y, EYE_DIAMETER);
    }
};

Also note that I've changed the random function to floor the random number to get an integer between 0 and 5 inclusive, which I then add 1 to get the proper 1 to 6 range. For debug purposes I've also added the die number as text in the lower right corner.

I've also introduced a circle function, and constants for the eye placement and diameter. This allows for easier change of the layout of the die face.

| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

The dice rolling functionality can be extracted to its own method:

var rollDice = function(diceX,diceY,diceSize,backgroundColor,foregroundColor)
    {
        ...
    };

Generating your random number can be cleaned up:

var number = round(random(0,5));

Each dot can be more generally thought of as a grid location, stored in a 1d array like this:

var dots = [
    [0,0], [0,1], [0,2],
    [1,0], [1,1], [1,2],
    [2,0], [2,1], [2,2]
];

The rolled number can then be translated into the required dots to be drawn via this map:

var numberToDotMap = [
    [4],
    [0,8],
    [0,4,8],
    [0,2,6,8],
    [0,2,4,6,8],
    [0,2,3,5,6,8]
];

Combining it all together, this is a more generic implementation:

var rollDice = function(diceX,diceY,diceSize,backgroundColor,foregroundColor)
{
    // change to change max and minimum numbers
    var number = round(random(0,5));

    var gridSize = diceSize/3;
    var dotSize = gridSize * 0.75;
    var rectSize = diceSize; 

    //every possible dot location on our grid
    var dots = [
        [0,0], [0,1], [0,2],
        [1,0], [1,1], [1,2],
        [2,0], [2,1], [2,2]
    ];

    //map to translate the given random number to an array of dots(grid coordinates) that should be drawn
    var numberToDotMap = [
        [4],
        [0,8],
        [0,4,8],
        [0,2,6,8],
        [0,2,4,6,8],
        [0,2,3,5,6,8]
    ];

    //draw background
    fill(backgroundColor.r, backgroundColor.g, backgroundColor.b);
    rect(diceX, diceY, diceSize, diceSize);

    //draw dots
    fill(foregroundColor.r, foregroundColor.g, foregroundColor.b);
    var whichDotsToDraw = numberToDotMap[number];
    for(var i=0; i<whichDotsToDraw.length; i++)
    {
        var dot = dots[whichDotsToDraw[i]];
        var dotX = dot[1] * gridSize + diceX + gridSize/2;
        var dotY = dot[0] * gridSize + diceY + gridSize/2;
        ellipse(dotX,dotY,dotSize,dotSize);
    }
};

mouseClicked = function()
{
    rollDice(100,100,200,
        {r:245, g:242, b:242},
        {r:0, g:0, b:0});
};
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Amazing there's only one correct answer on a code review site! \$\endgroup\$ – Fattie Dec 14 '15 at 23:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.