[JASS] Frost Shard v1.1d

[Moderator]

04:17, 1st Jan 2013
Magtheridon96: Approved.
Okay, well done.

If you're ever up for it, you can totally convert this thing to use arrays since your indices will fit in the range [0 ... 8191]

Other than that, this is a job well done.

 

[Almia]

Review
1)Because of Mag's review,make all of real functions takes integer level
2)Speed and modifier should be inside real function
3)
if SquareRoot( (x2-x1) * (x2-x1) + (y2-y1) * (y2-y1) ) < 30 then
->
if (x2-x1) * (x2-x1) + (y2-y1) * (y2-y1) < 900 then
This way it will be faster in calculating
4)[jass=]set fx = AddSpecialEffectTarget(FS_Sfx1(), target, "origin")
call DestroyEffect (fx)[/code]
->[/icode]call DestroyEffect(AddSpecialEffectTarget(FS_Sfx1(), target, "origin"))[//jass]
5)^Attachment should be configurable
6)local timer t1 = CreateTimer()
you didnt null this
7)Dont use SetUnitPosition,they are slower.Use SetUnitX/Y
8)Sin(angle) & Cos(angle) should be radians
9)0.03125 cache this into a function
10) set r = GetUnitState (target, UNIT_STATE_LIFE)/GetUnitState (target, UNIT_STATE_MAX_LIFE)
you can cache this
11)There are some lines that have misindention,fix it.

 

[Doomlord]

Review
1)Because of Mag's review,make all of real functions takes integer level
2)Speed and modifier should be inside real function
3)
if SquareRoot( (x2-x1) * (x2-x1) + (y2-y1) * (y2-y1) ) < 30 then
->
if (x2-x1) * (x2-x1) + (y2-y1) * (y2-y1) < 900 then
This way it will be faster in calculating
4)[jass=]set fx = AddSpecialEffectTarget(FS_Sfx1(), target, "origin")
call DestroyEffect (fx)[/code]
->[/icode]call DestroyEffect(AddSpecialEffectTarget(FS_Sfx1(), target, "origin"))[//jass]
5)^Attachment should be configurable
6)local timer t1 = CreateTimer()
you didnt null this
7)Dont use SetUnitPosition,they are slower.Use SetUnitX/Y
8)Sin(angle) & Cos(angle) should be radians
9)0.03125 cache this into a function
10) set r = GetUnitState (target, UNIT_STATE_LIFE)/GetUnitState (target, UNIT_STATE_MAX_LIFE)
you can cache this
11)There are some lines that have misindention,fix it.

1/, 2/ Can you elaborate a bit? I don't really understand
3/, 4/, 5/, 6/, 7/, 8/, 9/, 11/ Fixing now
10/ Huh?

 

[Almia]

1 and 2
1)make all real function has an arguement integer level,so that users can easily configure it,
2)the real locals speed and modifier should be inside a function,because you are using a formula(i think it is an option)
10)put the r inside a function(optional)

 

[Doomlord]

1 and 2
1)make all real function has an arguement integer level,so that users can easily configure it,
2)the real locals speed and modifier should be inside a function,because you are using a formula(i think it is an option)
10)put the r inside a function(optional)

Okay thanks.

 

[Magtheridon96]

• Code

  set t = CreateTimer ()
  set timerID = GetHandleId (t)
  call SaveUnitHandle(FS_HashtableVar(), timerID, 0, caster)
  call SaveUnitHandle(FS_HashtableVar(), timerID, 1, target)
  call SaveUnitHandle (FS_HashtableVar(), timerID, 2, CreateUnit(GetTriggerPlayer(), FS_DummyID(), GetUnitX(caster), GetUnitY(caster), Atan2(GetUnitY(target)-GetUnitY(caster), GetUnitX(target)-GetUnitX(caster))*bj_RADTODEG))
  call TimerStart( t , FS_Interval() , true , function FS_Periodic )

  
  After this block of code, t should be nulled.
• You don't need to inline the TriggerRegisterAnyUnitEventBJ function in the Initialization function. It's not worth it :/
• I'm going to show you how to correctly null and remove leaks.
  
  
  How-to

  
  If you have a block of code like this:
  

  local handle h = something()
  if somethingElse() then
      call DoNothing()
  elseif somethingElse2() then
      if somethingElse3() then
          call DoNothing()
      else
          call DoNothing()
      endif
  endif

  You will notice that I have a ton of branches in here.
  I want to null the handle h. What should I do?
  Well, some people would do this:
  

  local handle h = something()
  if somethingElse() then
      call DoNothing()
      set h = null
  elseif somethingElse2() then
      if somethingElse3() then
          call DoNothing()
          set h = null
      else
          call DoNothing()
          set h = null
      endif
  endif

  This is bad. You should null a handle or anything that extends a handle like a unit or a destructable in the same block in which you created/set it. The correct way is this:
  

  local handle h = something()
  if somethingElse() then
      call DoNothing()
  elseif somethingElse2() then
      if somethingElse3() then
          call DoNothing()
      else
          call DoNothing()
      endif
  endif
  set h = null

  
  Here's another example to clear it up:
  
  

  Example

  
  

  Bad

  local timer t
  if something() then
      set t = GetExpiredTimer()
  endif
  set t = null

  Bad

  local timer t = GetExpiredTimer()
  if something() then
      set t = null
  endif

  Good

  local timer t = GetExpiredTimer()
  if something() then
      // ..
  endif
  set t = null

  Good

  local timer t
  if something() then
      set t = GetExpiredTimer()
      // ...
      set t = null
  endif

  
  Hopefully, you'll be able to apply this information to rethink how you're nulling all your handles in the main function
• WEAPON_TYPE_WHOKNOWS is equal to null, but that's not important.
• You can get rid of the I2R calls inside the configurable functions if you add in a period to the end of any of the numbers in the expressions. (For example, 200 -> 200.)

 

[Losam]

I like it. Nice spell,great job.

 

[Doomlord]

^Updated to add Mag's suggestion.

 

[Furiion52]

This is really good! The initial sfx is nice and the frost shards after are great! The frost shards though look like a mass attack of white stuff, might be more suited if there were less... It is a really good spell
4.5/5