Add 'reward' property to Unit class #67
Conversation
packages/warriorjs-core/src/Unit.js
Outdated
| @@ -157,7 +159,7 @@ class Unit { | |||
| damage(receiver, amount) { | |||
| receiver.takeDamage(amount); | |||
| if (!receiver.isAlive()) { | |||
| this.earnPoints(receiver.maxHealth); | |||
| this.earnPoints(receiver.reward || receiver.maxHealth); | |||
There was a problem hiding this comment.
@RascalTwo I prefer we make this check in the constructor, and here just award receiver.reward. Also, there could be scenarios where we want the reward to be 0, which evaluates to false, so we may need to make this check more explicit (like reward === null ? maxHealth : reward).
packages/warriorjs-core/src/Unit.js
Outdated
| this.name = name; | ||
| this.character = character; | ||
| this.maxHealth = maxHealth; | ||
| this.reward = reward; |
There was a problem hiding this comment.
@RascalTwo Let's check for null here instead.
packages/warriorjs-core/src/Unit.js
Outdated
| */ | ||
| constructor(name, character, maxHealth, captive = false) { | ||
| constructor(name, character, maxHealth, captive = false, reward = null) { |
There was a problem hiding this comment.
@RascalTwo If you put the reward parameter before captive it could make the tests easier because you won't have to worry about passing a value for captive, see comment in the test.
| unit.earnPoints = jest.fn(); | ||
| unit.damage(receiver, 5); | ||
| expect(unit.earnPoints).toHaveBeenCalledWith(5); | ||
| }); | ||
|
|
||
| test('earns points equal to reward when killing unit with reward', () => { |
There was a problem hiding this comment.
@RascalTwo Assigning the reward to max health in the constructor makes this test obsolete. Instead, we should pass a reward when instantiating the unit for tests, and add a test to check that reward is assigned to max health when omitted.
|
Thanks for the comments! So I made the changes you requested, believe I got everything as you intended. |
|
@RascalTwo Thanks for your contribution, and congratulations for being the first contributor in this new stage of the project! It makes me very happy 😄 Would you mind pushing your changes again? I just realized I had the option to build forks disabled in CircleCI. |
This allows Units to reward a set amount of points when killed instead of their max health.
Codecov Report
@@ Coverage Diff @@
## master #67 +/- ##
==========================================
+ Coverage 86.02% 86.03% +0.01%
==========================================
Files 80 80
Lines 1016 1017 +1
Branches 153 154 +1
==========================================
+ Hits 874 875 +1
Misses 127 127
Partials 15 15
Continue to review full report at Codecov.
|
|
You're welcome, and thank you for taking to time to make those comments, I'm just happy to contribute. It's quite a fun game, lots of potential. |
|
Just for the record, I'm open to ideas on how to make the game better or more fun 😄. Most of it I think comes from the available levels, abilities, units, etc., so being able to make new ones as plugins for the game is a big plus. I'm writing the docs for Tower makers now. This PR you just contributed with gives more freedom to makers, which is great! |
olistic
changed the title
This allows Units to reward a set amount of points when killed instead of their max health.
Along with the new test, I tested it in an actual level in which I give a unit a reward, and it works as expected.