Convert Entity stats to a Hash #103

[Jeff-Hostetler]

I think it might be worth putting the stats into their own class after doing this. Seeing entity.stats.hp seems cleaner to me than entity.stats[:hp]. Let me know what you think about that.

Also I could be talked out of my alter_stats implementation for something more readable like;

def alter_stats(entity, equipping)

      # Alter the stats as appropriate.
      stats = entity.stats
      equipment_stats = [:attack, :defense, :agility]
      if equipping
        equipment_stats.each {|stat| stats[stat] += stat_change[stat] if stat_change[stat]}
      else
        equipment_stats.each {|stat| stats[stat] -= stat_change[stat] if stat_change[stat]}
      end

    end

[coveralls]

Coverage remained the same at 100.0% when pulling 3760d61082fde08912fb963875e5228bac54b273 on Jeff-Hostetler:entity-stats into e5bfeac50af2bb47160f6523b79f50fd46987645 on nskins:entity-stats.

[nskins]

@Jeff-Hostetler Looking good! I want to keep it as a Hash for now, but I'll think about your suggestion. #alter_stats is fine; no need to change it. Here are a few more additions to make:

• [x] Include documentation for the new private functions you've added.
• [x] Include max_hp in equipment_stats so that max_hp can also be altered by equipment.
• [x] Update the trivial case (in equippable_spec.rb) to use max_hp.
• [x] Write a test (in equippable_spec.rb) for the following situation: an Entity (with hp = 3, max_hp = 5) unequips an Equippable, decreasing max_hp by 4 (The Equippable has stat_change[:max_hp] = 4). The Entity should then have hp = 1, max_hp = 1.

Can you explain why stats in the Entity's constructor is an empty Hash, while stats in Player and Monster constructors use the default stats? I see your implementation of #set_stats already has the default stats. I think the constructors should be more consistent.

Also, could you write #set_stats so that it only overwrites only the stats that are passed? Consider this situation: an Entity could have attack = 5 and defense = 3. Then the following:

set_stats({ attack: 7})

should make the Entity have attack = 7 and defense = 3. Unless I'm mistaken, the current implementation appears to make defense = 1 in this case. Could you verify it will have the correct behavior?

Thank you! Let me know if you have questions.

[Jeff-Hostetler]

I'll take a look tonight. Do you prefer I just make another commit with fixes or do a new with on one commit

On May 24, 2017 11:30 AM, "Nicholas Skinsacos" notifications@github.com wrote:

@Jeff-Hostetler https://github.com/jeff-hostetler Looking good! I want to keep it as a Hash for now, but I'll think about your suggestion.

alter_stats is fine; no need to change it. Here are a few more additions

to make:

• Include max_hp in equipment_stats so that max_hp can also be altered by equipment.
• Update the trivial case (in equippable_spec.rb) to use max_hp.
• Write a test (in equippable_spec.rb) for the following situation: an Entity (with hp = 3, max_hp = 5) unequips an Equippable, decreasing max_hp by 4 (The Equippable has stat_change[:max_hp] = 4). The Entity should then have hp = 1, max_hp = 1.

Can you explain why stats in the Entity's constructor is an empty Hash, while stats in Player and Monster constructors use the default stats? I see your implementation of #set_stats already has the default stats. I think the constructors should be more consistent.

Also, could you write #set_stats so that it only overwrites only the stats that are passed? Consider this situation: an Entity could have attack = 5 and defense = 3. Then the following:

set_stats({ attack: 7})

should make the Entity have attack = 7 and defense = 3. Unless I'm mistaken, the current implementation appears to make defense = 1 in this case. Could you verify it will have the correct behavior?

Thank you! Let me know if you have questions.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nskins/goby/pull/104#issuecomment-303795385, or mute the thread https://github.com/notifications/unsubscribe-auth/AIjxuEkZcQqv0CPNy43X1j85kOqU5eRRks5r9GlOgaJpZM4NkiG2 .

[nskins]

Another commit is fine.

[Jeff-Hostetler]

@nskins For the new Equippable behavior should all of the stats ([:attack, :defense, :agility, :max_hp, :hp]) not be able to go below 1? If the Entity equips, does just the max_hp go up and hp stays where it is?

For the set_stats issue, the only way you could get into that situation is that you call set_stats on an instance of an Entity. The method is private and only called on the initialization so I don't see the point in covering that base unless you are wanting to protect from someone using send.

[nskins]

For the new Equippable behavior should all of the stats ([:attack, :defense, :agility, :max_hp, :hp]) not be able to go below 1?

Yes, and you should include that check in #set_stats (see below).

If the Entity equips, does just the max_hp go up and hp stays where it is?

Yes.

For the set_stats issue, the only way you could get into that situation is that you call set_stats on an instance of an Entity. The method is private and only called on the initialization so I don't see the point in covering a that base unless you are wanting to protect from someone using send.

Ah! Here's what to do: make #set_stats a public function. Then, for stats, change from attr_accessor to attr_reader. This way, we have a safe setter function (good practice in OOP) so the user can't change stats to an arbitrary value. The user will have to use #set_stats.

That being said, could you also include some tests for #set_stats to ensure it has all the behavior we've discussed?

[Jeff-Hostetler]

Okay, I like that.

[coveralls]

Coverage remained the same at 100.0% when pulling 955934c3b397658bcfc902f14adccae5707a6512 on Jeff-Hostetler:entity-stats into e5bfeac50af2bb47160f6523b79f50fd46987645 on nskins:entity-stats.

[Jeff-Hostetler]

As far as stats go, I ended up making a getter so that we can ensure that []= is not accessible to the hash. I added some specs around #set_stats and some other functionality to catch when nonsense values are passed in.

[nskins]

Nice job! Here are some minor errors to fix:

• [x] entity.rb, line 309, should include the variable name you are referencing after its type (see surrounding examples).
• [x] entity.rb, line 325, should read '> 0' instead of '>= 0'.
• [x] Write documentation for #stats (entity.rb, line 335). Just say it's a getter for stats - nothing fancy.
• [x] Write the above test from my first comment (fourth checkbox).
• [x] Change the error in #set_stats so that it raises an exception instead of printing text. The printed errors are used for in-game errors (like the player trying use a non-existent item), whereas programming errors should throw exceptions.

Thanks.

[coveralls]

Coverage remained the same at 100.0% when pulling 8049a03ae0e89f7a88e2c2f73ef21e451081eae9 on Jeff-Hostetler:entity-stats into e5bfeac50af2bb47160f6523b79f50fd46987645 on nskins:entity-stats.

[nskins]

I apologize if my explanation was not too good, but I was thinking the test would look something like this:

it "automatically decreases hp to match max_hp" do
      @entity.set_stats({ max_hp: 3, hp: 2 })
      @equippable.alter_stats(@entity, false)
      expect(@entity.stats[:max_hp]).to eq 1
      expect(@entity.stats[:hp]).to eq 1
    end

(Since @equippable has stat_change[:max_hp] = 2, I see now these initial stats for @entity work better.) I've included this test on your branch, but it's failing and saying that @entity.stats[:max_hp] = 2; it should be 1. Additionally, @entity.stats[:hp] = 2, but it should also be 1.

I'm not 100% sure what the test you just wrote actually checks. Ensure that entity.stats[:hp] is modified by #alter_stats if and only if the stat_change would cause @entity.stats[:max_hp] to become less than @entity.stats[:hp]. In this case, set @entity.stats[:hp] = @entity.stats[:max_hp].

After a little more poking around, it looks like this might be a bug in #set_stats. I don't want the behavior you've included on line 632 of entity_spec.rb. I want that call to #set_stats to cause entity.stats[:hp] = entity.stats[:max_hp] = 2; not 3! Start by fixing this test.

Sorry to be picky, but I need to make sure we fix this bug before the merge. Please let me know if there's any confusion.

[Jeff-Hostetler]

The test was checking that you can't get to 0 hp by unequipping (killing the entity). The line on 632 was to preserve behavior that is on master on line 30 in entity.rb

# Prevent HP > max HP.
@max_hp = @hp if @hp > @max_hp

but I have removed along with the spec and the rest of the suite is green. If I write

    it "hp cannot be more than max hp" do
      entity = Entity.new

      entity.set_stats({ max_hp: 2, hp: 3 })

      stats = entity.stats
      expect(stats[:max_hp]).to eq 2
      expect(stats[:hp]).to eq 2
    end

that fails as stats_hp is 3. I am assuming that should pass and I should make a change in set_stats to set hp to the min of max_hp and hp?

[coveralls]

Coverage remained the same at 100.0% when pulling 3c673cc5bb9e4ad0783b6e3b67e3419d00e4030a on Jeff-Hostetler:entity-stats into e5bfeac50af2bb47160f6523b79f50fd46987645 on nskins:entity-stats.

[nskins]

Okay - I see this was my fault. There was a bug I hadn't noticed on master. That line should read:

# Prevent HP > max HP.
@hp = @max_hp if @hp > @max_hp

Sorry about that!

I am assuming that should pass and I should make a change in set_stats to set hp to the min of max_hp and hp?

Yes, that sounds correct.

[Jeff-Hostetler]

No worries at all. That makes more sense.

[nskins]

Looks great! Merging to entity-stats now... will make a few minor changes and then merge to master. Thanks a lot, and please consider contributing again sometime.