HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

A dice roller in node.js

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
noderollerdice

Problem

I knocked out a dice roller in 2 days. I think this is the first coding project I've ever actually completed to a degree where I'm not embarrassed to post it.

This is my first time working with objects, sets, and recursion. It took a lot of trial and error and it's not the most stable (bad syntax can crash it), but I'm pretty proud.

When the bot hears roll in chat, it parses the syntax of the the message, rolls the dice, applies some special rules (pertinent to the Exalted RPG system), and responds back to chat with formatted text.

I'd love to hear how experts could improve my code, with the proviso that I'm new to coding and this was my first time using objects, sets, and recursion so my vocabulary and comprehension are still improving.

```
var Discord = require("discord.js");
var mybot = new Discord.Client();
credentials = require("./token.js");
mybot.loginWithToken(credentials.token);

// Look for messages starting with roll
// To-do: change to .roll
mybot.on("message", function(message) {
if (message.content.startsWith("roll")) {
mybot.reply(message, parseMessage(message));
}
});

//
// SYNTAX GUIDE:
// Handle: target number, double successes (single and #+),
// rerolls (single and cascading), autosuccess
//
// .roll/tn6/
// tn: single target number, values >= to this will count as a success. Default: 7
// db: double x's. 7 double's 7 only, 7+ is 7 and up. Default: 10
// re: reroll #
// as: adds a flat number of successes
//

function Roll(numDice) {
var roll = function(numDice) {
var rolls = [];
for (var i = 0; i = theRoll.target && theRoll.doubleSet.has(theRoll.rolls[i]) && theRoll.rerollSet.has(theRoll.rolls[i])) {
successes+=2;
theRoll.rolls[i] = "~~__"+theRoll.rolls[i]+"__~~";
} else if (theRoll.rolls[i] >= theRoll.target && theRoll.doubleSet.has(theRoll.rolls[i])) {
successes+=2;
theRoll.rolls[i] = "__"+theRoll.rolls[i]+"__";
} else if (theRol

Solution

You might want to experiment instead of Roll as the main object entity, try Dice. Generally speaking, in object oriented programming, we want to have the class that preferably maps to a specific object or in another word, prefer nouns for our classes over verbs. I do realize Roll can also be a noun but one can argue that it's just easily an action being taken place.

The reason why I suggest this is that this will likely help organize your functions and think about your objects more concretely. For example, if I'm thinking in term of a "Dice", what can I configure a dice with? Such as.. how many sides of a dice do I want? maybe, what are the faces of this dice? Another question would be, what type of actions can I do with a single dice? In your case, primarily we can "roll" a dice. Methods in a class is more intuitive with verbs or actions that the object can take. Also, our dice object can have interactions with other objects in the future, maybe we eventually want to build our chatbot to allow playing crabs or monopoly. We should theoretically be able to use the same Dice object as long as we keep its responsibilities very specific to what a dice can do.

Here is an example of how you may want to rearrange and hopefully, we will gain clarity and reusability:

/* I encourage using ES6 classes, by the way you should prefer function
   expressions over function declarations as there are hoisting properties
   that may catch you by surprise and honestly it's rare to see.
*/

class Dice {
    constructor(numOfFaces) {
        this.numOfFaces = numOfFaces;
        this.faceUpValue = null;
    }

    roll() {
        this.faceUpValue = Math.floor(Math.random() * this.numOfFaces + 1);
        return this.faceUpValue; // this is arbitrary, I prefer this method to not return face up value;
    }

    value() {
        return this.faceUpValue;
    }
}

// I'm going to introduce this Roll class just to show how we may organize this code.
class Roll {
    constructor(numOfDice) {
        this.numOfDice = numOfDice;
        this.dices = [];
        const numOfFaces = 10;
        for(let i = 0; i  {
            dice.roll();
        }
    }

    getDiceValues() {
        this.dices.map((dice) => {
            return dice.value();
        }
    }
}

// sample code, putting it all together
const numOfDices = 6;
const roller = new Roll(numOfDices);

roller.doIt();
console.log(roller.getDiceValues());

// reroll
roller.doIt();
console.log(roller.getDiceValues());


This illustration mostly just show how we can improve clarity and help us figure out where our code might go. In your original snippet, there is a lot functions just living on the global scope, we want to generally try to avoid those as much as possible. Generic functions generally are bad organization as it doesn't give any context to the function and the pattern definitely won't scale as we add more functionalities. Sometimes the trick is just figure out what new entity to create and code will often naturally find its place to where it belongs.

Code Snippets

/* I encourage using ES6 classes, by the way you should prefer function
   expressions over function declarations as there are hoisting properties
   that may catch you by surprise and honestly it's rare to see.
*/

class Dice {
    constructor(numOfFaces) {
        this.numOfFaces = numOfFaces;
        this.faceUpValue = null;
    }

    roll() {
        this.faceUpValue = Math.floor(Math.random() * this.numOfFaces + 1);
        return this.faceUpValue; // this is arbitrary, I prefer this method to not return face up value;
    }

    value() {
        return this.faceUpValue;
    }
}

// I'm going to introduce this Roll class just to show how we may organize this code.
class Roll {
    constructor(numOfDice) {
        this.numOfDice = numOfDice;
        this.dices = [];
        const numOfFaces = 10;
        for(let i = 0; i < numOfDice; i++) {
            // notice that we can create dices with different faces if we wanted
            const newDice = new Dice(numOfFaces);
            this.dices.push(newDice);
        }
    }

    doIt() {
        this.dices.forEach((dice) => {
            dice.roll();
        }
    }

    getDiceValues() {
        this.dices.map((dice) => {
            return dice.value();
        }
    }
}

// sample code, putting it all together
const numOfDices = 6;
const roller = new Roll(numOfDices);

roller.doIt();
console.log(roller.getDiceValues());

// reroll
roller.doIt();
console.log(roller.getDiceValues());

Context

StackExchange Code Review Q#136123, answer score: 4

Revisions (0)

No revisions yet.