[General] Trigger efficiency, is my new method better?

[Greenwhy]

This is mostly for clarification. I have just had the realisation that the way I make triggers can be greatly improved. Example:

This is a trigger in a map i have been making

• Hero Death
  • Events
    • Unit - A unit Dies
  • Conditions
    • ((Dying unit) is A Hero) Equal to True
    • ((Dying unit) belongs to an enemy of Player 12 (Brown)) Equal to True
  • Actions
    • Set Hero_AliveorDead[(Player number of (Owner of (Dying unit)))] = 0
    • Set Deaths[(Player number of (Owner of (Dying unit)))] = (Deaths[(Player number of (Owner of (Dying unit)))] + 1)
    • Multiboard - Set the text for (Last created multiboard) item in column 4, row ((Player number of (Owner of (Dying unit))) + 1) to (String(Deaths[(Player number of (Owner of (Dying unit)))])

the "Set Hero_AliveorDead" is required for another trigger which checks if all players heroes are dead

Have I improved the trigger here?

• Hero Death
  • Events
    • Unit - A unit Dies
  • Conditions
    • ((Dying unit) is A Hero) Equal to True
    • ((Dying unit) belongs to an enemy of Player 12 (Brown)) Equal to True
  • Actions
    • Set TempPlayer = (Owner of (Triggering unit))
    • Set Hero_AliveorDead[(Player number of TempPlayer)] = 0
    • Set Deaths[(Player number of TempPlayer)] = (Deaths[(Player number of TempPlayer)] + 1)
    • Multiboard - Set the text for (Last created multiboard) item in column 4, row ((Player number of TempPlayer) + 1) to (String(Deaths[(Player number of TempPlayer)]))

19 function calls vs 12 function calls (in trigger actions)
Can this be further improved?

Secondly...
I have a lot of arrays where the data item number refers to a player number (examples below) and I have not used 0 in the array as there is no player 0. Is it OK to do it this way or should I do (player number - 1)?

This is a trigger which contains several examples

• tp and pan to base
  • Events
  • Conditions
  • Actions
    • Wait 1.00 seconds
    • Set TempPlayerGroup = (All players controlled by a User player)
    • Player Group - Pick every player in TempPlayerGroup and do (Actions)
      • Loop - Actions
        • Set TempPlayer = (Picked player)
        • Camera - Pan camera for TempPlayer to StartPoint[(Player number of TempPlayer)] over 1.00 seconds
    • Custom script: call DestroyForce(udg_TempPlayerGroup)
    • Wait 0.50 seconds
    • Set TempPlayerGroup = (All players controlled by a User player)
    • Player Group - Pick every player in TempPlayerGroup and do (Actions)
      • Loop - Actions
        • Set TempPlayer = (Picked player)
        • Set TempPoint = (StartPoint[(Player number of TempPlayer)] offset by (-250.00, 100.00))
        • Unit - Move Hero[(Player number of (Picked player))] instantly to TempPoint
        • Custom script: call RemoveLocation(udg_TempPoint2)
    • Custom script: call DestroyForce(udg_TempPlayerGroup)
    • For each (Integer A) from 1 to 6, do (Actions)
      • Loop - Actions
        • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
          • If - Conditions
            • Hero_AliveorDead[(Integer A)] Equal to 0
          • Then - Actions
            • Set TempPoint = StartPoint[(Integer A)]
            • Set TempPoint2 = (TempPoint offset by (-250.00, 100.00))
            • Hero - Instantly revive Hero[(Integer A)] at TempPoint, Show revival graphics
            • Set Hero_AliveorDead[(Integer A)] = 1
            • Custom script: call RemoveLocation(udg_TempPoint)
            • Custom script: call RemoveLocation(udg_TempPoint2)
          • Else - Actions

 

[Maker]

1. You're not actually using the player, but player number. Therefore, instead of TempPlayer use TempInteger.

set TempInteger = (Player number of (Triggering player))

2. It is ok.

Remove the do nothing.

 

[WaterKnight]

1. Caching things is okay but would then advise to take an appropriate name for the variable. You would usually use a local variable (jass-only) as it's for the local context. Similarly, (Last created multiboard) is obscure as you probably do not actually mean to create new multiboards during runtime and then take "the last created multiboard" that was somehow established but your special multiboard you declared somewhere before.

Yeah, it's always kind of funny when GUI users ask about pushing efficiency
Should also say that there are different aspects one can orientate to as what is "good code". Readability, ingame performance, amount of work for the mapmaker, extendability/flexibility, map size, real scenario, ...

 

[cohadar]

While I was doing GUI I used TempIneger, TempX, TempY, TempFacing, TempUnit...

It is ok to use as long as you don't have any waits in your trigger.
If you have waits you cannot use generic temp variables.

 

[Greenwhy]

I have updated the last trigger in my original post, that is now what I'm using in my map at the moment.
Thanks Maker +rep

1. Caching things is okay but would then advise to take an appropriate name for the variable.

Are you saying I should use a better name for my variable?

You would usually use a local variable (jass-only) as it's for the local context.

I don't understand this, but that's probably because I don't know JASS.

Similarly, (Last created multiboard) is obscure as you probably do not actually mean to create new multiboards during runtime and then take "the last created multiboard" that was somehow established but your special multiboard you declared somewhere before.

I create a multiboard at the start of my map and any time i change it i refer to it this way. I have just learned that I should save it to a variable and refer to it that way, yet to fix all the references.

Yeah, it's always kind of funny when GUI users ask about pushing efficiency

Haha, I can see where you are coming from with this. I just don't have the motivation to learn JASS at the moment and I seem to be going OK without it... one day maybe but in the meantime I'd like to follow general good practice.
+rep

It is ok to use as long as you don't have any waits in your trigger.
If you have waits you cannot use generic temp variables.

I have removed/destroyed the generic variables before a wait command and reset them after the wait, this should prevent any problems???

 

[WaterKnight]

Are you saying I should use a better name for my variable?

A problem is the missing encapsulation in GUI. I do not expect you to create unique global variables for each trigger page as all get thrown in that ugly variable editor and are very limited in name length. So you may take shared variable names but then have to pay attention to overlappings. You do not have anything critical above but imagine you were to run another trigger from this one and that another trigger would also make use of the same variables. That would easily become a conflict. Local variables are only declared within a function call, so it can be recursively run and the outside environment has no access. That's also a reason why local variables can be shortened in name because the local little scope is simple and independent.

I have removed/destroyed the generic variables before a wait command and reset them after the wait, this should prevent any problems???

Yes, the trigger runs/event calls I mentioned above add to this circumstance, though. And you should be aware that not all event responses are constant in return value throughout the trigger call and that you might store other stuff that is not an event response. GUI users are lucky that these functions are trigger call-local anyway, so they are not overwritten by the next firing event.