Changes between Initial Version and Version 1 of Ticket #999, comment 21


Ignore:
Timestamp:
Mar 18, 2012, 9:44:57 PM (12 years ago)
Author:
historic_bruno

Legend:

Unmodified
Added
Removed
Modified

  • Ticket #999, comment 21

    initial v1  
    11Few comments from reviewing the patch:
    22 * `Garrison.js`: Are we sure that "healing" is the same concept for garrisoning and healers? I've never thought of it that way, but maybe it's logical.
    3  * `Heal.js`: When I see `HP` I think of the unit's max hitpoints, would another name be more suitable, like `HealthIncrease` or `HPIncrease`? Garrison also has some confusing element names that we should clean up some day.
     3 * `Heal.js`: When I see `HP` I think of the unit's max hitpoints, would another name be more suitable, like `HealthIncrease` or `HPIncreasePerHeal`? Garrison also has some confusing element names that we should clean up some day.
    44 * `Heal.GetUnhealableClasses` seems to be confusing the return types, sometimes it will return an array but if the classes are not defined it returns an empty string. I think you should return an empty array `[]` instead (or `undefined` but then you'd have to check for that everywhere).
    5  * What if we combined this logic into `Health.IsUnhealable`:
     5 * What if we combined all the healability logic into `Health.IsUnhealable`:
    66  {{{
    77this.template.Unhealable == "true" || this.GetHitpoints() <= 0 || this.GetHitpoints() >= this.GetMaxHitpoints()