Please give feedback to my first jass spell

[Grymlax]

2 days ago I started reading some Jass tutorials and now after a lot of time and frustration I’m ready to post my first Jass spell. The purpose of this post is feedback so please tell me everything I did wrong and everything I missed to clean.

thanks!

Oh right, I should perhaps also mention that there are no cool special effects or anything else fancy with this spell for that matter, at all. Just so you don't get disappointed if you take a look at the spell. This spell was made only so that I would get a better understanding of jass.

[COLOR="Orange"]B[/COLOR]oost of Life
While channeling the spell the hero will restore a certain % of the health points he is missing.
[COLOR="Orange"]Level 1[/COLOR] - Restores 50%
[COLOR="Orange"]Level 2[/COLOR] - Restores 75%
[COLOR="Orange"]Level 3[/COLOR] - Restores 100%

Gui/jass: Jass (finally )
Leakless: dunno (this is one of the things i need help with)
lagless: yupp
mui: yes :shades:

here is the code and further down on the post there is an attachment if you wanna see the spell in action.

function Trig_Boost_of_life_Actions takes nothing returns nothing
    local integer Index = 0
    local unit Caster = GetTriggerUnit()
    local real Missinglife = ( ( GetUnitState(Caster, UNIT_STATE_MAX_LIFE) - GetUnitState(Caster, UNIT_STATE_LIFE) ) * ( ( 1 + (GetUnitAbilityLevel(Caster, 'A000')) ) / 40.00 ) )
    local texttag Missinglifetext
    if ( GetSpellAbilityId() == 'A000' ) then
        loop
            exitwhen Index > 10
            if (GetUnitCurrentOrder(Caster) == 852600 ) then
                call SetUnitState(Caster, UNIT_STATE_LIFE, GetUnitState(Caster, UNIT_STATE_LIFE) + Missinglife)  
                set Missinglifetext = CreateTextTag()
                call SetTextTagText(Missinglifetext, ("+") + I2S(R2I(Missinglife)), 0.023)
                call SetTextTagPosUnit(Missinglifetext, Caster, 0.00)
                call SetTextTagColor(Missinglifetext, 50, 225, 50, 255)
                call SetTextTagVelocity(Missinglifetext, 0, 0.036)
                call SetTextTagLifespan(Missinglifetext, 3.00)
                call SetTextTagFadepoint(Missinglifetext, 2.00 )
                call SetTextTagPermanent(Missinglifetext, false)
                call SetTextTagVisibility(Missinglifetext,true)
                call TriggerSleepAction( 0.96 )
            else
            endif
            set Index = Index + 1
        endloop
    set Caster = null
    set Missinglifetext = null
    else
    endif
endfunction

//===========================================================================
function InitTrig_Boost_of_life takes nothing returns nothing
    set gg_trg_Boost_of_life = CreateTrigger(  )
    call TriggerRegisterAnyUnitEventBJ( gg_trg_Boost_of_life, EVENT_PLAYER_UNIT_SPELL_EFFECT )
    call TriggerAddAction( gg_trg_Boost_of_life, function Trig_Boost_of_life_Actions )
endfunction

 

[Flare]

Hmm, the code looks fine (can't find any leaks), but could be better if a timer was used instead of waits (even if you just made a non-MUI version with a timer) since it's alot more accurate than a wait.

And, if you want to get a unit's life more easily, just use GetWidgetLife (whichUnit). You can't get max life, but it's shorter than GetUnitState (whichUnit, UNIT_STATE_LIFE)

Also, you could use the GetUnitCurrentOrder (whichunit) == 852600 as an exit condition for the loop instead of an if within the loop e.g.

loop
exitwhen Index > 10 or GetUnitCurrentOrder (whichunit) != 852600
//Do your actions
endloop

As long your exitwhen's return a boolean, there shouldn't be a problem.

Just so you don't get disappointed if you take a look at the spell. This spell was made only so that I would get a better understanding of jass.

Can't see why anyone would be disappointed. IMO, it's a good start. I'll test the spell now (I'm curious to see how 10x floating texts stacked on each other is gonna look)

EDIT: Just tested, and it's pretty nice I was wrong about the text tags stacking on top of each other Good job

 

[Grymlax]

thanks for the feedback Flare. as this is not a spell i'm going to submit anywhere i think i will leave it as it is and just try to change the things you pointed out in future spells and triggers.

thanks again. (rep added)

 

[Kazuga]

Add a modification table (or whatever it's now called) so that people who does not know jass can modify the amount healed and easaly change the ability raw code. Something like this:

globals
private constant integer raw      = 'A004'            //Ability id
private constant real    delay    = 1                //Time between actions
private constant string ChannelEffect = "Abilities\\Spells\\Items\\AIil\\AIilTarget.mdl" //Special effect
endglobals

Nice spell btw.

 

[Grymlax]

I will think of that next time. thanks

 

[Larcenist]

A global block with customizable data is not only to help people with no JASS experiance, but also to help you. If you ever write a system of some kind you'll notice how using global blocks will help you modify that system of yours by simply putting global variables instead of random numbers here and there.

 

[AdamGriffith]

Why don't you have this as a condition? =S

( GetSpellAbilityId() == 'A000' )

Instead you are setting the locals each time even if the spell isn't the right id. So if it is the wrong spell you are just setting and then removing some variables... seems pointlesss.

 

[Grymlax]

A global block with customizable data is not only to help people with no JASS experiance, but also to help you. If you ever write a system of some kind you'll notice how using global blocks will help you modify that system of yours by simply putting global variables instead of random numbers here and there.

good to know

Why don't you have this as a condition? =S

Well i just tried to keep my functions to a minimum. guess i forgot what vexorian said in his tut.

Functions are your friends, remember.