[vJASS] My first simple vJass spell. Need criticism. :)

[Geshishouhu]

Hi all. If the spell is cast on ally unit, it periodically heals ally unit and remove negative buffs from it. If being cast on enemy, it damages it periodically.
I am still learning vJass, so this spell must be too simple to you guys. I just want to know if I am coding in the right way.

scope spell
    //Configuration Starts
    globals
        private constant integer AB_ID='A00T'       
        private real duration=40.                  
        private constant real period=0.50           
    endglobals
    
    private constant function Heal takes integer level returns real 
        return level * 50.
    endfunction 
    
    private constant function Damage takes integer level returns real
        return level * 10.
    endfunction
    
    private function Unit_Filter takes unit enemy, player casterOwner returns boolean
        return /*
        */IsUnitEnemy(enemy,casterOwner) and /*
        */ not IsUnitType(enemy, UNIT_TYPE_MAGIC_IMMUNE) and /*
        */ not IsUnitType(enemy, UNIT_TYPE_STRUCTURE) /*
        */
    endfunction
    //Configuration Ends
     
    native UnitAlive takes unit id returns boolean
    
    globals
        private hashtable ht=InitHashtable()
    endglobals 
    
    private struct Rejuvenation
        private timer t
        private unit caster
        private unit target
        private real dur
        private thistype next
        private thistype prev
        private static integer count = 0
        
        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 
                if LIBRARY_TimerUtils then
                    call ReleaseTimer(this.t)
                else
                    call PauseTimer(this.t)
                    //call DestroyTimer(this.t)  Fixed this, thanks Ruke.
                endif
            endif
            
            set this.caster = null
            set this.target = null
            //set this.t=null  Fixed this accordingly
        endmethod
    
        private static method callback takes nothing returns nothing
            local thistype this = thistype(0).next
            loop
                exitwhen this == 0
                set this.dur = this.dur - period
                
                if this.dur <= 0.00 or not UnitAlive(this.target) then
                    call this.destroy()
                endif

                if  Unit_Filter(this.target,GetOwningPlayer(this.caster))  then
                    call UnitDamageTarget(this.caster,this.target,Damage(GetUnitAbilityLevel(this.caster,AB_ID)),true,false,ATTACK_TYPE_CHAOS,DAMAGE_TYPE_DEMOLITION,null)         
                else
                    call UnitRemoveBuffs(this.target,false,true)
                    call SetUnitState(this.target,UNIT_STATE_LIFE,GetUnitState(this.target,UNIT_STATE_LIFE)+Heal(GetUnitAbilityLevel(this.caster,AB_ID)))
                    call DestroyEffect(AddSpecialEffectTarget("Abilities\\Spells\\Items\\AIma\\AImaTarget.mdl",this.target,"origin"))
                endif
                
                set this = this.next
            endloop
        endmethod
    
        private static method run takes nothing returns nothing
            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 count = count + 1
            if count == 1 then
                static if LIBRARY_TimerUtils then
                    set this.t=NewTimer()
                    call TimerStart(this.t,period,true,function thistype.callback)
                else
                    set this.t=CreateTimer()
                    call TimerStart(this.t, period, true, function thistype.callback)
                endif
            endif
            
            set this.dur=duration
            set this.target=GetSpellTargetUnit()
            set this.caster=GetTriggerUnit()
        endmethod
    
        private static method cast takes nothing returns boolean
            if GetSpellAbilityId()==AB_ID 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.cast))   
            set t=null 
        endmethod
    endstruct
    
endscope

 

[Ruke]

it's fine, except (at least for me) for the timer part. just use a hashtable and you will be fine .

edit: don't destroy the timer (private timer t), just pause it so you can recycle it.
edit: place call this.deallocate() at the very bottom.

 

[Geshishouhu]

it's fine, except (at least for me) for the timer part. just use a hashtable and you will be fine .

edit: don't destroy the timer (private timer t), just pause it so you can recycle it.
edit: place call this.deallocate() at the very bottom.

Thanks for the reply.
Do you mean putting call this.deallocate() at the bottom of the destroy method?

 

[Ruke]

Do you mean putting call this.deallocate() at the bottom of the destroy method?

yes

and get rid of set this.t=null (on destroy)

edit: by the way, you don't need structs for this. use them when you really see that you need more organization (here you only have a couple of variables/methods, nothing too complex).

 

[Geshishouhu]

yes

and get rid of set this.t=null (on destroy)

edit: by the way, you don't need structs for this. use them when you really see that you need more organization (here you only have a couple of variables/methods, nothing too complex).

Thanks. So last question, under what circumstance is a struct needed?

 

[TriggerHappy]

edit: by the way, you don't need structs for this. use them when you really see that you need more organization (here you only have a couple of variables/methods, nothing too complex).

I disagree.

He would have to create his own allocation and stuff which isn't needed.

Thanks. So last question, under what circumstance is a struct needed?

You're perfectly fine using structs, it's not overkill at all.

 

[Geshishouhu]

I disagree.

He would have to create his own allocation and stuff which isn't needed.




You're perfectly fine using structs, it's not overkill at all.

Aight. So do you mind taking a look at my simple spell?

 

[TriggerHappy]

Aight. So do you mind taking a look at my simple spell?

Sure.

• real duration=40.should be constant.
• I think you should be using something like Table, so you don't create a new hashtable each spell.
• if LIBRARY_TimerUtils then -> static if LIBRARY_TimerUtils then

 

[Geshishouhu]

Sure.

• real duration=40.should be constant.
• I think you should be using something like Table, so you don't create a new hashtable each spell.
• if LIBRARY_TimerUtils then -> static if LIBRARY_TimerUtils then

Thanks for the advice. But I don't know how to use Bribe's Table yet. :/
Edit: something like this?

    static if Table.has.exists then
        globals
            private Table tb
        endglobals
    else
        globals
            private hashtable ht = InitHashtable()
        endglobals
    endif

Edit: just realize the hashtable is never used in the spell. LOL

 

[Ruke]

I disagree.

He would have to create his own allocation and stuff which isn't needed.

use the traditional way (timer + hashtable). that will do the job .

Thanks. So last question, under what circumstance is a struct needed?

for more complex things, where you need more than 5 variables or need performance (loading/saving from hashtable is slower than array/struct).

edit: as you're using structs, you don't need TimerUtils (you're already recycling the timers).

 

[Geshishouhu]

edit: as you're using structs, you don't need TimerUtils (you're already recycling the timers).

Thank for pointing that out. +Rep

 

[PurgeandFire]

I would store the "Damage()" and "Heal()" parts in a variable so that you don't have to calculate it constantly in the "callback". Also, in general I would recommend SetWidgetLife() and GetWidgetLife(). There is no practical speed advantage, but it makes the code a bit more readable compared to SetUnitState(u, UNIT_STATE_LIFE).

 

[Geshishouhu]

I would store the "Damage()" and "Heal()" parts in a variable so that you don't have to calculate it constantly in the "callback". Also, in general I would recommend SetWidgetLife() and GetWidgetLife(). There is no practical speed advantage, but it makes the code a bit more readable compared to SetUnitState(u, UNIT_STATE_LIFE).

Thanks for the reply, PnF. I will take your advice.

 

[TriggerHappy]

use the traditional way (timer + hashtable). that will do the job .

Since when is that traditional, lol. Hashtables are relatively new and they are much slower than the regular allocation.

You're basically telling him to do more work for less efficiency.. haha

 

[BPower]

Not bad for the first attempt

Beside what is already mentioned above I would use a player variable instead of calling native GetOwningPlayer each loop.Unit_Filter(this.target,GetOwningPlayer(this.caster)).
40 seconds duration with 0.5 second timeout. That's 80 times GetOwningPlayer vs 1 time GetTriggerPlayer(). set this.owner = GetTriggerPlayer().

Currently you don't use the hashtable which I wouldn't use either. Also don't overdo it with static if's. For TimerUtils it is fair enough, but if you want to use Table in your resource just do it.
In general I don't use hashtables, if it is not necessary.

You double private the members of your struct, since the struct is private itself. --> private unit caster could be unit caster ( It doesn't really make a difference though.)

On THW we have a Jass Proper Application Guide aka JPAG. Mainly it standardizes variable and function names to improve read-ability. Meaning
constants like period or duration should be all capital letters and Unit_Filter should be UnitFilter.

 

[Ruke]

Since when is that traditional, lol. Hashtables are relatively new and they are much slower than the regular allocation.

haha no traditional xD, i wanted to say old-way.
no, they aren't relatively new <.< ...
yes they are slower, but you can't notice that on spells (not this one at least, although, that period is changing my opinion).

You're basically telling him to do more work for less efficiency.. haha

uhmm, i don't know, it would be pretty the same thing, but without using structs just because.

@Geshishouhu: the timer should be static.

edit: you don't need private static integer count = 0

 

[Geshishouhu]

Not bad for the first attempt

Beside what is already mentioned above I would use a player variable instead of calling native GetOwningPlayer each loop.Unit_Filter(this.target,GetOwningPlayer(this.caster)).
40 seconds duration with 0.5 second timeout. That's 80 times GetOwningPlayer vs 1 time GetTriggerPlayer(). set this.owner = GetTriggerPlayer().

Currently you don't use the hashtable which I wouldn't use either. Also don't overdo it with static if's. For TimerUtils it is fair enough, but if you want to use Table in your resource just do it.
In general I don't use hashtables, if it is not necessary.

You double private the members of your struct, since the struct is private itself. --> private unit caster could be unit caster ( It doesn't really make a difference though.)

On THW we have a Jass Proper Application Guide aka JPAG. Mainly it standardizes variable and function names to improve read-ability. Meaning
constants like period or duration should be all capital letters and Unit_Filter should be UnitFilter.

Thanks for the review, BPower. I will look into the JPAG and use a player variable.

haha no traditional xD, i wanted to say old-way.
no, they aren't relatively new <.< ...
yes they are slower, but you can't notice that on spells (not this one at least, although, that period is changing my opinion).



uhmm, i don't know, it would be pretty the same thing, but without using structs just because.

@Geshishouhu: the timer should be static.

edit: you don't need private static integer count = 0

Thanks again Ruke. Is there a difference if the timer is not static?

 

[edo494]

I would prefer using 1 timer that ticks every 0.0325 seconds for instance(can be higher, but with slightly less accuracy) then having N running timers(N = running instances) and only pause the timer so it doesnt call callback uselessly every time when there are 0 running instances

the way you use the timer currently is also implying you want something like that, but you instead use instance dependant timer

Yes, that would require a bit more work on the callback if you want each instance tick at different rates, but is doable

edit: I will answer your question:

Difference between static and non-static variable is, that static variable is not dependant on the instance, so it shares the same state with all instances(basically global variable) whreas non-static variable has different states(values) for every instance, which is like global array(this is exact implementation, unless you specify size of higher than 8191 for structs)

The advantage of static timer is, you only need to initialize it once, and you dont need TimerUtils for it, you just pause it when there are no running instances

 

[Geshishouhu]

I would prefer using 1 timer that ticks every 0.0325 seconds for instance(can be higher, but with slightly less accuracy) then having N running timers(N = running instances) and only pause the timer so it doesnt call callback uselessly every time when there are 0 running instances

the way you use the timer currently is also implying you want something like that, but you instead use instance dependant timer

Yes, that would require a bit more work on the callback if you want each instance tick at different rates, but is doable

edit: I will answer your question:

Difference between static and non-static variable is, that static variable is not dependant on the instance, so it shares the same state with all instances(basically global variable) whreas non-static variable has different states(values) for every instance, which is like global array(this is exact implementation, unless you specify size of higher than 8191 for structs)

The advantage of static timer is, you only need to initialize it once, and you dont need TimerUtils for it, you just pause it when there are no running instances

Got it, thanks. +Rep if I can.