Is this well-written?

[Moon_Raven]

Ok, so I needed a trigger which looks like this:
if two units are on a circle of power they are teleported on a location.
So I wrote this:

function Trig_N3_PRVI_Actions takes nothing returns nothing
local unit u
local location l
local group g
local integer n
local integer count
set count = 0
set g = GetUnitsInRectAll(gg_rct_Circle1)
set n = CountUnitsInGroup(g)
if (n>2) then
    loop
    exitwhen(count==2)
    set g = GetUnitsInRectAll(gg_rct_Circle1)
    set u = GroupPickRandomUnit(g)
    if (( GetUnitTypeId(u) != 'ncp3' ) ) then
        set l = Location(-8229,5738)
        call SetUnitPositionLoc(u,l)
        call RemoveLocation(l)
        set count = count +1
    endif
    endloop
endif
endfunction

//===========================================================================
function InitTrig_N3_PRVI takes nothing returns nothing
    set gg_trg_N3_PRVI = CreateTrigger(  )
    call TriggerRegisterTimerEventPeriodic( gg_trg_N3_PRVI, 0.40 )
    call TriggerAddAction( gg_trg_N3_PRVI, function Trig_N3_PRVI_Actions )
endfunction

but the game seems to be a little bit laggy from the beggining, does this trigger cause that? Does it have some leaks?

 

[Vestras]

Yes, it leaks. You never null either n, u, g or l and you don't destroy g. (DestroyGroup(g))

 

[Romek]

local unit u
local location l
local group g
local integer n
local integer count
set count = 0
set g = GetUnitsInRectAll(gg_rct_Circle1)
set n = CountUnitsInGroup(g)

Could be:

local unit u
local location l
local group g = GetUnitsInRectAll(gg_rct_Circle1)
local integer n = CountUnitsInGroup(g)
local integer count = 0

> (( GetUnitTypeId(u) != 'ncp3' ) )
GetUnitTypeId(u) != 'ncp3'. Less bracket spam.

    set gg_trg_N3_PRVI = CreateTrigger(  )
    call TriggerRegisterTimerEventPeriodic( gg_trg_N3_PRVI, 0.40 )
    call TriggerAddAction( gg_trg_N3_PRVI, function Trig_N3_PRVI_Actions )

Could be:

call TimerStart(CreateTimer(), 0.4, true, function Trig_N3_PRVI_Actions)

And, as mentioned before, it leaks. (Also, You've got some useless BJs there)

You could initialize that location, as it's never going to change anyway.
Remove it after the loop if you do that.

 

[Lyle.]

Don't mean to be off-topic but in response to Romek,

Just curious does it make a noticeable difference if you initialize your variables when you declare them as opposed to initializing them after they have been declared?

 

[Romek]

> Just curious does it make a noticeable difference if you initialize your variables when you declare them
Noticeable difference? No.
Though it does make your code nicer.
And it's probably faster, by a few microseconds. (Which is all but noticeable)

 

[wraithseeker]

call DestroyGroup(g)
set g = null
set l = null
set u = null

Add this at the end of your code, you are not fixing leaks.

 

[Tom Jones]

    loop
    exitwhen(count==2)
    set g = GetUnitsInRectAll(gg_rct_Circle1)
    set u = GroupPickRandomUnit(g)
    if (( GetUnitTypeId(u) != 'ncp3' ) ) then
        set l = Location(-8229,5738)
        call SetUnitPositionLoc(u,l)
        call RemoveLocation(l)
        set count = count +1 //<-- Oh oh.
    endif
endloop

Your setting a part of the loop's exitwhen expression inside an if. This could prove very inefficient. Consider if the unit was of 'ncp3' type, then you'll have an endless loop.

 

[Moon_Raven]

But it can't be, that is a circle of power
Thanks a lot guys!