[JASS] Zinc code optimization

[darkrider]

Hello guys, I've been a GUI coder since 2008 and i decided to learn JASS vJASS and Zinc altogether. So here i made a spell in Zinc and i would like if you could give me some tips to make mi code better and more understandable to normal human eyes.

What does the spell do?

Well, what it does is simple. It consists of an item that upgrades itself by the units you kill. If you kill 3 units it changes to first upgrade, if you kill 6 then it changes to second upgrade and finally if you kill 9 units, it will change to the last upgrade which is an item that has an special ability (which does something and then returns everything to 0, the counter and the item)

Here is the code

//! zinc
library LifeBottleKillsUnit{
    integer x[];
    integer y[9];
    integer maxItems;
    integer charges[];

    function conditions() -> boolean{
        unit u = GetKillingUnit();
        return ((IsUnitType(u, UNIT_TYPE_HERO) == true ) && (IsUnitType(GetDyingUnit(), UNIT_TYPE_STRUCTURE) == false) && ((GetInventoryIndexOfItemTypeBJ(u, x[0]) > 0) || (GetInventoryIndexOfItemTypeBJ(u, x[1]) > 0) || (GetInventoryIndexOfItemTypeBJ(u, x[2]) > 0)));
    }

    function changeItem(unit u, integer playerId){
        integer i;
        for(0 <= i <= maxItems-2){
            if (GetInventoryIndexOfItemTypeBJ(u, x[i]) > 0){
                RemoveItem( UnitItemInSlot(u, x[i]));
                UnitAddItemById( u, x[i+1]);
                break;
            }
        }
        if(i==maxItems-2){y[playerId] = 0;}
    }
    
    function addChargesToItem(){
        unit u = GetKillingUnit();
        integer i = GetPlayerId(GetOwningPlayer(u));
        y[i] = y[i]+1;
        if(( y[i] == charges[1]) || (y[i] == charges[2]) || (y[i] == charges[3])){
            changeItem(u, i);
        }
    }
    
    function conditionT2() -> boolean{
        return (GetItemTypeId(GetManipulatedItem()) == x[maxItems-1]);
    }
    
    function resetValues(){
        RemoveItem(GetManipulatedItem());
        UnitAddItemById( GetTriggerUnit(), x[0]);
    }
    
    function onInit()
    {
        //Unit dies add charge trigger
        trigger t = CreateTrigger();
        //Unit casts fully-upgraded item's ability trigger
        trigger t2 = CreateTrigger();
        TriggerRegisterAnyUnitEventBJ( t, EVENT_PLAYER_UNIT_DEATH );
        TriggerAddCondition(t, Condition(function conditions));
        TriggerAddAction(t, function addChargesToItem);
        
        //Hero uses item, reset everything
 
        TriggerRegisterAnyUnitEventBJ(t2, EVENT_PLAYER_UNIT_USE_ITEM );
        TriggerAddCondition(t2, Condition(function conditionT2));
        TriggerAddAction(t2, function resetValues);
        
        t = null;
        t2 = null;
        
        //initialization
        x[0] = 'afac';
        x[1] = 'spsh';
        x[2] = 'bgst';
        x[3] = 'ajen';
        charges[1] = 3;
        charges[2] = 6;
        charges[3] = 9;
        maxItems = 4;
        //end initialization
    }
}
//! endzinc

 

[Axarion]

You should remove some BJs

UnitAddItemByIdSwapped -> UnitAddItemById
GetConvertedPlayerId -> GetPlayerId

 

[darkrider]

BJs are the kind of things i drag from GUI (as i don't know the functions yet, i convert GUI to JASS and then use the result :/)

thanks ^^


EDIT: I've replaced those 2 and also

this

UnitHasItemOfTypeBJ()

for this

GetInventoryIndexOfItemTypeBJ() > 0

*Updated first post*

 

[Bribe]

You do not need to initialize the variable "i" right before the for-loop as the for-loop adds a "set i = 0" right before compiling the loop.

 

[darkrider]

oh!, i didn't see that one

thanks ^^

 

[Raven0]

You should null the trigger variables after your done with them. I'm pretty sure they are only pointers, but its good practice anyways.

You have an unneeded double function call with the first triggers condition. Honestly, I would just add the conditions into the action functions, it'll be more efficient that way.

Instead of repeating function calls like GetKillingUnit() and GetPlayerId(), just call them in the begining of the function and set them to a local variable. Then just refer to the local variable whenever you need to reference it in the function.

Just my thoughts ^-^ GL

 

[darkrider]

You should null the trigger variables after your done with them. I'm pretty sure they are only pointers, but its good practice anyways.

Done, missed that

You have an unneeded double function call with the first triggers condition. Honestly, I would just add the conditions into the action functions, it'll be more efficient that way.

You are saying like "if (something) then (the rest of the actions)"?

and the double function call was just in case i wanted to add some more conditions later

Instead of repeating function calls like GetKillingUnit() and GetPlayerId(), just call them in the begining of the function and set them to a local variable. Then just refer to the local variable whenever you need to reference it in the function.

Just my thoughts ^-^ GL

Yep, you are correct. I wanted to learn the functions so i typed them a lot xD. I've changed what you said, but now i have a question. Should local variables be removed and nulled? or are they just fine that way?

 

[Raven0]

You are saying like "if (something) then (the rest of the actions)"?

Usually its "if (not something) then (skip rest of actions)" to make the code cleaner, like so:

scope OMGWTFBBQ initializer Init

    globals
        integer ABILITY_ID = 'h001'
    endglobals

    private function SkillLearned takes nothing returns nothing
        local unit LearningUnit
        
        if GetLearnedSkill() != ABILITY_ID then
            return
        endif
        
        //remember to set local variables only after you've done the check (to avoid leaks)
        set LearningUnit = GetTriggerUnit()
        
        //===============
        //enter code here
        //===============
        
        //null unit variable when done
        set LearningUnit = null
    endfunction
    
    //===========================================================================
    private function Init takes nothing returns nothing
        local trigger t = CreateTrigger()
        
        call TriggerRegisterAnyUnitEventBJ(t, EVENT_PLAYER_HERO_SKILL)
        call TriggerAddAction(t, function SkillLearned)
        
        set t = null
    endfunction
    
endscope

Should local variables be removed and nulled? or are they just fine that way?

Depends on the data type. Things like integers, reals, and booleans don't need to be nulled. However, certain types (usually blizz-defined types) like units, should be nulled. You just sort of have to remember which ones need to be destroyed or nulled as you go.



________________________________________

Oh and also regarding the use of BJs. Remember that they aren't ALL bad, some are actually quite useful.

 

[Bribe]

List of all things which should not be nulled:

1. Parameters (these can be much faster than locals because of this)
2. Real
3. Integer
4. Code
5. String
6. Boolean

All local-variable handles should be nulled in all cases. Why:

1. The handle stack is not deallocated properly at the endfunction line.
2. This causes increase in memory for every allocated handle.
3. With the primary stack (units, timers, groups) this can cause handle locks as well.

 

[darkrider]

Im sorry it took me so long to post. But I've been busy with university. Thanks a lot for the assistance I'm gonna update my code now ^^

EDIT: Question: How could i null the variable "u" from the function "conditions"?

 

[Bribe]

Three ways:

1. (best) Instead of local unit, use a global unit
2. (decent) Set the return value to a boolean, null u, return the boolean
3. (worst) Spam GetKillingUnit() instead of setting it to a variable