Issue 18 #50

Cadesh · 2019-11-24T22:42:53Z

[REFACTOR] returnHeightAndWeight()

Description of pull request

Util.js

Added a dice throw function that receives the quantity and sides of dice.

Core.js

Added function to calculate character height;
Added function to calculate character weight;
Deleted repeated function calls on each of character race (applySubraceBonuses) and replaced for a single call inside function NewCharacter;
Deleted old functions in core.js: returnCharacterHeight() and returnHeightAndWeight(height, weight);
Added test/core.test.js with methods to test character weight. This is to be used with JEST (check instructions bellow)

Resolves #18

Has it been tested?

Yes
No
Testing not needed

Cadesh · 2019-11-24T22:56:33Z

Hello @KingFruit85, here is a proposal for the refactoring of Weight and Height calculation.

I also added some JEST tests to validate Weight in the folder test. To run the tests you must add to your package.json the following fragments:

  "scripts": {
    "test": "jest"
  },

  "devDependencies": {
    "jest": "^24.9.0"
  }

Then run npm install. After the installation type npm test and the tests in folder will be executed with an output like this:

 PASS  test/core.test.js
  √ dragonborn weight in range (3ms)
  √ dwarf weight in range (1ms)
  √ elf weight in range
  √ gnome weight in range
  √ half-elf weight in range (1ms)
  √ half-orc weight in range
  √ human weight in range
  √ halfling weight in range (1ms)
  √ tiefling weight in range

Test Suites: 1 passed, 1 total
Tests:       9 passed, 9 total
Snapshots:   0 total
Time:        2.666s
Ran all test suites.

If you like we can add Travis to run this automatically and then add a nice badge to README like this one:

KingFruit85

Hey @Cadesh I really appreciate you contributing to this project :)

I have reviewed and requested some changes. Please let me know if you need any more info.

KingFruit85 · 2019-11-25T19:49:57Z

utils.js

+ * @param {Number} diceSides - integer for the number of sides 2, 4, 6, 8, 10, 12, 20, ...
+ * @returns {Number} integer with the result of dice throw.  
+*///------------------------------------------------------------------------------
+function diceThrow (diceQuantity = 1, diceSides) {


What is the reason for diceQuantity = 1, does it provide any benefit over just diceQuantity?

This is just a default value, actually it would be better if it was

function diceThrow (diceSides, diceQuantity = 1 )

this way you could call the function as diceThrow(8) and get a return of 1d8

KingFruit85 · 2019-11-25T20:14:44Z

test/core.test.js

+test('tiefling weight in range', () => {
+    expect(calculateCharacterWeight('tiefling')).toBeGreaterThanOrEqual(114);
+    expect(calculateCharacterWeight('tiefling')).toBeLessThanOrEqual(238);
+});


These are great, nice to see how the tests work :) however I think they are going to require a bit of tweaking.

Using a Dwarf as an example:

The max weight of 211 is never going to be reached, even if both d6's roll max: 115 + 12 = 127

The minimum weight can also fail if the result of the 2 x d6 rolls add up to less than 3 (1 - 1 / 2 - 1).

The issue is with the way you have calculated the characters weight, and to be fair the table you linked is quite misleading! I'll leave a comment on the calculateCharacterWeight() function that explains why these numbers don't match up.

This is a doubt I had when checking the table here

It says the weight range, these are the numbers I used in the tests. I noticed that there were something wrong with the calculations but I did not want to change your original calculations. If you know the correct ones please send me a link and I will update the pull request.

KingFruit85 · 2019-11-25T20:39:14Z

core.js

+ * @param {String} race - the character race.
+ * @returns {Number} the calculated weight in lbs.  
+*///------------------------------------------------------------------------------
+function calculateCharacterWeight(race) {


So the table you used as a source didn't explain how the weight figure is calculated, and my previous code was also incorrect so I wasn't helping you out there either!

While a player can just roll or pick a random number, the method described in the players hand book to calculate height and weight is as follows:

First step is to calculate the players height, using a hill dwarf as an example:

Base Height: 3'8"
Height Modifier: 2d4

Base Weight: 115lb
Weight Modifier: (result of Height Modifier) x 2d6

So if I rolled a height modifier of 6 I would calculate the players weight as 115 + (6 x 2d6)

This would also explain the weight ranges described in your source table, rolling minimum values on both height modifier and weight modifier would result in : 115 + (2x2) = 119

And max values would be 115 + (8x12) = 211

hopefully that makes sense 👍

Oh, great I will fix it and then send a new commit to this PR.

KingFruit85 · 2019-11-25T20:49:08Z

core.js

+      break;
+      default: return "invalid race provided as function arguement [returnCharacterHeight()]";
+  }
+  return finalHeight;


I have just tested this and in some instances the finalHeight isn't fixed to 1 decimal place

I assume the total result of the addition of the base height and the randomized amount needs to be set to .toFixed(1)

Yes, this is correct, I will also add this change to the next PR

Cadesh and others added 2 commits November 24, 2019 18:12

FIX #18: fixed conflicts

b659fb7

Fix #18: [REFACTOR] returnHeightAndWeight

c2c3655

KingFruit85 requested changes Nov 25, 2019

View reviewed changes

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 18 #50

Issue 18 #50

Cadesh commented Nov 24, 2019 •

edited

Loading

Cadesh commented Nov 24, 2019

KingFruit85 left a comment •

edited

Loading

KingFruit85 Nov 25, 2019

Cadesh Nov 30, 2019

KingFruit85 Nov 25, 2019

Cadesh Nov 30, 2019

KingFruit85 Nov 25, 2019 •

edited

Loading

Cadesh Nov 30, 2019

KingFruit85 Nov 25, 2019

Cadesh Nov 30, 2019

Issue 18 #50

Issue 18 #50

Conversation

Cadesh commented Nov 24, 2019 • edited Loading

[REFACTOR] returnHeightAndWeight()

Description of pull request

Resolves #18

Has it been tested?

Cadesh commented Nov 24, 2019

KingFruit85 left a comment • edited Loading

Choose a reason for hiding this comment

KingFruit85 Nov 25, 2019

Choose a reason for hiding this comment

Cadesh Nov 30, 2019

Choose a reason for hiding this comment

KingFruit85 Nov 25, 2019

Choose a reason for hiding this comment

Cadesh Nov 30, 2019

Choose a reason for hiding this comment

KingFruit85 Nov 25, 2019 • edited Loading

Choose a reason for hiding this comment

Cadesh Nov 30, 2019

Choose a reason for hiding this comment

KingFruit85 Nov 25, 2019

Choose a reason for hiding this comment

Cadesh Nov 30, 2019

Choose a reason for hiding this comment

Cadesh commented Nov 24, 2019 •

edited

Loading

KingFruit85 left a comment •

edited

Loading

KingFruit85 Nov 25, 2019 •

edited

Loading