[vJass] Blood Sorceress Spellpack

[Moderator]

15:30, 21th June 2015
IcemanBo: https://www.hiveworkshop.com/threads/vjass-blood-sorceress-spellpack.265292/post-2697185

 

[jonbon29]

You should post the triggers, spell description and required libraries if any. I haven't tested the pack yet, I'll try it later though I think they're good.

Edit: the pack seems great, I like the spells in it.

 

[Reventhous]

sorry about that, slow internet connection here. I'll update.

 

[IcemanBo]

- Add a proper spell description.
- Add documentation. At least for user in config part it's essential.

private function Conditions takes nothing returns boolean
    if ( not ( GetSpellAbilityId() == SPELL_ID ) ) then
        return false
    endif
    return true
endfunction

^We're not in GUI here. You can make it more efficient. You could directly return the expression.
But anyway you can combine action and condition into condition.
Just make a normal if statement to filter the correct ability.
- Don't use PauseUnit operation.
- The BJs can be avoided.
- One global group can be used instead of dynamicly create and destroy locals.
- Keep working in RAD in Jass. Deg is only needed for unit's facing.
- Local agents need to be nulled. Read the reference part: https://www.hiveworkshop.com/threads/memory-leaks.263410/post-2661133
- I haven't tested the spells yet, but I recomment you to make the cinematic operation optional. It's not MUI.
- Variable names and function names should be descriptive.
- v0.01 intends to be not a finished version. Change it next time when you upload.

Enough for next update.

 

[Cokemonkey11]

Never use hashtable without an abstraction.

 

[Reventhous]

Wow, that a lot. I should remake the code.

 

[Reventhous]

Updated! I remade Freezing Wave code

 

[BPower]

Everything which is shinning red in your code, ( check your post ), should
be replaced by a native. Don't use locations in vJass, but rather coordinates x/y.

 

[Reventhous]

How to replace TriggerRegisterAnyUnitEventBJ? and why I can't use BJs?

 

[BPower]

How to replace TriggerRegisterAnyUnitEventBJ?

This one can stay, it's actually good.

and why I can't use BJs?

Because they lack efficiency, basically they are wrapper function to blizzard natives.

 

[IcemanBo]

You have not changed all mentioned points from my first feedback.

Furthermore,

• The configs need to be documented.
• == true and == false can be avoided.
  Just remove the true check, and instead of the false check
  you can place a not in front of the expression.
• call GroupRemoveUnit(grp, enm) can be moved out of the if-statement.
• OwnerOfCaster could be initialisized as member of an instance to reduce function calls.
• Use natives over BJs.
• Use functions to calculate damage, not simple a constant. In JASS you can use that neat feature to have more power in modifying.
• Implement native UnitAlive into your code to check if a unit is dead, or check (if UnitTypeId equals 0, or unit type is UNIT_TYPE_DEAD).
• Locations in your code are not needed. Work around.
• Names like "HS" are not descriptive. Sometimes a bit longer names are better
  to gain readability. Please change them.
  Also names your struct members with only 1/2 letters need to be changed.
  Names should be descriptive if possible.
• I suggets using TimerUtils. It can attach an instance to a timer, and recycle timers.
  It would be helpful in your case, because then you also can remove your hashtable. (and don't need to think about using Table)
• Set/Get a unit's life needs to be done via Get/Set WidgetLife natives. It's faster.

There is pretty much to change, also again, please read again my previous post,
because there are still some critical points that needs to be changed.