Grab and Pull [Jass]

[Moderator]

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

14:35, 14th Sep 2013
Maker: Get rid of pause unit and don't use locations, use coordinates.
You have unnulled timer variables so you leak

 

[Maker]

-Do not use locations
-condforcatch leaks unit and timer, null the variables

if ( not ( IsUnitAlly(GetEnumUnit(), GetOwningPlayer(a)) != true ) ) then
        return false
    endif
    if ( not ( GetUnitState(GetEnumUnit(), UNIT_STATE_LIFE) <= 0 != true ) ) then
        return false
    endif
    return true

->

return IsUnitEnemy(GetEnumUnit(), GetOwningPlayer(a)) and not IsUnitType(GetEnumUnit(), UNIT_TYPE_DEAD)

 

[Magtheridon96]

Don't delete and re-upload your spells, use the Update button.
I work really hard on these reviews:

I can see you started out with GUI and did a conversion to JASS, but there are a lot of major flaws with the code.

1. The identation is pretty off, so it's kind of hard to read. Pass your code through this tool to see what good indentation is like.
2. You need to have proper documentation at the top that gives a name, author, version, importing instructions and a virgin's tears.
3. In almost any modern language I can think of, == true is the most redundant thing ever. You can remove it and have the same semantics.
4. Your Spell Conditions function is both sub-optimal and unreadable. Here's a better alternative:
   

   function Trig_spell_Conditions takes nothing returns boolean
       return GetSpellAbilityId() == 'A000'
   endfunction

5. Don't check if a unit is alive or dead based on life, it's terrible. It is perfectly possible to have a dead unit with a million HP.
   A unit is dead if and only if IsUnitType(my_unit, UNIT_TYPE_DEAD) and GetUnitTypeId(my_unit) != 0 is true.
   A unit is alive if the above is false.
6. Instead of using a heavy squareroot call here:
   SquareRoot(dx * dx + dy * dy) >= 140.1
   Just square both sides:
   dx*dx + dy*dy >= 140*140
   SquareRoot is pretty darn slow.
7. You have a ton of location, group and special effect memory leaks.
   To resolve them, you need to cache any location or group you use into a variable and you should remove them with RemoveLocation and DestroyGroup respectively. Special effects can be cleaned up instantly like this:
   call DestroyEffect(AddSpecialEffectTarget("Abilities\\Spells\\Human\\Thunderclap\\ThunderClapCaster.mdl",catched,"origin"))
   In fact, here, you shouldn't be using locations at all. There are much better alternatives that use coordinates directly.
   For example, this: GetLocationY(GetUnitLoc(catched)) can simply be GetUnitY(catched). It's faster and it avoids creating a location that you need to store and destroy. Avoid locations at all costs in this spell and you'll avoid all the location leaks.
8. As for groups, create them, use GroupEnumUnitsInRange, iterate over them with the FirstOfGroup-loop method instead of the ForGroup method and then destroy them, or use a global group that you would create once, and reuse that group when you enumerate instead of creating and destroying a local group.
   
   
   The FirstOfGroup idiom

   
   

   local group g = CreateGroup()
   local unit u
   
   call GroupEnumUnitsInRange(g, x, y, radius, null)
   loop
       set u = FirstOfGroup(g)
       exitwhen u == null
       call GroupRemoveUnit(g, u)
   
       if UnitFilter(u) then
           // Actions with u here
       endif
   endloop

   
   
   This has been proven to be much faster than ForGroup and it gets faster and faster as the number of units gets higher, so it's very scalable as well.
   
9. You need to use constant functions that return constant values in order to allow the user to configure your spell without having to dig in the mud and understand your code.
   
   
   Example

   
   

   constant function SpellPrefix_AbilityId takes nothing returns integer
       return 'A000'
   endfunction
   
   constant function SpellPrefix_GetEffectPath takes nothing returns string
       return "Abilities\\Spells\\Human\\Thunderclap\\ThunderClapCaster.mdl"
   endfunction
   
   constant function SpellPrefix_GetDamage takes integer level returns real
       return 60. + level * 40
   endfunction

   
   
10. Your functions need much better names and you need to follow the standard convention that everyone follows. Give your functions a prefix unique to your spell too so the chances of colliding with other functions in the map code aren't high.
    Read this for a detailed reference of how we should name our functions, constants, variables and the like.

There are probably some optimization issues I haven't looked at, but I can get to those when the code is in a better state.
Cheers.

 

[War3I4i]

The skill works bad without SquareRoot, try it by yourself))) And I corrected the errors about which you told me

thanks for help, i fixed it

 

[Magtheridon96]

Yes, I can see the code is mostly better now, but there are a few things that need to be fixed and I'll list them out in detail eventually.

As for the SquareRoot part, are you 100% sure it's not working?

I mean, it works because:

(√x == y) is a boolean expression exactly equivalent to (x == y^2)

What we're doing is removing the SquareRoot and comparing with the other side squared (140*140)

 

[War3I4i]

yeah it doesn't work good without SquareRoot, try to remove SquareRoot and use skill, it will work bad

 

[Magtheridon96]

Your code doesn't even compile.
You're declaring a local timer in the middle of a function.

Locals can only be declared at the top.
If this actually compiled for you, you're using some weird third party extension.

 

[War3I4i]

jass new gen pack automatically transport local variables at the top, so i dont worry about it

 

[Magtheridon96]

JNGP doesn't do that on it's own in any configuration that I can enable, what are your settings?

 

[War3I4i]

i use only "Enable Jass Helper", it's automatically on parser and optimizes all variables

I use jass new gen pack exp 2

 

[Tank-Commander]

even so, it's bad coding and doesn't compile in the standard world editor as a labelled JASS resource and not vJASS it -should- compile in the standard editor, so even with the understanding that some program may do that, it's not acceptable - not to mention bad practice.

Edit: thought I'd put my comment back in here since this is a delete-reupload, in future, just update it ok?

First: I'd fix the indentation, 0 indentation is really not that great for readability and general conventions
Secondly: Triggers lack proper names
Thirdly: Lacks configuration
Fourthly: Leaks a special effect
Fifthy: 0 documentation/code commenting
Sixthly: Spell lacks a proper name

only thing there no longer accurate is the special effect leak

and not sure if anybody else pointed this out already

set bj_lastCreatedUnit = CreateUnitAtLoc(GetOwningPlayer(a), 'h000', Location(x,y), angle)
call UnitApplyTimedLife( bj_lastCreatedUnit, 'BTLF', 1.50 )
set dammy = bj_lastCreatedUnit

Why not set the variable "dammy" to the created unit (also should probably be "dummy") and scrap the whole bj_lastCreatedUnit

Eidt2: on the subject of being labelled a JASS resource, you have a globals block in the map header, firstly why?, secondly, why? oh and don't double post, use the edit button

 

[War3I4i]

Thank you, i'll try to fix it all. About "dammy". Sorr, i'm Russian and we call it "dammy", not "dummy" ;D

 

[Tank-Commander]

Oh I also should mention on your configuration (i.e. when you say the only thing you need to change is 'A000' you forgot about making the dummy unit configurable (hardcoded as 'h000' atm) just as a basic configuration, of course, most things in this spell should be configurable i.e. special effects, dummy unit duration, etc. etc.

 

[War3I4i]

Oh yes, i forgot about it, thanks

 

[Magtheridon96]

Oh, you're using the unofficial, experimental Version 2.0

I see how that works.
Could you keep the locals at the top so it works for everyone? I'm not sure if this was a good design choice for Version 2.0 and I may need to speak to moyack about it :/

 

[War3I4i]

ok, now i'll transport all variables on top

 

[deathismyfriend]

post your triggers in the spell description.

 

[looking_for_help]

As for the SquareRoot part, are you 100% sure it's not working?

I mean, it works because:

(√x == y) is a boolean expression exactly equivalent to (x == y^2)

What we're doing is removing the SquareRoot and comparing with the other side squared (140*140)

Negative x values?

Edit:
No, checked negative values don't crash the thread so it should behave the same...

 

[FUJI]

Post Triggers Not anyone here is in the PC, I'm at phone

 

[War3I4i]

Done, i posted trigger

 

[deathismyfriend]

“Spell Review Inside“

REVIEW

TRIGGER FIXES

Locations should not be used in jass. Use real values instead. Don't use bj_last created unit. Just do set dummy = CreateUnit() You need to null dummy at end of trigger. This will get overwritten every time. SaveTimerHandle(hash,GetHandleId(key1),1,t) Also when using null simply declare it in the function don't declare a local variable for it. Don’t use TriggerAddAction() use TriggerAddCondition(). Look at my tutorial converting GUI to efficient jass. This leaks a unit group every time. call ForGroup(GetUnitsInRangeOfLocAll(75,GetUnitLoc(dammy)), function catch2) // catch units in range of loc dammy Fix your indentation. Please read the spell submission rules. Don’t use BJs in jass. Use the natives that they call. (all BJs are not bad) This leaks the locals every time it is called. It is also unnecessary and inefficient to store something that is only used once. function condfordead takes nothing returns boolean //death condition local timer t = GetExpiredTimer() // get timer local unit dammy = LoadUnitHandle(hash,GetHandleId(t),1) //load dammy if(not( GetUnitState(dammy, UNIT_STATE_LIFE) <= 0 == true)) then // if dammy is dead then return false endif return true endfunction Here is how it should be. function condfordead takes nothing returns boolean //death condition if( GetUnitState( LoadUnitHandle( hash, GetHandleId( GetExpiredTimer()), 1), UNIT_STATE_LIFE) <= 0 == true) then // call your action trigger from here endif return false endfunction Don’t use GetUnitState for detecting life of unit use GetWidgetLife(). To detect if a unit is dead you can’t check the unit’s health. Units can get healed even when they are dead. Use this to properly detect if a unit is dead. if IsUnitType( unit, UNIT_TYPE_DEAD) or GetUnitTypeId( unit) == 0 then That was from a quick glance I may of missed some things.

FINAL RATING

Needs Fix

[/TD][/tr]
If you have any questions or problems with my review please tell me. Original Review Template by Doomlord.

 

[Maker]

Negative x values?

In this context there will be no negative values as there are n² operations. A real squared will always be positive.

 

[Magtheridon96]

Just to explain how deathismyfriend's check works:

Alive check:
not IsUnitType(u, UNIT_TYPE_DEAD) and GetUnitTypeId(u) != 0
is equal to:
not IsUnitType(u, UNIT_TYPE_DEAD) and not GetUnitTypeId(u) == 0

By De Morgan's Law, (¬A ∧ ¬B) ⇔ ¬(A ∨ B)

So it's equivalent to:

not (IsUnitType(u, UNIT_TYPE_DEAD) or GetUnitTypeId(u) == 0)

And to negate that and get a death check, we just remove the not operator and get:

IsUnitType(u, UNIT_TYPE_DEAD) or GetUnitTypeId(u) == 0

This however, is sub-optimal.

To check if a unit is dead, you do this:
IsUnitType(u, UNIT_TYPE_DEAD)

But do note, if this check returns false, the unit is not necessarily alive.

In fact, doing a life check to detect a dead unit is valid along with the typeId check.
GetWidgetLife(u) <= 0.405 and GetUnitTypeId(u) != 0 is a valid death check.

 

[War3I4i]

Oh guys, i know jass only 3 days.... =(

 

[deathismyfriend]

Oh guys, i know jass only 3 days.... =(

thats ok these reviews are to help improve your skills and make spells to a good efficiency and to make them proper.