[JASS] How can I improve my function?

[FaceJaraxxus]

Hello! I'm doing a dynamic description system for crunchy tooltips. It's called every 6th seconds and works like a charm. However, what ways can I improve it?

globals
    string array gladQ
endglobals

function GladDescriptionQ takes nothing returns nothing
local unit u = gladiatorHero
local integer abilityId = 'A05F'
local ability a = BlzGetUnitAbility(u, abilityId)
local integer i = GetUnitAbilityLevel(u, abilityId)

set gladQ[i - 1]  = "Charge towards a target and deal |c00ff0000" + I2S(R2I(eldritchBonus * 16 + (i*4))) + "|r damage upon impact.|n|nAfterwards the Gladiator receives a 90% damage reduction for 4 seconds. During this time, its attack and movement speed is increased by|cff00ff00 " + I2S(MathRound(R2I(eldritchBonus))) + "%.|r|n
|c0000ffffCooldown: 10 seconds.|c006666ff
Mana: " + I2S(25 + i*5) +".|r |n|n"

call BlzSetAbilityExtendedTooltip(abilityId, gladQ[i - 1], i - 1)
call IncUnitAbilityLevelSwapped(abilityId, u)
call DecUnitAbilityLevelSwapped(abilityId, u)
endfunction

I'm thinking that I don't actually need to use a string array. I could just have it be a local string that's being declared, and subsequently change

        call BlzSetAbilityExtendedTooltip(abilityId, gladQ[i - 1], i - 1)

this line.

Also, is it worth declaring a local unit variable instead of referencing the global unit variable like I'm doing now?

local unit u = gladiatorHero

Anything more I could improve? Thanks!

UPDATE

I adapted to Uncle's solution! Thanks to this, my functions are less spread out. Also, I did not think of converting the math variables into strings before "printing" them, which is super helpful.

library Gladiator

    globals
    unit gladiatorHero = null
    real eldritchBonus = 0.0
    private integer array heroAbility
    endglobals

    private function GladiatorUpdateQ takes integer lvl returns string
        local string damage = I2S(R2I((lvl * 4 + 16) * eldritchBonus))
        local string speed = I2S(MathRound(R2I(eldritchBonus)))
        local string mana = I2S(25 + lvl * 5)
        local string s = "Charge towards a target and deal |c00ff0000" + damage + "|r damage upon impact.|n|nAfterwards the Gladiator receives a 90% damage reduction for 4 seconds. During this time, its attack and movement speed is increased by |cff00ff00" + speed + "%.|r|n|n|c006666ffMana cost: " + mana + "|n|c0000ffffCooldown: 10 seconds.|r"
        set damage = null
        set speed = null
        set mana = null
        return s
    endfunction

    private function GladiatorUpdateW takes integer lvl returns string
    local string damage = I2S(R2I(eldritchBonus * (10 + 2*lvl)))
    local string cooldown = I2S(10 - lvl)
        local string s = "The Gladiator's next attack within 4 seconds will strike the target's soul, dealing an additional |cff00ff00" + damage + " damage|r. The damage is increased by 1% for every percent of health the Gladiator is missing.|n|n|cff8080ffMana cost: 20.|r|n|cff00ffffCooldown: " + cooldown + " seconds.|r"
    set damage = null
    set cooldown = null
        return s
    endfunction

    private function GladiatorUpdateE takes integer lvl returns string
    local string lifesteal = I2S(R2I(15 + eldritchBonus))     
    local string eBonus = I2S(R2I(eldritchBonus))
    local string s = "Dealing damage to an enemy restores |c0000ff00" + lifesteal + "%|r of damage as healing to the Gladiator. Effective healing is increased by 50% in Crypts and 100% against the Floor Boss.|n|nThe Gladiator's abilities scale with |c0000ff00Eldritch Bonus|r which is equal to |c0000ff00" + eBonus + ".|r"
    set lifesteal = null
    set eBonus = null
        return s
    endfunction

    function GladiatorUpdateAbility takes integer abilityIndex returns nothing
        local integer id = heroAbility[abilityIndex]
        local integer lvl = GetUnitAbilityLevel(gladiatorHero, id)

        if (lvl == 0) then
            return
        endif

        if (id == heroAbility[0]) then
            call BlzSetAbilityExtendedTooltip(id, GladiatorUpdateQ(lvl), lvl - 1)
        elseif (id == heroAbility[1]) then
            call BlzSetAbilityExtendedTooltip(id, GladiatorUpdateW(lvl), lvl - 1)
        elseif (id == heroAbility[2]) then
            call BlzSetAbilityExtendedTooltip(id, GladiatorUpdateE(lvl), lvl - 1)
        endif

        call IncUnitAbilityLevelSwapped(id, gladiatorHero)
        call DecUnitAbilityLevelSwapped(id, gladiatorHero)
    endfunction

    function GladiatorSetup takes unit u returns nothing
        set gladiatorHero = u
        set heroAbility[0] = 'A05F' // Q
        set heroAbility[1] = 'A0D9' // W
        set heroAbility[2] = 'A05E' // E
    endfunction

endlibrary

GladiatorSetup is called when the Gladiator hero is picked and passed along. E.g.

//function for checking picked hero
if hero_id == 'N03M' then //Unit type id of the hero
call GladiatorSetup(u)
endif

I currently call all three updates at the same time, every 6 seconds because that's when I update the Gladiator's Eldritch Bonus variable.
First: I have a function which handles the Eldritch Bonus:

function GladDesc_Actions takes nothing returns nothing
// declare locals
    if gladiatorHero == null then
             return
     else
        if GetUnitAbilityLevel(gladiatorHero, 'A05E') >= 1 then // If hero exists, but hasn't grabbed the ability yet then we don't do anything

        // Eldritch Bonus code

        call GladDesc_Handler() //We call handler to update the descriptions
        endif
    endif
endfunction

After setting the bonus, it calls the handler. The handler will update all functions that depend on the Eldritch Bonus.

//Locals and other calls
call GladiatorUpdateAbility(0)
call GladiatorUpdateAbility(1)
call GladiatorUpdateAbility(2)

Note that I am calling the same function thrice with different input. Here is an area to improve my handler.
1. I could add checks to not have to call make all calls in case they are not learned. But since we instantly return if the ability is not learned, it shouldn't be a large concern.
2. I could just call the GladiatorUpdateAbility function and make it update all learned abilities. Here's how it could look like:

    function GladiatorUpdateAbility takes nothing returns nothing
        local integer lvl = 0

    if GetUnitAbilityLevel(gladiatorHero, heroAbility[0]) >= 1 then
        set lvl = GetUnitAbilityLevel(gladiatorHero, heroAbility[0])
            call BlzSetAbilityExtendedTooltip(heroAbility[0], GladiatorUpdateQ(lvl), lvl - 1)
        call IncUnitAbilityLevelSwapped(heroAbility[0], gladiatorHero)
            call DecUnitAbilityLevelSwapped(heroAbility[0], gladiatorHero)
    endif

    if GetUnitAbilityLevel(gladiatorHero, heroAbility[1]) >= 1 then
        set lvl = GetUnitAbilityLevel(gladiatorHero, heroAbility[1])
            call BlzSetAbilityExtendedTooltip(heroAbility[1], GladiatorUpdateW(lvl), lvl - 1)
        call IncUnitAbilityLevelSwapped(heroAbility[1], gladiatorHero)
            call DecUnitAbilityLevelSwapped(heroAbility[1], gladiatorHero)
    endif
    
    if GetUnitAbilityLevel(gladiatorHero, heroAbility[2]) >= 1 then
        set lvl = GetUnitAbilityLevel(gladiatorHero, heroAbility[2])
            call BlzSetAbilityExtendedTooltip(heroAbility[2], GladiatorUpdateE(lvl), lvl - 1)
        call IncUnitAbilityLevelSwapped(heroAbility[2], gladiatorHero)
            call DecUnitAbilityLevelSwapped(heroAbility[2], gladiatorHero)
        endif

    endfunction

Which means that we only call it once in the handler without passing any arguments.

call GladiatorUpdateAbility()

Both ways of doing it works. If we're updating a single ability's description during specific events, such as ability level up, then the first solution to update a single description is better. For updating all at once, the second one will be helpful, yet it will obviously look more cluttered since we're repeating the same code in multiple if-statements.

Any concerns about the code? Would you have done it differently?

 

[Uncle]

You don't need the Array and the local variable is unnecessary.

Perhaps expand this to a library dedicated to the Gladiator hero, assuming there can be only one at a time:

library HeroGladiator

    globals
        private unit Hero
        private integer array Hero_Abilities
    endglobals

    private function GladiatorUpdateQ takes integer lvl returns string
        local string dmg = I2S(R2I(eldritchBonus * 16 + (lvl * 4)))
        local string speed = I2S(MathRound(R2I(eldritchBonus)))
        local string mana = I2S(25 + lvl * 5)

        local string s = "Charge towards a target and deal |c00ff0000" + dmg + "|r damage upon impact.|n|nAfterwards the Gladiator receives a 90% damage reduction for 4 seconds. During this time, it's attack and movement speed is increased by |cff00ff00" + speed + "%.|r|n|c0000ffffCooldown: 10 seconds. |c006666ffMana: " + mana + ".|r|n|n"

        set dmg = null
        set speed = null
        set mana = null
        return s
    endfunction

    private function GladiatorUpdateW takes integer lvl returns string
        local string s = "Unimplemented W."
        return s
    endfunction

    private function GladiatorUpdateE takes integer lvl returns string
        local string s = "Unimplemented E."
        return s
    endfunction

    private function GladiatorUpdateR takes integer lvl returns string
        local string s = "Unimplemented R."
        return s
    endfunction

    function GladiatorUpdateAbility takes integer abilityIndex returns nothing
        // abilityIndex uses 0 = Q, 1 = W, 2 = E, 3 = R

        local integer id = Hero_Abilities[abilityIndex]
        local integer lvl = GetUnitAbilityLevel(Hero, id)

        // Exit early if it's unlearned
        if (lvl == 0) then
            return
        endif

        if (id == Hero_Abilities[0]) then
            call BlzSetAbilityExtendedTooltip(id, GladiatorUpdateQ(lvl), lvl - 1)
        elseif (id == Hero_Abilities[1]) then
            call BlzSetAbilityExtendedTooltip(id, GladiatorUpdateW(lvl), lvl - 1)
        elseif (id == Hero_Abilities[2]) then
            call BlzSetAbilityExtendedTooltip(id, GladiatorUpdateE(lvl), lvl - 1)
        elseif (id == Hero_Abilities[3]) then
            call BlzSetAbilityExtendedTooltip(id, GladiatorUpdateR(lvl), lvl - 1)
        endif

        call IncUnitAbilityLevelSwapped(id, Hero)
        call DecUnitAbilityLevelSwapped(id, Hero)
    endfunction

    function GladiatorSetup takes unit u returns nothing
        set Hero = u
        set Hero_Abilities[0] = 'A05F' // Q
        set Hero_Abilities[1] = 'A05F' // W
        set Hero_Abilities[2] = 'A05F' // E
        set Hero_Abilities[3] = 'A05F' // R
    endfunction

endlibrary

With this design, you would assign your Gladiator upon selecting it (so when it first comes into play):

• Events
  • Unit - A unit Enters (Playable map area)
• Conditions
  • (Unit-Type of (Triggering unit)) Equal to Gladiator
  • ((Triggering unit) is an Illusion) Equal to False
• Actions
  • Trigger - Turn off (this trigger)
  • Custom script: call GladiatorSetup( GetTriggerUnit() )

Then ideally you would update it's settings whenever a related change is made:

• Events
  • Unit - A unit Learns a skill
• Conditions
  • (Learned hero skill) Equal to Charge (Gladiator - Q)
• Actions
  • Custom script: call GladiatorUpdateAbility(0)

^ That would update his Q ability for example. You could also do it every 6 seconds, it shouldn't be an issue.

You can add any other Gladiator specific functions and variables here. Note that this isn't necessarily ideal, if your heroes use much of the same logic then using Structs and/or Hashtables to avoid repetition could be a pretty nice way to avoid repeating yourself.

 

[FaceJaraxxus]

You don't need the Array and the local variable is unnecessary.

Perhaps expand this to a library dedicated to the Gladiator hero, assuming there can be only one at a time:

library HeroGladiator

    globals
        private unit Hero
        private integer array Hero_Abilities
    endglobals

    private function GladiatorUpdateQ takes integer lvl returns string
        local string dmg = I2S(R2I(eldritchBonus * 16 + (lvl * 4)))
        local string speed = I2S(MathRound(R2I(eldritchBonus)))
        local string mana = I2S(25 + lvl * 5)

        local string s = "Charge towards a target and deal |c00ff0000" + dmg + "|r damage upon impact.|n|nAfterwards the Gladiator receives a 90% damage reduction for 4 seconds. During this time, it's attack and movement speed is increased by |cff00ff00" + speed + "%.|r|n|c0000ffffCooldown: 10 seconds. |c006666ffMana: " + mana + ".|r|n|n"

        set dmg = null
        set speed = null
        set mana = null
        return s
    endfunction

    private function GladiatorUpdateW takes integer lvl returns string
        local string s = "Unimplemented W."
        return s
    endfunction

    private function GladiatorUpdateE takes integer lvl returns string
        local string s = "Unimplemented E."
        return s
    endfunction

    private function GladiatorUpdateR takes integer lvl returns string
        local string s = "Unimplemented R."
        return s
    endfunction

    function GladiatorUpdateAbility takes integer abilityIndex returns nothing
        // abilityIndex uses 0 = Q, 1 = W, 2 = E, 3 = R

        local integer id = Hero_Abilities[abilityIndex]
        local integer lvl = GetUnitAbilityLevel(Hero, id)

        // Exit early if it's unlearned
        if (lvl == 0) then
            return
        endif

        if (id == Hero_Abilities[0]) then
            call BlzSetAbilityExtendedTooltip(id, GladiatorUpdateQ(lvl), lvl - 1)
        elseif (id == Hero_Abilities[1]) then
            call BlzSetAbilityExtendedTooltip(id, GladiatorUpdateW(lvl), lvl - 1)
        elseif (id == Hero_Abilities[2]) then
            call BlzSetAbilityExtendedTooltip(id, GladiatorUpdateE(lvl), lvl - 1)
        elseif (id == Hero_Abilities[3]) then
            call BlzSetAbilityExtendedTooltip(id, GladiatorUpdateR(lvl), lvl - 1)
        endif

        call IncUnitAbilityLevelSwapped(id, Hero)
        call DecUnitAbilityLevelSwapped(id, Hero)
    endfunction

    function GladiatorSetup takes unit u returns nothing
        set Hero = u
        set Hero_Abilities[0] = 'A05F' // Q
        set Hero_Abilities[1] = 'A05F' // W
        set Hero_Abilities[2] = 'A05F' // E
        set Hero_Abilities[3] = 'A05F' // R
    endfunction

endlibrary

With this design, you would assign your Gladiator upon selecting it (so when it first comes into play):

• Events
  • Unit - A unit Enters (Playable map area)
• Conditions
  • (Unit-Type of (Triggering unit)) Equal to Gladiator
  • ((Triggering unit) is an Illusion) Equal to False
• Actions
  • Trigger - Turn off (this trigger)
  • Custom script: call GladiatorSetup( GetTriggerUnit() )

Then ideally you would update it's settings whenever a related change is made:

• Events
  • Unit - A unit Learns a skill
• Conditions
  • (Learned hero skill) Equal to Charge (Gladiator - Q)
• Actions
  • Custom script: call GladiatorUpdateAbility(0)

^ That would update his Q ability for example. You could also do it every 6 seconds, it shouldn't be an issue.

You can add any other Gladiator specific functions and variables here. Note that this isn't necessarily ideal, if your heroes use much of the same logic then using Structs and/or Hashtables to avoid repetition could be a pretty nice way to avoid repeating yourself.

Great suggestion and thank you for the confirmations! This is structurally better and more readable than the functions I planned. It also works with how my current function calls are set up. However, what's the point of having the possibility of exiting early via Return? Since we're taking e.g. ability[0], lvl will never be 0?

 

[Uncle]

Great suggestion and thank you for the confirmations! This is structurally better and more readable than the functions I planned. It also works with how my current function calls are set up. However, what's the point of having the possibility of exiting early via Return? Since we're taking e.g. ability[0], lvl will never be 0?

You said you were calling this once every 6 seconds. If that's the case, I added a failsafe in case your Hero doesn't actually have the ability yet.

 

[FaceJaraxxus]

You said you were calling this once every 6 seconds. If that's the case, I added a failsafe in case your Hero doesn't actually have the ability yet.

Ah all right I understand! One more question, is there any specific reason for making the declared globals private other than making them inaccessible outside of the library? As I understand it, the idea is that this library was made as a non-specific set-up for other heroes as well.

 

[Uncle]

Ah all right I understand! One more question, is there any specific reason for making the declared globals private other than making them inaccessible outside of the library? As I understand it, the idea is that this library was made as a non-specific set-up for other heroes as well.

I'm using private because you're meant to interface with the public functions. But of course you could make Hero accessible if you'd like, there'd be no harm in that, just name it something that isn't so generic.

But the library is currently dedicated to the Gladiator and would not support an additional Hero. Currently, you would create a new copy of the Library in a separate script and replace all instances of Gladiator with your new Hero's name as well as edit any functions that have Hero specific data like GladiatorUpdateQ/W/E/R.

If you wanted it to be more generic then you should use a Struct and/or Hashtables like I mentioned before. I don't really know how to do it well in Jass, Lua would be pretty easy though. At the end of the day you will need function calls that are unique to each Hero, unless you create a really elaborate string + data formatting system but that's a large undertaking. Or maybe there's an easier solution, I'm not too sure.

 

[FaceJaraxxus]

I'm using private because you're meant to interface with the global functions. But of course you could make Hero accessible if you'd like, there'd be no harm in that, just name it something that isn't so generic.

But the library is currently dedicated to the Gladiator and would not support an additional Hero. Currently, you would create a new copy of the Library in a separate script and replace all instances of Gladiator with your new Hero's name as well as edit any functions that have Hero specific data like GladiatorUpdateQ/W/E/R.

If you wanted it to be more generic then you should use a Struct and/or Hashtables like I mentioned before. I don't really know how to do it too well in Jass, Lua would be pretty easy though. At the end of the day you will need function calls that are unique to each Hero, unless you create a really elaborate string + data formatting system but that's a large undertaking. Or maybe there's an easier solution, I'm not too sure.

Oh okay! I get it now!

I understand that it's meant for the hero, but my idea was like you suggested: that it can be copied and repurposed for different heroes by changing the name of things. So thank you for explaining, the current way of using a library was exactly what I needed.

I have updated my post, first implementing your solution then adapting it to my current event handling. Hope it doesn't disappoint