[JASS] Single Cooldown Reset

[Dvoth]

This is my first jass trigger, I've been listening to Nestharus video tutorial and I'd like to have constructive criticism on it.

library ResetSingleCooldown

/******************************************************************************
*   To reset a cooldown just use:
*       call UnitResetSingleCooldown(Unit Targeted, Ability ID)
*                                       unit        integer
*******************************************************************************/
    
    function UnitResetSingleCooldown takes unit u, integer a returns nothing
        local integer i = GetUnitAbilityLevel(u, a)
        if i > 0 then
            call UnitRemoveAbility(u, a)
            call UnitAddAbility(u, a)
            call SetUnitAbilityLevel(u, a, i)
        endif
    endfunction
    
endlibrary

 

[Teelo]

Looks fine to me. Do you have a question? Does it not work?

 

[Dvoth]

Yes it works fine, I just though there might be better ways to do this.

 

[Almia]

the if block should be removed.
Who the hell will check an empty ability to reset it?

 

[Maker]

I think it is good to have the if/then/else there just incase someone tries to reset an ability the unit doesn't have. It adds practially no extra strain on your computer and prevents units getting abilities they don't have.

 

[Dvoth]

I'll have to agree with Maker on this one, it's true that in most cases it will be useless but not all cases.

 

[themerion]

Just going to throw out some thoughts about naming.

Most functions that begin with a unit argument will have Unit in their name, so I would consider:

UnitResetSingleCooldown

The code will be a slight bit more self-documenting if you use something more explicit than 'integer a'. Like, for instance abilityID or abiID. ('unit u' feels like de-facto standard, so that's fine)

function UnitResetSingleCooldown takes unit u, integer abilityID

Also, I know some people say you should always keep short variable names to improve speed. I've never encountered a situation where that would matter to a library function like this (and even if it would matter, you can just use the map optimizer, and get both nice code and a lot more speed than you would get from manually trying to shorten variable names...)

 

[Dvoth]

I change the function name to UnitResetSingleCooldown since I though it made a lot of sense. But I kept "a" as ability id, I added a small description.

Thanks for the feedback.

 

[Teelo]

library ResetSingleCooldown

/******************************************************************************
*   To reset a cooldown just use:
*       call UnitResetSingleCooldown(Unit Targeted, Ability ID)
*                                       unit        integer
*******************************************************************************/
   
    function UnitResetSingleCooldown takes unit u, integer a returns nothing
        local integer i = GetUnitAbilityLevel(u, a)

        if i == 0 then
            return
        endif

        call UnitRemoveAbility(u, a)
        call UnitAddAbility(u, a)
        call SetUnitAbilityLevel(u, a, i)
    endfunction
   
endlibrary

I don't like unneeded if blocks.

 

[Dvoth]

Doesn't that do exactly the same thing? I honestly don't understand the difference, can you please explain further.

 

[edo494]

a little bit (maybe faster, maybe not) different way of doing this is:

library ResetSingleCooldown

/******************************************************************************
*   To reset a cooldown just use:
*       call UnitResetSingleCooldown(Unit Targeted, Ability ID)
*                                       unit        integer
*******************************************************************************/
   
    function UnitResetSingleCooldown takes unit u, integer a returns nothing
        local integer i = GetUnitAbilityLevel(u, a)
        if i > 0 and UnitRemoveAbility(u, a) then
            call UnitAddAbility(u, a)
            call SetUnitAbilityLevel(u, a, i)
        endif
    endfunction
   
endlibrary

I dont know if it is faster but if nothing its the same speed
Dont worry, the UnitRemoveAbility will not fire if i > 0 is not evaluated to true and even if it did, if you dont have the ability, this function returns false and nothing is removed anyway

but because it is so easy I think your way(or my but I would say its the same speed) is the fastest way to do it, unless you are some Jass wizard

 

[Dvoth]

Aaah, ok must be quite hard to see the speed difference. Which ever is faster it's by so little I'm assuming since the code is so short.

 

[edo494]

nah, I dont really think it makes any difference, he must call the function one way or the other, but there may be some hidden speed boost in if block just as there is in Condition block compared to Action block of trigger.
This way its a little bit shorter so it can be more readable but it also depends on how much stuff you have in the condition side of if (if i > 0 ...)

 

[Teelo]

Doesn't that do exactly the same thing? I honestly don't understand the difference, can you please explain further.

Its about code readability. Your way wraps the bulk of your code block around a condition. I prefer to perform the assertions and just return false if they fail, then perform the code block.

Take for example, would you prefer:

if (...) {
action
action
action
action
action
action
action
action
action
action
action
}

or

if (!...) return;
action
action
action
action
action
action
action
action
action
action
action

By wrapping a code block in an if statement, you've got to keep track of nesting it properly. Even with a 3 line code block, you're still leaving room to expand it later. Its just a style, and you were looking for opinions.

 

[Dvoth]

Aaah ok I'm getting what you meant, I probably will keep it as is since I'm used to GUI and it's more similar that way . Thanks for the comments anyways .