Death Coil v1.1c

[Moderator]

12th Dec 2015
IcemanBo: Too long time as NeedsFix. Rejected.

14:10, 4th Mar 2011
Bribe:

Really original name; 0.01 is way too fast for a timer like this, SpellStruct is too inefficient for such a simple usage as you give this, you don't need == true, capitalization on struct member variables? Really bad for readability. GetUnitZ needs to use a static location.

Status: Rejected until updated

 

[Lambdadelta]

I ripped this from my post at TheHelper

The ability reminds me of a dog leash, the target is held on by the obelisk then pulled back if he tries to escape, also reminds me of my other spell Restrain ​

Coding

I like how you use Jesus4Lyf's resources, I haven't seen a spell submitted with them in use :O However, there are numerous things you can improve in this ability:

1. Configuration: The ability has all the number figures hardcoded into the ability, this is bad. It is bad because you force users to go through your code themselves if they want to change any figures. The solution is to create a list of globals / functions that allow the user to easily modify spell values.
   
   Your ability should have these figures configurable:
   • Snap distance (if (SquareRoot(.x*.x+.y*.y)>=800.00) then
   • Teleport effect (call DestroyEffect(AddSpecialEffect("Abilities\\Spells\\Demon\\DarkPortal\\DarkPortalTarget.mdl",.x1,.y1))
   • Damage dealt (call Damage_Spell(.caster,.targetUnit,50*.level+25))
   • Dummy rawcode (set .d=CreateUnit(.owner,'h000',.x2,.y2,0.00))
   • Lightning type (set .l=AddLightning("MFPB",true,.x1,.y1,.x2,.y2))
   • Spell duration (call UnitApplyTimedLife(.d,'BTLF',15.00))
   • Lightning colour (call SetLightningColor(.l,100.00,0.00,0.00,10))
   • Timer period (call .startTimer(.periodic,0.01))
   • Ability rawcode (set thistype.abil='A000')
   
   The globals, being constant, will inline into the ability code so it'll be the same as what you are doing presently.
   .
2. Encapsulation: Your struct is this: struct DeathCoil extends SpellStruct, someone else could name a struct DeathCoil as well, this leads to the names conflicting. The solution is to place the struct inside a scope or library, and then adding private at the front of the struct.
   .
3. Variable Naming:
   
   

   .
       unit d
       real x
       real y
       real x1
       real x2 
       real y1
       real y2
       lightning l

   
   It'd be nice to assist other users checking your code by commenting your code. Variable names such as d[/code], and [icode=jass]l don't really tell me much (ignoring the fact they stand for dummy and lightning) either, but in bigger code, you'll definitely want to name things better. I'd suggest something like dummy and lightning.
   .
4. Damage isn't particularly needed for this ability, it's a commonly used system and is actually quite nice, but I'd just leave it to the user to make the ability run off Damage themselves, besides, you can always make Damage optional.
   .
5. if (SquareRoot(.x*.x+.y*.y)>=800.00) then
   You can remove the square root of this by squaring both sides of the equation. ( .x * .x + .y * .y ) >= (800.00 * 800.00) --> ( .x * .x + .y * .y ) >= (640000.00). However, the value should be configurable as well.

 

[Inferior]

look good so far, except for the fact that you implement 3 systems, for just 1 function call:
call Damage_Spell(.caster,.targetUnit,50*.level+25) can be replaced with
call UnitDamageTarget(.caster,.targetUnit,50.*.level+25,true,false,ATTACK_TYPE_NORMAL,DAMAGE_TYPE_SPELL,null)

 

[Carnerox]

I just updated the things you guys listed.
Edit* Hold that thought, I forgot to add something.
Edit Edit* Okay, now should be fine.

 

[Inferior]

not like this!!

private struct DeathCoil extends SpellStruct
        unit DUMMY
        static integer ABIL_ID='A000'
        integer DUMMY_ID='h000'
        integer DAMAGE=50*.level+25
        string EFFECT="Abilities\\Spells\\Demon\\DarkPortal\\DarkPortalTarget.mdl"
        string LIGHTNING_ID="MFPB"
        attacktype ATTACK_TYPE=ATTACK_TYPE_NORMAL
        damagetype DAMAGE_TYPE=DAMAGE_TYPE_MAGIC
        lightning LIGHTNING
        real DISTANCE=800.00
        real PERIODIC=0.01
        real DURATION=15.00
        real COLOR_R=100.00
        real COLOR_G=0.00
        real COLOR_B=0.00
        real COLOR_A=10.00

use sth like this:

globals
    private constant integer ABIL_ID = 'A000'
    private constant real     MAX_OFFSET = 800.
    .....
endglobals

 

[Carnerox]

Updated.

 

[Inferior]

why do your methods take an integer level when you use the level via SpellStruct.

Convert them into method operators:

method operator Damage takes nothing returns real
    return 50.00*.level+25.00
endmethod
        
method operator Distance takes nothing returns real
    return 100*.level+400.00       
endmethod
        
method operator Duration takes nothing returns real            
    return 5.00*.level+10.00
endmethod

 

[Carnerox]

Ah, okay.. I never knew about the operator part.

 

[baassee]

As I dislike using SpellStruct I like this spell. The idea is kinda cool actually though the lightning should really have some height as it is now along the ground (maybe that was the purpose but I haven't found any line about it yet).

 

[Carnerox]

Updated!

 

[tjordell]

This looks pretty nice and high coded xD Although I cant try the map because I'm on my iPhone xD Keep it up man!

 

[Carnerox]

Thanks.

I plan to make a version without SpellStruct as well.

 

[baassee]

Gosh, SpellStruct. Damn you at TheHelper

It wouldn't be that hard to replace it? They told me to even make a plain JASS version of a hero revival.

 

[Carnerox]

Just updated it, with the version that doesn't use SpellStruct.

 

[baassee]

Very nice one!

 

[Carnerox]

Thanks.

 

[watermelon_1234]

Short Review:

• You should only move the lightning if it doesn't get destroyed. Just add an else to your if and move that action there.
  Ex:
  

              if (SquareRoot(DeathCoil.x*DeathCoil.x+DeathCoil.y*DeathCoil.y)>=DeathCoil.Distance(DeathCoil.LEVEL)) then
                  ...
              elseif (IsUnitType(DeathCoil.DUMMY,UNIT_TYPE_DEAD)==true) or (IsUnitType(DeathCoil.TARGET,UNIT_TYPE_DEAD)==true) then
                  ...
              else
                  call MoveLightningEx(DeathCoil.LIGHTNING,true,DeathCoil.x1,DeathCoil.y1,DeathCoil.z1,DeathCoil.x2,DeathCoil.y2,DeathCoil.z2)
              endif

• Minor improvement: You can use GetTriggerPlayer instead of GetOwningPlayer(dc.CASTER)
  It's faster and easier to type. ;P
• It's a bit confusing to have configuration functions inside the struct and then tell the user "Do not change anything in this struct!"
  If I were you, I would just have the configuration functions as separate from the struct, and use constant functions that take in variables.
  
  Of course, this is just a matter of preference and appearance so it's not all too important. XP

This is only on the code without SpellStruct:

• It's a bad idea to not destroy the struct when you're done with it.
• What you're doing with TIMER is not a good idea. This tutorial should give you a better idea of how to do this.
• The only thing that should be static is TIMER. Everything else being static causes the spell to be not MUI.
• I'm unsure about the code with SpellStruct, but I think you should have an onDestroy method since there are some things that are repeated when the spell should be done. Specifically, everything under the check for whether the dummy or target is dead can be put in the onDestroy method and anything repeated in the above check can be omitted. This is just to cut down on redundancy.

 

[Lambdadelta]

SpellStruct version:

• The library should have SpellStruct as a requirement.

• There is some sort of unset standard in which struct members are camelCase, local variables are lowercase and constant globals are CAPITAL_CASE. I don't see why you name the dummy and lightning members in your struct as DUMMY & LIGHTNING. It looks terrible!

Anyways, this is how I want your configuration section to look like;

Code

Fuck you Hiveworkshop JASS tags

    globals
        private constant integer    ABIL_ID		    = 'A000'	// Rawcode of the ability.
        private constant integer    DUMMY_ID	    = 'h000'	// Rawcode of the obelisk.

        private constant real 	    PERIODIC	    = 0.01		// How often the timer will run the function.

        private constant real 	    COLOR_R		    = 100.00	// Lightning red
        private constant real 	    COLOR_G		    = 0.00		// Lightning green
        private constant real       COLOR_B		    = 0.00		// Lightning blue
        private constant real 	    COLOR_A		    = 10.00		// Lightning alpha
        private constant real 	    FLOAT		    = 45.00		// Lightning height
        private constant string     LIGHTNING_ID    = "MFPB"	// Lightning type - Mana Flare

        private constant string 	EFFECT		    = "Abilities\\Spells\\Demon\\DarkPortal\\DarkPortalTarget.mdl"

        private constant attacktype ATTACK_TYPE	    = ATTACK_TYPE_NORMAL
        private constant damagetype DAMAGE_TYPE	    = DAMAGE_TYPE_MAGIC
        private constant boolean 	DAMAGE_MELEE    = true
        private constant boolean 	DAMAGE_RANGED   = false
    endglobals

Code

 

[baassee]

I don't get it BlackRose, you always fail with the JASS tags

Haha I didn't even notice that this was made in libraries, believed that it was scopes

 

[Lambdadelta]

I don't get it BlackRose, you always fail with the JASS tags

Haha I didn't even notice that this was made in libraries, believed that it was scopes

I don't know They clearly hate me.

 

[SanKakU]

pretty cool spell but it uses a lot of systems that i don't know how to use...in effect...i don't know how the spell does what it does.

 

[Carnerox]

I just updated the spell.

 

[SanKakU]

did you fix problem with non spellstruct version? i thinkonly the spell struct version worked...

 

[Bribe]

        method GetUnitZ takes unit u returns real
            local location Point=Location(GetUnitX(u),GetUnitY(u))
            local real Real=GetLocationZ(Point)+GetUnitFlyHeight(u)
            call RemoveLocation(Point)
            set Point=null
            return Real
        endmethod

Really need a global location. Not only, but why not use GetUnitLoc? This not only wastefully uses dynamic locations but does it inefficiently

Why post the code twice?