Dice Game by KankuroGB · Pull Request #180 · reactivepixel/Max-Bot

KankuroGB · 2018-07-23T03:18:51Z

Added commands to direct message the user a list of all supported dice in a well formatted embedded message and to roll a dice from the list of supported dice.

A future release will allow users to roll multiple dice and add in their ability perks.

gmoore10 · 2018-07-23T03:35:04Z

Hi Toby,

The listDice() method of the dice controller kicks back an UnhandledPromiseRejection as listDice() must return a string.

I would also recommend adjusting your __eslint.js file to include 'bot/**/*.js' as a path in order to test all of MaxBot's code. I made that adjustment and all tests did pass.

Thanks,

Garrett

gmoore10

Everything looks good on my end. Approved.

KankuroGB · 2018-07-23T03:49:07Z

In regards to the _eslint.js file. I don't think it needs to be drilled into like that as it was 'bot' previously before. I also just ran tests with the change you suggested and it didn't make any difference leading me to believe that having just 'bot' as a testing route means it just summarizes everything within that folder instead of outputting each file in the console like the test folder does.

reactivepixel · 2018-07-24T19:45:39Z

bot/controllers/dice.js

+    const { message } = this;
+    const dice = message.parsed[1].split(',');
+    util.log('Parsed message: ', dice, 4);
+    let results = 'You rolled a ';


Variable name semantically ambiguous

reactivepixel · 2018-07-24T19:47:15Z

bot/controllers/dice.js

+    // Decide what equation to run based on the dice type
+    switch (dice.toString()) {
+      case 'D4':
+        results += Math.floor((Math.random() * 4) + 1);


Duplicate logic: Lines 46, 49, 52, 55, 58
Magic numbers (same lines)

reactivepixel · 2018-07-24T19:47:50Z

bot/controllers/dice.js

+  // Perform the rolling of the dice
+  rollAction() {
+    const { message } = this;
+    const dice = message.parsed[1].split(',');


Magic Number

reactivepixel · 2018-07-24T19:49:00Z

bot/controllers/dice.js

+        break;
+      default:
+        // Return an error if user didn't input a supported die type
+        results = 'You rolled an unidentified die, please use `!listDice` to see all available dice!';


Tip: If the command changes this helper message will be invalid. Try to keep these instances to a min.

reactivepixel · 2018-07-24T19:49:33Z

bot/controllers/dice.js

+
+    // Send an embedded message to the user in their dms
+    message.member.send({ embed: {
+      color: 0x9d0a0e,


Magic (hex) number

reactivepixel · 2018-07-24T19:50:59Z

bot/controllers/dice.js

+    let diceString = 'Here is a list of all available dice:';
+
+    // Map the dice array and add it to the string to return
+    dice.map((val) => {


Poor semantic variable name.

reactivepixel · 2018-07-24T19:52:15Z

bot/controllers/dice.js

+  listDice() {
+    const { message } = this;
+    const dice = ['D4', 'D6', 'D8', 'D10', 'D12', 'D20']; // Dice array
+    let diceString = 'Here is a list of all available dice:';


Semantically poor variable name.

reactivepixel · 2018-07-24T19:58:31Z

bot/controllers/dice.js

+      color: 0x9d0a0e,
+      description: diceString,
+    } });
+    return 'Use this information wisely!';


Your current setup sends two dms when only 1 is needed. Additionally, you have created a race condition with your other message.member.send(). Would be better to see the color embed happen on the base controller and you can configure that from here, so that other controllers can benefit from this logic.

reactivepixel · 2018-07-24T20:01:14Z

bot/controllers/dice.js

+  // List all the dice
+  listDice() {
+    const { message } = this;
+    const dice = ['D4', 'D6', 'D8', 'D10', 'D12', 'D20']; // Dice array


Why not support a D100? or a D3? You could more elegantly handle this by parsing the provided input and interpret the number following the D as the number you intend to mathematically use... skipping the magic number aspect and dynamical supporting what ever number your user wants rather than limiting the user to the narrow choices you have provided.

…tiple rolls of different dice. Removed limited dice types. Removed listDice command as it is no longer needed.

gmoore10 · 2018-07-27T21:37:58Z

bot/controllers/dice.js

+
+    diceCommand.map((singleDie) => {
+      // Get the specifics of the roll based on the location of the 'd'
+      let rollCount = singleDie.toLowerCase().substr(0, singleDie.indexOf('d'));


This line is causing an issue that is crashing the bot when an unexpected input is found.

Because of the way this line is written, it is looking for an indexOf on a lowercase d... however, when I pass an uppercase D (or any other letter that isn't a D for that matter), the indexOf returns an unexpected value which turns the rollCount into a value that is not a number.

Because of this, line 44 (rollCount -= 1;) may not behave as expected and cause the while loop to loop endlessly until the bot runs out of memory.

I think this line should be:

let rollCount = singleDie.toLowerCase().substr(0, singleDie.toLowerCase.indexOf('d'));

However, this will not completely fix the issue. If I put in something other than an uppercase D or lowercase d, the bot will still crash.

You might need to implement some sort of check on the indexOf() to make sure the value is within range. If not, it should return an error indicating the problem with the input.

gmoore10 · 2018-07-27T21:38:52Z

Hi Toby,

I found an issue with the logic in the !roll function. Please see my notes above.

Thanks,

Garrett

Added conditional to check for specific formatting. Added error output if input wasn’t correct.

KankuroGB · 2018-07-27T22:27:40Z

I've fixed the error outlined above as well as added a conditional and error for when the user inputs an invalid dice format.

gmoore10 · 2018-07-27T22:34:24Z

Good work fixing the issue with the lowercase/uppercase d. I did still manage to crash the bot by running !roll 2SD9 though. It might be worth researching a good regex pattern to cover all possible mistakes.

Edited command description to be valid. Edited error message to be valid.

KankuroGB · 2018-07-27T23:22:50Z

Thanks for the feedback! The roll command now has regex to check for the proper format as well an extra conditional to verify the beginning of the command is accurate.

The description and error messages have also been updated to account for support of !roll D20 now being interpreted the same as !roll 1d20 thanks to the regex.

gmoore10

Everything looks good on my end. Any die size and any number of rolls works correctly. Regex pattern correctly checks for the appropriate #D# format.

reactivepixel · 2018-07-28T19:22:11Z

bot/controllers/dice.js

+  // Perform the rolling of the dice
+  rollAction() {
+    const { message } = this;
+    const diceCommand = message.parsed[1].split(',');


Magic Number (1) and Semantically diceCommand should be plural.

Fixed these issues. For the magic number, this is present in all the other files as well, which is the only reason I assumed this instance of it was okay.

Added roll and listDice commands.

9b1483f

KankuroGB requested a review from gmoore10 July 23, 2018 03:19

Fixed UnhandledPromiseRejectionWarning

1deca52

gmoore10 approved these changes Jul 23, 2018

View reviewed changes

reactivepixel reviewed Jul 24, 2018

View reviewed changes

Added support for multiple dice rolls of the same type as well as mul…

9572acb

…tiple rolls of different dice. Removed limited dice types. Removed listDice command as it is no longer needed.

KankuroGB requested a review from gmoore10 July 27, 2018 19:06

gmoore10 reviewed Jul 27, 2018

View reviewed changes

Fixed out of memory error.

a7dbb19

Added conditional to check for specific formatting. Added error output if input wasn’t correct.

Added regex and an additional conditional for dice format checking.

4b4e673

Edited command description to be valid. Edited error message to be valid.

gmoore10 self-requested a review July 28, 2018 02:22

gmoore10 approved these changes Jul 28, 2018

View reviewed changes

reactivepixel reviewed Jul 28, 2018

View reviewed changes

Fixed magic number and semantic naming

546600a

reactivepixel closed this Aug 21, 2019

reactivepixel deleted the HornToby_Roll branch August 21, 2019 17:24

Conversation

KankuroGB commented Jul 23, 2018

Uh oh!

gmoore10 commented Jul 23, 2018

Uh oh!

gmoore10 left a comment

Choose a reason for hiding this comment

Uh oh!

KankuroGB commented Jul 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reactivepixel Jul 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmoore10 commented Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KankuroGB commented Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmoore10 commented Jul 27, 2018

Uh oh!

KankuroGB commented Jul 27, 2018

Uh oh!

gmoore10 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

reactivepixel Jul 24, 2018 •

edited

Loading

gmoore10 commented Jul 27, 2018 •

edited

Loading

KankuroGB commented Jul 27, 2018 •

edited

Loading