[vJASS] Is this spell leakless? Thanks.

[Geshishouhu]

Hi all. I am making a spell that can increase a group of hero's stats for a certain period of time. The code looks like the following:

struct Spell

    private static constant integer ABIL_CODE = 'Arej'

    private static constant real TIMEOUT = 20.00

    private thistype next
    private thistype prev

    //private static integer count = 0

    private unit caster
    private unit target
    private unit u
    private real damage
    private real duration
    private real x
    private real y
    
    private timer iterator 
    
    private group g

    private method destroy takes nothing returns nothing

        call this.deallocate()

        set this.next.prev = this.prev
        set this.prev.next = this.next

        //set count = count - 1
        //if count == 0 then
        call PauseTimer(this.iterator)
        //endif
        call DestroyGroup(this.g)
        set this.caster = null
        set this.target = null
        set this.g=null
        set this.iterator=null
    endmethod
    
    private static method Add takes nothing returns nothing
        local unit enumm=GetEnumUnit()
        local integer str=GetHeroStr(enumm,false) 
        local integer agi=GetHeroAgi(enumm,false)
        local integer intt=GetHeroInt(enumm,false)
        call SetHeroStr(enumm,str+1000,true)
        call SetHeroAgi(enumm,agi+1000,true)
        call SetHeroInt(enumm,intt+1000,true)
        call BJDebugMsg("+1000")
        set enumm=null
    endmethod
    
    private static method Subtract takes nothing returns nothing
        local unit enumm=GetEnumUnit()
        local integer str=GetHeroStr(enumm,false) 
        local integer agi=GetHeroAgi(enumm,false)
        local integer intt=GetHeroInt(enumm,false)
        call SetHeroStr(enumm,str-1000,true)
        call SetHeroAgi(enumm,agi-1000,true)
        call SetHeroInt(enumm,intt-1000,true)
        call BJDebugMsg("-1000")
        set enumm=null
    endmethod

    private static method callback takes nothing returns nothing
        local thistype this = thistype(0).next
        call ForGroup(this.g,function thistype.Subtract)
        call BJDebugMsg("destroy")
        call this.destroy()
        set this = this.next
    endmethod

    private static method run takes nothing returns boolean
        local thistype this = thistype.allocate()

        set this.next = 0
        set this.prev = thistype(0).prev
        set thistype(0).prev.next = this
        set thistype(0).prev = this

        set this.caster = GetTriggerUnit()
        set this.target = GetSpellTargetUnit()
       set this.g=CreateGroup()
        set this.x=GetUnitX(this.target)
        set this.y=GetUnitY(this.target)
        set this.iterator=CreateTimer()
        
        call GroupEnumUnitsInRange(this.g,this.x,this.y,500.,null)
        call ForGroup(this.g,function thistype.Add)
        
        //set count = count + 1
        //if count == 1 then
        call TimerStart(this.iterator, TIMEOUT, false, function thistype.callback)
        //endif

        return false
    endmethod
    
    private static method spell takes nothing returns boolean
        if GetSpellAbilityId()==ABIL_CODE then
            call thistype.run()
        endif
        return false
    endmethod

    private static method onInit takes nothing returns nothing
        local trigger t = CreateTrigger()
        call TriggerRegisterAnyUnitEventBJ(t, EVENT_PLAYER_UNIT_SPELL_EFFECT)
        call TriggerAddCondition(t, Condition(function thistype.spell))
        set t = null
    endmethod
endstruct

It does work but is it leakless? Thanks all.

 

[TriggerHappy]

timer iteratorleaks, you never destroy or recycle it.

It could also use some improvements such as re-using handles and avoidingForGroup

Also in your current code these are kinda useless besides for maybe readability.

local integer str=GetHeroStr(enumm,false) 
local integer agi=GetHeroAgi(enumm,false)
local integer intt=GetHeroInt(enumm,false)

 

[Geshishouhu]

timer iteratorleaks, you never destroy or recycle it.

It could also use some improvements such as re-using handles and avoidingForGroup

Also in your current code these are kinda useless besides for maybe readability.

local integer str=GetHeroStr(enumm,false) 
local integer agi=GetHeroAgi(enumm,false)
local integer intt=GetHeroInt(enumm,false)

Thanks for pointing that out.
Do I need to use this when using Timer? I am not sure what those for... :/

set count = count + 1
        if count == 1 then
        call TimerStart(.....)
        endif

 

[BPower]

Currently your code picks friends and foes. You have no use for unit target and unit caster or you didn't post the full code.

I'm in the best mood so I re-wrote your spell the way I would do it and comented everything. (Except the ForGroup thing ...)

As always it helps to start reading at the bottom (onInit)

scope MySpellName //library MySpellName uses TimerUtils, Alloc

//Use scope or library to seperate this code from the rest in your map.
//Use a scope if the code of this spell is not used by any other resource.
//Scopes are compiled after libraries. So they can use all information of any library by default. Libraries can't use scopes.
//Libraries can use libraries with one of these keyowrds: uses, needs, requires.
//Picture it like a book. A library is chapter one, scope is chapter two.


    globals//Design you configuration part as clean and readable as possible
        private constant integer ABIL_CODE = 'Arej'
        private constant real      TIMEOUT = 20.00
    endglobals
    
    private function GetHeroStat takes nothing returns integer
    //takes integer level returns integer or takes integer WhichStat returns integer to return different values
    //depending on the stat for instance 100 for int 200 for strenght, 300 for agi                                                        
        return 1000//100 * WhichStat
    endfunction
    
    private struct MySpell extends array//http://www.hiveworkshop.com/forums/jass-ai-scripts-tutorials-280/coding-efficient-vjass-structs-187477/
        implement Alloc//That's a personal thing, but I think Alloc is just really nice.
        group g//I wouldn't use a group array at all, but for now its okay.
        
        
        private static method callback takes nothing returns nothing
            local timer t = GetExpiredTimer()//We get the timer
            local thistype this = GetTimerData(t)//GetTimerData will return the integer connected to the TimerHandle of t, its a function within the TimerUtils library
            call ForGroup(this.g,function thistype.subtract)//lowercase for methods
            
            call DestroyGroup(this.g)
            set this.g      = null
            
            call ReleaseTimer(t)//ReleaseTimer will "recycle" the used timer, its a function within the TimerUtils library. Primary it just pauses the timer and throws it back into the Timer stack.
            
            call BJDebugMsg("destroy")
            call this.deallocate()//method deallocate() and static method allocate() can be found in the Alloc module inside the Alloc library
            set t = null//Null the local timer variable
        endmethod
        
        //Like TriggerHappy said, erase useless code if you don't need it for readability.
         private static method add takes nothing returns nothing
            local unit enumm = GetFilterUnit()//Atm I can't remember why  GetFilterUnit() is better than GetEnumUnit(), but I think it is ....
            local integer i = GetHeroStat()//Make the spell more configurable? ( ofc this is optional )
            call SetHeroStr(enumm,GetHeroStr(enumm,false)+i,true)//call SetHeroStr(enumm,GetHeroStr(enumm,false)+1000,true)
            call SetHeroAgi(enumm,GetHeroAgi(enumm,false)+i,true)
            call SetHeroInt(enumm,GetHeroInt(enumm,false)+i,true)
            call BJDebugMsg("+1000")
            set enumm=null
        endmethod
   
        private static method subtract takes nothing returns nothing
            local unit enumm = GetFilterUnit()//In case this doesn't work use GetEnumUnit() as I said I'm not 100% sure.
            local integer i = GetHeroStat()
            call SetHeroStr(enumm,GetHeroStr(enumm,false)-i,true)
            call SetHeroAgi(enumm,GetHeroAgi(enumm,false)-i,true)
            call SetHeroInt(enumm,GetHeroInt(enumm,false)-i,true)
            call BJDebugMsg("-1000")
            set enumm=null
        endmethod
        
        
        private static method run takes nothing returns nothing// <--- returns nothing
            local thistype this = allocate()
            local real x  = GetSpellTargetX()//Use a local real x and y instead
            local real y  = GetSpellTargetY()

            /*The linked list is a total overkill here imo */
        
            set this.g      = CreateGroup()
            
            call GroupEnumUnitsInRange(this.g, x, y, 500., null)
            call ForGroup(this.g,function thistype.add)//add should be lower case.
              
            //For NewTimerEx you need library TimerUtils --> [url]http://www.wc3c.net/showthread.php?t=101322[/url]
            //It allows you to recylce timers, just like Alloc this is optional, but pretty good
            call TimerStart(NewTimerEx(this), TIMEOUT, false, function thistype.callback)
        endmethod
        
        private static method spell takes nothing returns boolean
            if (GetSpellAbilityId() == ABIL_CODE) then//Use brackets (.... == ...)  
                call thistype.run()
            endif
            return false//Conditions must return a booolean, function calls not.
        endmethod
        
        private static method onInit takes nothing returns nothing
            local trigger t = CreateTrigger()
            call TriggerRegisterAnyUnitEventBJ(t, EVENT_PLAYER_UNIT_SPELL_EFFECT)
            call TriggerAddCondition(t, Condition(function thistype.spell))
            set t = null
        endmethod
    endstruct
endscope

 

[Geshishouhu]

Currently your code picks friends and foes. You have no use for unit target and unit caster or you didn't post the full code.

I'm in the best mood so I re-wrote your spell the way I would do it and comented everything. (Except the ForGroup thing ...)

As always it helps to start reading at the bottom (onInit)

scope MySpellName //library MySpellName uses TimerUtils, Alloc

//Use scope or library to seperate this code from the rest in your map.
//Use a scope if the code of this spell is not used by any other resource.
//Scopes are compiled after libraries. So they can use all information of any library by default. Libraries can't use scopes.
//Libraries can use libraries with one of these keyowrds: uses, needs, requires.
//Picture it like a book. A library is chapter one, scope is chapter two.


    globals//Design you configuration part as clean and readable as possible
        private constant integer ABIL_CODE = 'Arej'
        private constant real      TIMEOUT = 20.00
    endglobals
    
    private function GetHeroStat takes nothing returns integer
    //takes integer level returns integer or takes integer WhichStat returns integer to return different values
    //depending on the stat for instance 100 for int 200 for strenght, 300 for agi                                                        
        return 1000//100 * WhichStat
    endfunction
    
    private struct MySpell extends array//http://www.hiveworkshop.com/forums/jass-ai-scripts-tutorials-280/coding-efficient-vjass-structs-187477/
        implement Alloc//That's a personal thing, but I think Alloc is just really nice.
        group g//I wouldn't use a group array at all, but for now its okay.
        
        
        private static method callback takes nothing returns nothing
            local timer t = GetExpiredTimer()//We get the timer
            local thistype this = GetTimerData(t)//GetTimerData will return the integer connected to the TimerHandle of t, its a function within the TimerUtils library
            call ForGroup(this.g,function thistype.subtract)//lowercase for methods
            
            call DestroyGroup(this.g)
            set this.g      = null
            
            call ReleaseTimer(t)//ReleaseTimer will "recycle" the used timer, its a function within the TimerUtils library. Primary it just pauses the timer and throws it back into the Timer stack.
            
            call BJDebugMsg("destroy")
            call this.deallocate()//method deallocate() and static method allocate() can be found in the Alloc module inside the Alloc library
            set t = null//Null the local timer variable
        endmethod
        
        //Like TriggerHappy said, erase useless code if you don't need it for readability.
         private static method add takes nothing returns nothing
            local unit enumm = GetFilterUnit()//Atm I can't remember why  GetFilterUnit() is better than GetEnumUnit(), but I think it is ....
            local integer i = GetHeroStat()//Make the spell more configurable? ( ofc this is optional )
            call SetHeroStr(enumm,GetHeroStr(enumm,false)+i,true)//call SetHeroStr(enumm,GetHeroStr(enumm,false)+1000,true)
            call SetHeroAgi(enumm,GetHeroAgi(enumm,false)+i,true)
            call SetHeroInt(enumm,GetHeroInt(enumm,false)+i,true)
            call BJDebugMsg("+1000")
            set enumm=null
        endmethod
   
        private static method subtract takes nothing returns nothing
            local unit enumm = GetFilterUnit()//In case this doesn't work use GetEnumUnit() as I said I'm not 100% sure.
            local integer i = GetHeroStat()
            call SetHeroStr(enumm,GetHeroStr(enumm,false)-i,true)
            call SetHeroAgi(enumm,GetHeroAgi(enumm,false)-i,true)
            call SetHeroInt(enumm,GetHeroInt(enumm,false)-i,true)
            call BJDebugMsg("-1000")
            set enumm=null
        endmethod
        
        
        private static method run takes nothing returns nothing// <--- returns nothing
            local thistype this = allocate()
            local real x  = GetSpellTargetX()//Use a local real x and y instead
            local real y  = GetSpellTargetY()

            /*The linked list is a total overkill here imo */
        
            set this.g      = CreateGroup()
            
            call GroupEnumUnitsInRange(this.g, x, y, 500., null)
            call ForGroup(this.g,function thistype.add)//add should be lower case.
              
            //For NewTimerEx you need library TimerUtils --> [url]http://www.wc3c.net/showthread.php?t=101322[/url]
            //It allows you to recylce timers, just like Alloc this is optional, but pretty good
            call TimerStart(NewTimerEx(this), TIMEOUT, false, function thistype.callback)
        endmethod
        
        private static method spell takes nothing returns boolean
            if (GetSpellAbilityId() == ABIL_CODE) then//Use brackets (.... == ...)  
                call thistype.run()
            endif
            return false//Conditions must return a booolean, function calls not.
        endmethod
        
        private static method onInit takes nothing returns nothing
            local trigger t = CreateTrigger()
            call TriggerRegisterAnyUnitEventBJ(t, EVENT_PLAYER_UNIT_SPELL_EFFECT)
            call TriggerAddCondition(t, Condition(function thistype.spell))
            set t = null
        endmethod
    endstruct
endscope

Thank you so much, BPower! I will give you Rep when I can. Many thanks for the re-write.
Edit: I think GetFilterUnit() should be replaced by GetEnumUnit(). I tried the first one yesterday and it did not work.

 

[Arhowk]

Yeah, filter should only be used when using Filter(function xxx)