Summon the Knight of Light

[MyPad]

Not bad for a first spell submission. A slave bound to his master through his lifeforce serves in life as he does in death.
While the summoned unit can be killed without any issues, killing the summoner will immediately cut off the lifeforce the
summoned unit is infused with.

Spoiler: Code Review

There's a lot of things to tackle here, so this might take a while.

• Since the spell code is already encapsulated within a scope, it is assumed that all of the functions within the scope
  are private. That being said, the functions contained are not private. This can be addressed by adding a "private"
  keyword before the functions themselves. Similarly, this can be applied with the global variables as well.
  
  
• In the initialization function KnightOfLight, the trigger which detects the summoning event is created twice, once
  at the declaration of the variable and another right after that. The second declaration should be removed since that
  results in an unused trigger being generated.
  
  

  // Added a private keyword before the function
  private function KnightOfLight takes nothing returns nothing
      local trigger t = CreateTrigger()
      local integer index
      // Please remove the line below
      // set t = CreateTrigger()
      // ...
  endfunction

  
  
• In the same function, there's no need to inline what the BJ function TriggerRegisterAnyUnitEventBJ already does.
  Simply call that function instead, since any gain in performance from the inlined version is negated by the prospect
  of running only once.
  
  
• In JASS, the usage of location handles is highly discouraged, since one can work directly with coordinates instead.
  
  
• The usage of two hashtables for implementing the spell mechanics is odd, since hashtable usage can be avoided in
  most cases. Aside from that, this will leave the user with two less hashtables to use, since there is a limit on the
  number of hashtables that can be generated.
  
  
• In the trigger action that runs on a summoning event KnightOfLight_Activacion_Acciones, there are a lot of things
  that must be addressed.

  •     //Variables Locales
        local unit array u
        local location array p
        local integer n
        local integer i
        local lightning r
        local effect e
        //Fin de Variables Locales

    
    In most cases, I would gloss over the short variable names if their purpose is clearly understandable at a first glance.
    However, using arrays like this leads to confusion on what the variable represents (or references). For example, consider
    two local unit variables named "summoned" and "summoner". From a glance, it is immediately clear what each variable
    represents, making it easier to adjust the code as necessary. This is not easily achievable with local array variables, where
    one has to figure out the contents at a specific index, making it more difficult to update the code.
    
    

  •     if ( GetUnitTypeId(GetSummonedUnit()) == 'h000' ) then
            // Guardado de tablas hash
            // Invocador
            set u[1] = GetSummoningUnit()
            // Invocacion
            set u[2] = GetSummonedUnit()
            // Inicio de invocacion animada
            // Metadatos
            set p[1] = GetUnitLoc(u[1])
            set p[2] = GetUnitLoc(u[2])
            set i = GetHandleId(u[1])
            set e = LoadEffectHandle(KnightOfLightTabla,i,StringHash("AlasConjurador"))
            set u[3] = LoadUnitHandle(KnightOfLightTabla,i,StringHash("Invocacion"))
            call KillUnit(u[3])
            call DestroyEffect( e )
            call DestroyLightning(LoadLightningHandle(KnightOfLightTabla,i,StringHash("Relampago")))
            call FlushChildHashtable(KnightOfLightTabla,i )
            call GroupAddUnit( KnightOfLightGrupo, u[1] )
            // Efectos especiales
            set e = AddSpecialEffectTarget("HolyBlastEfecto.mdl", u[1], "chest")
            set r = AddLightningEx("DRAB", true, GetUnitX(u[1]), GetUnitY(u[1]), GetLocationZ(p[1]) + 50, GetUnitX(u[2]), GetUnitY(u[2]), GetLocationZ(p[2]) + 150)
            call SaveBoolean( triggerT, StringHash("Relampagos"), StringHash("Caballero de luz"), true)
            call AddUnitAnimationProperties( u[2], "alternate", true )
            call SetUnitAnimation( u[2], "morph" )
            // Fin de los efectos especiales
            call EnableTrigger( LoadTriggerHandle( triggerT, StringHash("Detonadores"), StringHash("KnightOfLightHechizo") ) )
            call SaveEffectHandle( KnightOfLightTabla, i, StringHash("AlasConjurador"), e)
            call SaveUnitHandle( KnightOfLightTabla, i, StringHash("Invocacion"), u[2] )
            call SaveLightningHandle(KnightOfLightTabla, i, StringHash("Relampago"), r )
            // Limpiando fugas
            set e = null
            set n = 1
            loop
                exitwhen n == 3
                call RemoveLocation(p[n])
                set p[n] = null
                set n = n + 1
            endloop
            set n = 1
            loop
                exitwhen n == 4
                set u[n] = null
                set n = n + 1
            endloop
        else
            call DisplayTextToForce( GetPlayersAll(), "TRIGSTR_017" )
        endif

    
    Suppose that the summoning event runs, and the unit type summoned does not match. This will result in a message
    being printed ("no unidad"). This appears to be a debug message that was not removed. Removing that debug message
    is recommended.
    
    The conditional statement can be rewritten such that the action terminates if the condition is not satisfied. This leads
    to the following rewrite
    
    

        if ( GetUnitTypeId(GetSummonedUnit()) != 'h000' ) then
            return
        endif
        // Guardado de tablas hash
        // Invocador
        set u[1] = GetSummoningUnit()
        // Invocacion
        set u[2] = GetSummonedUnit()
        // Inicio de invocacion animada
        Metadatos
        set p[1] = GetUnitLoc(u[1])
        set p[2] = GetUnitLoc(u[2])
        set i = GetHandleId(u[1])
        set e = LoadEffectHandle(KnightOfLightTabla,i,StringHash("AlasConjurador"))
        set u[3] = LoadUnitHandle(KnightOfLightTabla,i,StringHash("Invocacion"))
        call KillUnit(u[3])
        call DestroyEffect( e )
        call DestroyLightning(LoadLightningHandle(KnightOfLightTabla,i,StringHash("Relampago")))
        call FlushChildHashtable(KnightOfLightTabla,i )
        call GroupAddUnit( KnightOfLightGrupo, u[1] )
        // Efectos especiales
        set e = AddSpecialEffectTarget("HolyBlastEfecto.mdl", u[1], "chest")
        set r = AddLightningEx("DRAB", true, GetUnitX(u[1]), GetUnitY(u[1]), GetLocationZ(p[1]) + 50, GetUnitX(u[2]), GetUnitY(u[2]), GetLocationZ(p[2]) + 150)
        call SaveBoolean( triggerT, StringHash("Relampagos"), StringHash("Caballero de luz"), true)
        call AddUnitAnimationProperties( u[2], "alternate", true )
        call SetUnitAnimation( u[2], "morph" )
        // Fin de los efectos especiales
        call EnableTrigger( LoadTriggerHandle( triggerT, StringHash("Detonadores"), StringHash("KnightOfLightHechizo") ) )
        call SaveEffectHandle( KnightOfLightTabla, i, StringHash("AlasConjurador"), e)
        call SaveUnitHandle( KnightOfLightTabla, i, StringHash("Invocacion"), u[2] )
        call SaveLightningHandle(KnightOfLightTabla, i, StringHash("Relampago"), r )
        // Limpiando fugas
        set e = null
        set n = 1
        loop
            exitwhen n == 3
            call RemoveLocation(p[n])
            set p[n] = null
            set n = n + 1
        endloop
        set n = 1
        loop
            exitwhen n == 4
            set u[n] = null
            set n = n + 1
        endloop

    
    

  •     set n = 1
        loop
            exitwhen n == 3
            call RemoveLocation(p[n])
            set p[n] = null
            set n = n + 1
        endloop
        set n = 1
        loop
            exitwhen n == 4
            set u[n] = null
            set n = n + 1
        endloop

    
    When the number of entries within these array variables are that small, it would be better to inline
    them (typically ranging from 2 to 8), saving up an unnecessary "addition" operation, as well as a
    comparison operation.
    
    
  • I think it would be more convenient if the resulting values from the StringHash functions are stored
    in "static" variables ("static" variables being constant variables that are assigned at runtime). That
    said, it would be preferable that the StringHash function is not used at all.
    
    

  •     call SaveBoolean( triggerT, StringHash("Relampagos"), StringHash("Caballero de luz"), true)    call AddUnitAnimationProperties( u[2], "alternate", true )
        call SetUnitAnimation( u[2], "morph" )
        // Fin de los efectos especiales
        call EnableTrigger( LoadTriggerHandle( triggerT, StringHash("Detonadores"), StringHash("KnightOfLightHechizo") ) )

    
    The trigger stored at triggerT can be stored directly in a global trigger variable instead, reducing unnecessary
    overhead from accessing contents from the triggerT hashtable (which appears to be a wasted hashtable once
    optimized).
• In the trigger action that runs on a periodic event (0.03 seconds) KnightOfLight_Hechizo_AccionesDeGrupo, there are a lot of things
  that must be addressed.
  • When getting the amount of life of a unit, you can use the following native instead to optimize performance:
    
    

    native GetWidgetLife takes widget w returns real
    native SetWidgetLife takes widget w, real amount returns nothing

    
    What the native above does is get the amount of health of the widget handle. This widget handle can accept
    units, as well as destructables and items. Similarly, SetWidgetLife sets the amount of health of the widget.
    
    
  • The DistanceBetweenPoints function involves the usage of a SquareRoot function, which can be quite costly in
    a periodic timer with a short interval. If you'd like to optimize this,
    consider comparing the square of the distance with the square of the maximum distance (which in this case is
    800.00*800.00). Otherwise, you can skip this one.
    
    
  • Since the number of units in the group changes only upon death or summoning event, this means that the
    number of units can be easily determined with the usage of a global counter variable (integer). Knowing this,
    one can easily avoid calling the function CountUnitsInGroup, a function that runs in O(n) time which is called
    O(n) times in the periodic, leading to a time complexity of O(n^2), which is not good when dealing with
    a lot of instances.
• Instead on relying on an internal timer for the periodic event, a timer handle can be created directly to handle the
  same periodic event, freeing up a trigger handle for use.
  
  
• Configurability wise, this spell is a rigid fossil compared to most, having mostly hardcoded constants in place of configurable
  variables or functions. Some examples of hardcoded constants/chunks which can be made configurable:
  • Function KnightOfLight_Activacion_Acciones
    
    

    // What if the spell summoned a unit with a different rawcode?
    // This means that the value at the right hand side is a configurable value
    // and should be easily modifiable for the user. Right now, the value
    // at the right hand side is hardcoded to be 'h000'
    if ( GetUnitTypeId(GetSummonedUnit()) == 'h000' ) then
        //...
    //...

    
    

    if ( GetUnitTypeId(GetSummonedUnit()) == 'h000' ) then
        //...
        // The attachment effect might not be desirable for some, so
        // assume that they are willing to change the effect. In this
        // case, the path to the model (and the attachment point)
        // is/are hardcoded value/s.
        set e = AddSpecialEffectTarget("HolyBlastEfecto.mdl", u[1], "chest")
    
        // Similarly, the lightning model might not be to the liking of the user, so
        // assume that they are willing to change that.
        set r = AddLightningEx("DRAB", true, GetUnitX(u[1]), GetUnitY(u[1]), GetLocationZ(p[1]) + 50, GetUnitX(u[2]), GetUnitY(u[2]), GetLocationZ(p[2]) + 150)
        // ...
    // ...

    
    
  • Function KnightOfLight_Hechizo_AccionesDeGrupo
    
    

    if( GetUnitState(u[1], UNIT_STATE_LIFE) > 0 ) and ( GetUnitState(u[2], UNIT_STATE_LIFE) > 0 ) then
            set p[1] = GetUnitLoc(u[1])
            set p[2] = GetUnitLoc(u[2])
            // Perhaps the cut-off range is too long or too short.
            // In any case, this range can be considered a configurable
            // value.
            if ( ( DistanceBetweenPoints(p[1], p[2]) <= 800.00 ) ) then
                call MoveLightningEx(r, true, GetUnitX(u[1]), GetUnitY(u[1]), GetLocationZ(p[1]) + 50, GetUnitX(u[2]), GetUnitY(u[2]), GetLocationZ(p[2]) + 150)
                call SaveBoolean( triggerT, StringHash("Relampagos"), StringHash("Caballero de luz"), true)
            else
            // ...
            // At this point, it is a stretch to say that this chunk is configurable
            // but suppose that the user might want something different to happen, like keeping
            // the summoned unit within range only instead of forcing the summoned unit to
            // reposition close to the summoner. Hence, the following chunk can be moved
            // either into a function or a textmacro.
            call SetUnitPositionLoc(u[2],p[3] )
            call SetUnitFacing(u[2], GetUnitFacing(u[1]))
            call IssueTargetOrder( u[2], "smart", u[1] )
    // ...

Due to the number of issues pointed out in the review, the bundle shall be Awaiting Update