[Spell] Help with spell

[HBT_Smaug]

I've made this spell based on Flame Phoenix tutorial: Making a spell in vJASS.
I need someone to check it for leaks/bugs or any way to make it more efficient so I can put it in my map.
Sorry if I'm breaking any rule, I'm new to HiveWorkShop forums. Thank you

scope Spell initializer Spell
 
//==============================SETUP======================================
    globals
       private constant integer SPELL_ID = 'A001' //rawcode of the ability
       private constant string DAMAGE_EFFECT = "Abilities\\Spells\\Undead\\FrostNova\\FrostNovaTarget.mdl"
    endglobals
    
    private function Range takes integer level returns real
    //returns the range the spell will affect
        return level * 150.
    endfunction
    
    private function Damage takes integer level returns integer
    //returns the damage enemies will take
        return level * (GetHeroInt(GetTriggerUnit(), true ))
    endfunction
    
    private function Targets takes unit target returns boolean
        //the units the spell will affect
        return (GetWidgetLife(target) > 0.405) and (IsUnitType(target, UNIT_TYPE_STRUCTURE) == false) and (IsUnitType(target, UNIT_TYPE_MAGIC_IMMUNE) == false) and (IsUnitType(target, UNIT_TYPE_MECHANICAL) == false)
    endfunction
//==============================SETUP END===================================
    globals
        private group all 
        private boolexpr b 
    endglobals
//============================================================================
    private function Pick takes nothing returns boolean
        return Targets(GetFilterUnit())
    endfunction
//======================Conditions=================================================

    private function Conditions takes nothing returns boolean
        return GetSpellAbilityId() == SPELL_ID
    endfunction

//=======================Actions====================================================

    private function Actions takes nothing returns nothing
        local location spellLoc = GetSpellTargetLoc()
        local real spellX = GetLocationX(spellLoc)
        local real spellY = GetLocationY(spellLoc)
        local unit caster = GetTriggerUnit()
        local integer level = GetUnitAbilityLevel(caster, SPELL_ID)
        local unit f
        
        call GroupEnumUnitsInRange(all, spellX, spellY, Range(level), b)
        
        loop
            set f = FirstOfGroup(all)
            exitwhen(f == null)
            call GroupRemoveUnit(all, f)
            if IsUnitEnemy(f, GetOwningPlayer(caster)) then
                call UnitDamageTarget(caster, f, Damage(level), true, false, ATTACK_TYPE_CHAOS, DAMAGE_TYPE_DIVINE, null)
                call DestroyEffect(AddSpecialEffectTarget(DAMAGE_EFFECT, f, "origin"))
            endif
        endloop
            
        
        
        call RemoveLocation(spellLoc)
        set spellLoc = null
        set caster = null
    endfunction

//====================================================================================
    private function Spell takes nothing returns nothing
        local trigger SpellTrg = CreateTrigger()
        call TriggerRegisterAnyUnitEventBJ(SpellTrg, EVENT_PLAYER_UNIT_SPELL_EFFECT )
        call TriggerAddCondition(SpellTrg, Condition( function Conditions ) )
        call TriggerAddAction( SpellTrg, function Actions )
     
        //setting globals
        set all = CreateGroup()
        set b = Condition (function Pick)
     
        //preloading effects
        call Preload(DAMAGE_EFFECT)
        //preloading Ability 
        set bj_lastCreatedUnit = CreateUnit(Player(PLAYER_NEUTRAL_PASSIVE),'I000', 0, 0, 0)
        call UnitAddAbility(bj_lastCreatedUnit, 'A001')
        call KillUnit(bj_lastCreatedUnit)
    endfunction
endscope

//===========================================================================

 

[Malhorne]

This tutorial can be optimized for sure.
I learned vJASS there too ^^'
Old memories

Anyway some optimizations :
Use native UnitAlive takes unit id returns boolean at the begginning of your code so you'll be able to use this awesome native.
Then in your filter function :

private function Targets takes unit target returns boolean
    //the units the spell will affect
    return (GetWidgetLife(target) > 0.405) and (IsUnitType(target, UNIT_TYPE_STRUCTURE) == false) and (IsUnitType(target, UNIT_TYPE_MAGIC_IMMUNE) == false) and (IsUnitType(target, UNIT_TYPE_MECHANICAL) == false)
endfunction

->

private function Targets takes unit target returns boolean
        //the units the spell will affect
        return UnitAlive(target) and not (IsUnitType(target, UNIT_TYPE_STRUCTURE)) and not (IsUnitType(target, UNIT_TYPE_MAGIC_IMMUNE)) and not (IsUnitType(target, UNIT_TYPE_MECHANICAL))
endfunction

local location spellLoc = GetSpellTargetLoc()
        local real spellX = GetLocationX(spellLoc)
        local real spellY = GetLocationY(spellLoc)

->

local real spellX = GetSpellTargetX()
local real spellY = GetSpellTargetY()

I suggest merging your conditions and actions into your condition like this :

private function Conditions takes nothing returns boolean
    //Declare locals (no initialization of those vars)
    if GetSpellAbilityId() == SPELL_ID then
        //Initialize your locals
        //Do your spell
    endif
    return false
endfunction

 

[HBT_Smaug]

Thank you Malhorne, this will help me improve my JASS/VJASS skills.

 

[Malhorne]

No problem at all.