Criticism needed

07-13-2009, 10:06 AM

So... I'm coding a bit in vJass in this period and I'm quite happy with the results of my spells, but I'm pretty sure that I'm always missing something in order to improve the code. (fix leaks etc.)

Tell me how can I improve this spell:

EDIT: MUIed

Aura of Decay:

scope AuraofDecay initializer Init

globals
    private constant integer HeroId = 'H001'
    private constant integer Aura = 'A00C'
    private constant integer AuraStatIcon = 'A00A'
    private constant integer PosBuff = 'A00B'
    private constant integer NegBuff = 'A005'

    private unit array Hero
    private integer Heroes = 0
    private unit H = null
    private player UP = null
    private group ENUM
endglobals

private function EnemyDeathKnights takes nothing returns boolean
return (IsUnitInGroup(GetFilterUnit(), GetUnitsOfTypeIdAll(HeroId))) and (IsUnitEnemy(GetFilterUnit(), UP)) and (GetUnitAbilityLevel(GetFilterUnit(), Aura) != 0)
endfunction

private function AlliedDeathKnights takes nothing returns boolean
return (IsUnitInGroup(GetFilterUnit(), GetUnitsOfTypeIdAll(HeroId))) and (IsUnitAlly(GetFilterUnit(), UP)) and (GetUnitAbilityLevel(GetFilterUnit(), Aura) != 0)
endfunction

private function BuffRemove takes nothing returns boolean
set UP = GetOwningPlayer(GetFilterUnit())
if(GetUnitAbilityLevel(GetFilterUnit(), NegBuff) != 0) then
    call GroupEnumUnitsInRangeCounted(ENUM, GetUnitX(GetFilterUnit()), GetUnitY(GetFilterUnit()), 900.00, Condition(function EnemyDeathKnights), 1)
    if (CountUnitsInGroup(ENUM) == 0) then
        call UnitRemoveAbility(GetFilterUnit(), NegBuff)
    endif
    set ENUM = null
endif
if (GetUnitAbilityLevel(GetFilterUnit(), PosBuff) != 0) then
    call GroupEnumUnitsInRangeCounted(ENUM, GetUnitX(GetFilterUnit()), GetUnitY(GetFilterUnit()), 900.00, Condition(function AlliedDeathKnights), 1)
    if (CountUnitsInGroup(ENUM) == 0) then
        call UnitRemoveAbility(GetFilterUnit(), PosBuff)
    endif
    set ENUM = null
endif
return false
endfunction

private function Group takes nothing returns boolean
local unit u = GetFilterUnit()
if (IsUnitEnemy(u, GetOwningPlayer(H))) and (GetWidgetLife(u) > 0.405) and (IsUnitType(u, UNIT_TYPE_MAGIC_IMMUNE) == false) and (IsUnitType(u, UNIT_TYPE_MECHANICAL) == false) and (IsUnitType(u, UNIT_TYPE_ANCIENT) == false) then
    if (GetUnitAbilityLevel(u, NegBuff) < (GetUnitAbilityLevel(H, Aura))) then
        call UnitAddAbility(u, NegBuff)
        call SetUnitAbilityLevel(u, NegBuff, (GetUnitAbilityLevel(H, Aura)))
    endif
    call UnitDamageTarget(H, u, (GetUnitAbilityLevel(H, Aura)), true, true, ATTACK_TYPE_MAGIC, DAMAGE_TYPE_DEATH, WEAPON_TYPE_WHOKNOWS)
    call DestroyEffect(AddSpecialEffectTarget("Abilities\\Spells\\Undead\\DeathandDecay\\DeathandDecayDamage.mdl", u, "chest"))
elseif (IsUnitAlly(u, GetOwningPlayer(H))) and (IsUnitType(u, UNIT_TYPE_SUMMONED)) then
    if (GetUnitAbilityLevel(u, PosBuff) < (GetUnitAbilityLevel(H, Aura))) then
        call UnitAddAbility(u, PosBuff)
        call SetUnitAbilityLevel(u, PosBuff, (GetUnitAbilityLevel(H, Aura)))
    endif
    call SetUnitState(u, UNIT_STATE_LIFE, (GetUnitState(u, UNIT_STATE_LIFE) + 1))
endif
set u = null
return false
endfunction

private function DecayCheck takes nothing returns boolean
local integer n = 0
set ENUM = CreateGroup()
loop
    exitwhen (n == Heroes)
    if (Hero[n] != null) then
        set H = Hero[n]
        call GroupEnumUnitsInRange(ENUM, GetUnitX(Hero[n]), GetUnitY(Hero[n]), 900.00, Condition(function Group))
    endif
    set n = n + 1
endloop
call GroupEnumUnitsInRect(ENUM, bj_mapInitialPlayableArea, Condition(function BuffRemove))
call DestroyGroup(ENUM)
set H = null
return false
endfunction

private function AuraofDecayMain takes nothing returns boolean
    if (GetLearnedSkill() == Aura)  then
        if (GetUnitAbilityLevel(GetLearningUnit(), Aura) == 1) then
            set Hero[Heroes] = GetLearningUnit()
            call UnitAddAbility(GetLearningUnit(), AuraStatIcon)
        set Heroes = Heroes + 1
        else
            call SetUnitAbilityLevel(GetLearningUnit(), AuraStatIcon, (GetUnitAbilityLevel(GetLearningUnit(), Aura)))
        endif
    endif
return false
endfunction

private function Removal takes nothing returns boolean
    if (GetItemTypeId(GetManipulatedItem()) == 'tret') then
        call UnitRemoveAbility(GetManipulatingUnit(), Aura)
        call UnitRemoveAbility(GetManipulatingUnit(), AuraStatIcon)
    endif
return false
endfunction

//===========================================================================
private function Init takes nothing returns nothing
    local trigger t1 = CreateTrigger()
    local trigger t2 = CreateTrigger()
    local trigger t3 = CreateTrigger()

    call TriggerRegisterAnyUnitEventBJ(t1, EVENT_PLAYER_HERO_SKILL)
    call TriggerAddCondition(t1, Condition(function AuraofDecayMain))

    call TriggerRegisterTimerEvent(t2, 0.50, true)
    call TriggerAddCondition(t2, Condition(function DecayCheck))

    call TriggerRegisterAnyUnitEventBJ(t3, EVENT_PLAYER_UNIT_USE_ITEM)
    call TriggerAddCondition(t3, Condition(function Removal))

endfunction

endscope

07-13-2009, 11:31 AM	#2
Anitarf	It's not MUI.

07-13-2009, 11:44 AM

Michael Peppers

Quote:

Originally Posted by Anitarf

It's not MUI.

It's MPI, though. As a unique hero spell, it can work this way too.

Anyway, thoughts on making it MUI?

EDIT: Nevermind. I just had an idea :P

EDIT 2: Done. MUIed.

07-13-2009, 03:34 PM

Hans_Maulwurf

you leak GetUnitLoc(Hero[n]). why dont use the x,y function?
and you are enuming and destroying the local groups in BuffRemove without ever creating them. does this even work oO? also why are you using two different groups? you should use a global group there anyway
using a trigger just for DecayCheck seems a bit overkill to me. but i dont know if it would be any better to make a periodic timer run that function instead of TriggerRegisterTimerEvent so.. meh

07-13-2009, 03:49 PM

Michael Peppers

Quote:

Originally Posted by Hans_Maulwurf

you leak GetUnitLoc(Hero[n]). why dont use the x,y function?

Fixed. Thanks.

Quote:

Originally Posted by Hans_Maulwurf

and you are enuming and destroying the local groups in BuffRemove without ever creating them. does this even work oO?

Yep, I've created them :)

Quote:

Originally Posted by Hans_Maulwurf

also why are you using two different groups? you should use a global group there anyway

One is for the allied units, one for the enemy units.

Quote:

Originally Posted by Hans_Maulwurf

using a trigger just for DecayCheck seems a bit overkill to me. but i dont know if it would be any better to make a periodic timer run that function instead of TriggerRegisterTimerEvent so.. meh

Allright, thanks again for the feedback.

07-13-2009, 05:19 PM	#6
Viikuna-	Your group usage is pretty bad, you should not create&destroy them like that. Use a global group for calling GroupEnums and do your stuff in filterfunction. Getting GroupUtils is not a bad idea, neither is reading that GroupUtils thread.

07-13-2009, 05:36 PM

Michael Peppers

Quote:

Originally Posted by Viikuna-

Your group usage is pretty bad, you should not create&destroy them like that. Use a global group for calling GroupEnums

Is that a performance issue? Or only fancy code habits? :D

Quote:

Originally Posted by Viikuna-

and do your stuff in filterfunction.

Uhm... well... I do, when I can, but sometimes it's not possible.

Quote:

Originally Posted by Viikuna-

Getting GroupUtils is not a bad idea, neither is reading that GroupUtils thread.

Okie, I'll look into it.

07-13-2009, 06:07 PM	#8
Viikuna-	Creating and destroying handles is slow, so its better to recycle them when you can. ( Or just use one global group for all GroupEnum, one global dummy caster for adding all buffs, one global timer for all knockbacks... ) Also, if I remember right, some guy in that GroupUtils thread said that GroupEnum leaks with temporary groups.

07-13-2009, 07:40 PM	#9
Michael Peppers	Thanks. Check your rep if you didn't yet