| 01-15-2010, 10:31 AM | #1 | |
Just wandering. Is there anything I can improve in my coding? The small snippet it requires here;
 JASS://+-------------------------------------------- //| Water Cyclone v1.00d by BlackRose //+-------------------------------------------- //| Requires: //+---------- //| TimerUtils by Vexorian //| AutoFly by Azlier //| xefx by Vexorian //| xepreload by Vexorian //| //| Description //+------------ //| Becomes the core of a vicious cyclone, waves of water swirl in, //| sweeping in enemy units. Upon reaching the eye, the water violently //| explodes, releasing enemy units swept in a violent nova outwards. //| //| Other: //+-------- //| Uses a modified model of the NagaDeath effect. For performance issues. //+---------------------------------------------------------------------------+ library WaterCyclone initializer onInit requires TimerUtils, AutoFly, xefx, xepreload globals private constant integer SPELL_ID = 'A000' private constant boolean PRELOAD = true private constant boolean DAMAGE_OVERTIME = false private constant boolean SETXY = false // Use Position or XY. Position stops orders. private constant real TIMER_INTERVAL = 0.03125 // Each x do stuff. private constant integer FIREWORKS_NUMBER = 35 // How many firework effects are made when done spinning in. private constant string FIREWORKS_EFFECT = "Abilities\\Weapons\\SpiritOfVengeanceMissile\\SpiritOfVengeanceMissile.mdx" // Main model of the effect. private constant string FIREWORKS_FLASH = "" // Constant flash on effect. private constant real FIREWORKS_SCALE = 1.00 private constant string ON_LANDING_EFFECT = "war3mapImported\\NagaDeathSpare.mdx"//"" private constant boolean CREATE_ON_DUMMY = false private constant real MIN_DIST = 200. // How far the missiles can go. private constant real MAX_DIST = 600. // Random calculations. private constant real DURATION_DIVIDOR = 280. // Duration is DIST / 280 private constant real HEIGHT_MULTIPLIER = 1.1 // Height is DIST * 1.1 private constant real AOE = 900 // Well, not really AoE. How far the spirals start from the caster. private constant real ENUM_AOE = 200 // Radius enemy units can be pulled into the washing machine (Cyclone). private constant real ENUM_HEIGHT = 90 // Max height units can be to get pulled. private constant integer SPIRAL_NUMBER = 3 // How many spirals lines there are. private constant real SPININ_DURATION = 1.5 // How long it take to spiral inwards. private constant real ANGLE_INCREMENT = 5 // Positive for clockwise, negative for anti-clockwise. This spell is indeed a toilet. private constant real HEIGHT = 45. // Height of the effect moving in. If you set one. private constant string INWARDS_EFFECT = "" // Missile of the spiral. In this case it's none and just constant water flashing. private constant string FLASH_EFFECT = "war3mapImported\\NagaDeathSpare.mdx" private constant real INWARDS_SCALE = 1.00 // Missile (INWARDS_EFFECT) size? private constant attacktype ATK_TYPE = ATTACK_TYPE_NORMAL private constant damagetype DMG_TYPE = DAMAGE_TYPE_MAGIC private constant weapontype WPN_TYPE = null endglobals // First half is dealt over time during the spin. // Second half is dealt upon the landing. // If DAMAGE_OVERTIME is true. private function DAMAGE takes integer Level returns real return Level * 125. endfunction // ========================================================= // NOT CONFIGURABLE // ========================================================= globals private constant group ENUM_GROUP = CreateGroup() private constant group INGROUP = CreateGroup() private group TEMP_GROUP private unit TEMP_UNIT private real TX private real TY endglobals native UnitAlive takes unit u returns boolean // ================================================================ // Which units can be added to the swirl. // Currently it is: // -Enemy units of caster, // -Alive units // -Non structures. // -Units under the height limit. // -And units not already in a swirl group. // ================================================================ private keyword WCData globals private WCData DATA endglobals private function UnitFilter takes nothing returns boolean local unit u = GetFilterUnit() if IsUnitEnemy( u, DATA.Owner ) and /* */ UnitAlive( u ) and /* */ IsUnitType( u, UNIT_TYPE_STRUCTURE ) == false and/* */ GetUnitFlyHeight( u ) < ENUM_HEIGHT and /* */ IsUnitInGroup( u, INGROUP ) == false then call GroupAddUnit( INGROUP, u ) call GroupAddUnit( DATA.DamagedGroup, u ) call GroupAddUnit( TEMP_GROUP, u ) endif set u = null return false endfunction // Credit to AceHart, moyack. Which one? I'm confused. private function GetParabolaZ takes real x,real d,real h returns real return 4 * h * x * (d - x) / (d * d) endfunction private struct FireworksData private timer t boolean IsUnitDummy = false real Distance real Height real Duration unit Dummy unit Source real DamageDealt = 0 real DamageMax real DPS real x real y real z real Speed real Cosine real Sine real d = 0 xefx myfx private method onDestroy takes nothing returns nothing // If we didn't have the spell DAMAGE_OVERTIME. Damage the 1/2 now. static if not DAMAGE_OVERTIME then call UnitDamageTarget( .Source, .Dummy, .DPS, true, true, ATK_TYPE, /* */ DMG_TYPE, WPN_TYPE ) // If it was not a dummy, detect how much damage was leftover and deal it. else if .IsUnitDummy == false then set .DPS = .DamageMax - .DamageDealt call UnitDamageTarget( .Source, .Dummy, .DPS, true, true, ATK_TYPE, /* */ DMG_TYPE, WPN_TYPE ) endif endif // If it was a effect, destroy it and create effect. if .IsUnitDummy then static if CREATE_ON_DUMMY then call DestroyEffect( AddSpecialEffect( ON_LANDING_EFFECT, .x, .y ) ) endif call .myfx.destroy() // Else it was a real unit and we make effect, add pathing, clean up. else call DestroyEffect( AddSpecialEffect( ON_LANDING_EFFECT, .x, .y ) ) call SetUnitPathing( .Dummy, true ) call GroupRemoveUnit( INGROUP, .Dummy ) set .Dummy = null set .Source = null endif call ReleaseTimer( .t ) endmethod private static method move takes nothing returns nothing local thistype d = GetTimerData( GetExpiredTimer() ) set d.d = d.d + d.Speed set d.x = d.x + d.Speed * d.Cosine set d.y = d.y + d.Speed * d.Sine set d.z = GetParabolaZ( d.d, d.Distance, d.Height ) if d.IsUnitDummy == false then if SETXY then call SetUnitX( d.Dummy, d.x ) call SetUnitY( d.Dummy, d.y ) else call SetUnitPosition( d.Dummy, d.x, d.y ) endif call SetUnitFlyHeight( d.Dummy, d.z, 0 ) else set d.myfx.x = d.x set d.myfx.y = d.y set d.myfx.z = d.z if FIREWORKS_FLASH != "" then call d.myfx.flash( FIREWORKS_FLASH ) endif endif if DAMAGE_OVERTIME and d.IsUnitDummy == false then call UnitDamageTarget( d.Source, d.Dummy, d.DPS, true, false, ATK_TYPE, /* */ DMG_TYPE, WPN_TYPE ) set d.DamageDealt = d.DamageDealt + d.DPS endif if d.d >= d.Distance then call d.destroy() endif endmethod static method create takes real x, real y, unit Target, unit Source, real DPS returns thistype local thistype d = thistype.allocate() local real angle = GetRandomReal( 0, 359 ) * bj_DEGTORAD set d.Distance = GetRandomReal( MIN_DIST, MAX_DIST ) set d.Duration = d.Distance / DURATION_DIVIDOR set d.Height = d.Distance * HEIGHT_MULTIPLIER set d.x = x set d.y = y set d.z = 0 set d.Speed = d.Distance / ( d.Duration / TIMER_INTERVAL ) set d.Cosine = Cos( angle ) set d.Sine = Sin( angle ) if Target == null then set d.IsUnitDummy = true set d.myfx = xefx.create( x, y, angle ) set d.myfx.fxpath = FIREWORKS_EFFECT else set d.IsUnitDummy = false set d.Dummy = Target call SetUnitPathing( d.Dummy, false ) set d.Source = Source set d.DPS = DPS static if DAMAGE_OVERTIME then set d.DamageMax = d.DPS * (SPININ_DURATION / TIMER_INTERVAL ) endif endif set d.t = NewTimer() call SetTimerData( d.t, d ) call TimerStart( d.t, TIMER_INTERVAL, true, function thistype.move ) return 0 endmethod endstruct private struct WCData unit Caster player Owner xefx array myfx[SPIRAL_NUMBER] real array Cosine[SPIRAL_NUMBER] real array Sine[SPIRAL_NUMBER] group array Group[SPIRAL_NUMBER] group DamagedGroup real Radius = AOE real RadianAdder real RadiusDecrement real cx real cy real DPS timer t private static method GroupCallback takes nothing returns nothing local unit u = GetEnumUnit() local FireworksData f = FireworksData.create( DATA.cx, DATA.cy, u, DATA.Caster, DATA.DPS ) static if not DAMAGE_OVERTIME then call UnitDamageTarget( DATA.Caster, u, DATA.DPS, true, true, /* */ ATK_TYPE, DMG_TYPE, WPN_TYPE ) endif set u = null endmethod private method onDestroy takes nothing returns nothing local integer i = 0 local FireworksData f call ReleaseTimer( .t ) if INWARDS_EFFECT != "" then loop exitwhen i == SPIRAL_NUMBER call .myfx[i].destroy() set i = i + 1 endloop endif set i = 0 loop exitwhen i == FIREWORKS_NUMBER set f = FireworksData.create( .cx, .cy, null, null, 0 ) set i = i + 1 endloop set DATA = this call ForGroup( .DamagedGroup, function thistype.GroupCallback ) call GroupClear( .DamagedGroup ) set .Caster = null set .Owner = null endmethod private static method create takes nothing returns thistype local thistype d = thistype.allocate() local real x local real y local integer i = 0 local real angle = 360 / SPIRAL_NUMBER set d.t = NewTimer() set d.Caster = GetTriggerUnit() set d.Owner = GetOwningPlayer( d.Caster ) set d.cx = GetUnitX(d.Caster) set d.cy = GetUnitY(d.Caster) set d.RadiusDecrement = ( AOE / ( SPININ_DURATION/TIMER_INTERVAL) ) set d.RadianAdder = ANGLE_INCREMENT * bj_DEGTORAD set d.DPS = DAMAGE( GetUnitAbilityLevel( d.Caster, SPELL_ID ) ) / 2 static if DAMAGE_OVERTIME then set d.DPS = d.DPS / ( SPININ_DURATION / TIMER_INTERVAL ) endif loop exitwhen i == SPIRAL_NUMBER set d.Cosine[i] = ( bj_DEGTORAD * (angle*i) ) set d.Sine[i] = ( bj_DEGTORAD * (angle*i) ) set x = d.cx + AOE * Cos(d.Cosine[i]) set y = d.cy + AOE * Sin(d.Sine[i]) if INWARDS_EFFECT != "" then set d.myfx[i] = xefx.create( x, y, HEIGHT ) set d.myfx[i].fxpath = INWARDS_EFFECT endif if d.Group[i] == null then set d.Group[i] = CreateGroup() else call GroupClear( d.Group[i] ) endif set i = i + 1 endloop if d.DamagedGroup == null then set d.DamagedGroup = CreateGroup() else call GroupClear( d.DamagedGroup ) endif call SetTimerData( d.t, d ) return d endmethod static method MoveXY takes nothing returns nothing local unit u = GetEnumUnit() call SetUnitX( u, TX ) call SetUnitY( u, TY ) static if DAMAGE_OVERTIME then call UnitDamageTarget( DATA.Caster, u, DATA.DPS, true, true, /* */ ATK_TYPE, DMG_TYPE, WPN_TYPE ) endif set u = null endmethod private static method update takes nothing returns nothing local thistype d = GetTimerData(GetExpiredTimer()) local real x local real y local integer i = 0 local unit u loop exitwhen i == SPIRAL_NUMBER set d.Cosine[i] = d.Cosine[i] + d.RadianAdder set d.Sine[i] = d.Sine[i] + d.RadianAdder set x = d.cx + d.Radius * Cos(d.Cosine[i]) set y = d.cy + d.Radius * Sin(d.Sine[i]) if INWARDS_EFFECT != "" then set d.myfx[i].x = x set d.myfx[i].y = y endif if FLASH_EFFECT != "" then call DestroyEffect( AddSpecialEffect( FLASH_EFFECT, x, y ) ) endif set DATA = d set TEMP_GROUP = d.Group[i] call GroupEnumUnitsInRange( ENUM_GROUP, x, y, ENUM_AOE, function UnitFilter ) set TX = x set TY = y set DATA = d call ForGroup( d.Group[i], function thistype.MoveXY ) set i = i + 1 endloop set d.Radius = d.Radius - d.RadiusDecrement if d.Radius <= 0 then call d.destroy() endif endmethod static method CheckSpell takes nothing returns boolean if GetSpellAbilityId() == SPELL_ID then call TimerStart( WCData.create().t, TIMER_INTERVAL, true, function WCData.update ) endif return false endmethod endstruct private function onInit takes nothing returns nothing local trigger t = CreateTrigger() static if PRELOAD then call XE_PreloadAbility( SPELL_ID ) endif call TriggerRegisterAnyUnitEventBJ( t, EVENT_PLAYER_UNIT_SPELL_EFFECT ) call TriggerAddCondition( t, Condition( function WCData.CheckSpell ) ) endfunction endlibrary |
| 01-15-2010, 12:44 PM | #2 |
| 01-15-2010, 01:32 PM | #3 | |
Quote:
|
| 01-15-2010, 01:49 PM | #4 |
JASS:set d.Distance = GetRandomReal( 200, 600) set d.Duration = d.Distance / 280 set d.Height = d.Distance * 1.1 This must be outlined into constants! :) |
| 01-15-2010, 03:02 PM | #5 |
Well, hopefully you aren't offended if you wind up with a lot of nitpicky comments, but since you made this thread for educational purposes you should be prepared for that. If you keep updating the code in the original post, I'll keep posting improvements until I don't see any left. For now: 1.) Using AutoFly is a bad idea. It only saves two lines of code in any spell in exchange for an extra requirement. Not to mention that AutoFly itself could be replaced with 10 lines of code:  JASS:2.) There is no real reason for this to be a scope instead of a library. You should not depend on scopes in your coding since they are a feature that Vexorian has mentioned would not exist for Zinc/Boa/vJASS2. That's because they aren't useful for anything important, and they allow you to get away with not explicitly naming your requirements. 3.) There's no need to intent everything by 1 tab just because it's inside a scope/library. 4.) Preloading should probably be optional using static ifs. 5.) You don't need to store a boolexpr inside a global, and you don't need to type Condition() when you're passing a code argument to a native that takes a boolexpr/filterfunc. 6.) You don't need to null a trigger that will never be destroyed, as you do in the onInit method. 7.) It's not good for performance to declare local variables within functions used as conditions for whether a spell will run or not. You can remove the need for the local by making the call SetTimerData( d.t, d ) statement within the create method of WCData, then use this to start the timer: call TimerStart( WCData.create().t, TIMER_INTERVAL, true, function WCData.update ) 8.) Even if your structs are not exposed to the user, you should strive for as much encapsulation as possible, because it makes your code more understandable. Making as many of the methods and members private as possible allows the reader to know at a glance where they are used or not used. 9.) Combining 7 and 8, if you make the timer private, you can't access it outside of the struct as you do currently. Solution: Make CheckSpell a static method of the WCData struct. 10.) You can convert this spell to use one timer rather than many, but if you need help with that specifically I'll save it for a later post. (This is not an exhaustive list, just things I've noticed right now.) Most of these comments are not even related to the actual code, just the structure and design of it. I think that the biggest difference between an intermediate coder and a good one is the ability to structure and design their code properly. Focusing on that will be the best way to improve your skills. If you can take the kind of changes you make here and apply it to future code, you should be able to get your coding "up to standard." |
| 01-15-2010, 04:08 PM | #6 |
| 01-16-2010, 12:28 AM | #7 | ||||||||||||||
Oh my, I was playing around with the spell and pasted it before looking at it first. Note the effect being fireball o.o. Quote:
2. Some poeple tell me to use it, others tell me to not as it messses up with optimizer. Dunno about this one. 3. Now that I think of it. It wouldn't be equal, since units might get put into the swirl at different times, resulting in different damage. This means the caster would have to stand at a far range for maximum damage? If you did mean like set it to be able to deal 20 damage per second? -- Quote:
http://www.thehelper.net/forums/showthread.php?t=106212 I dunno. I got the formula from that thread. -- Quote:
Quote:
Quote:
Quote:
Quote:
Quote:
Quote:
Quote:
Quote:
Quote:
-- Quote:
Quote:
|
| 01-16-2010, 12:33 AM | #8 |
| 01-16-2010, 06:15 AM | #9 |
| 01-16-2010, 09:56 AM | #10 |
JASS:if not DAMAGE_OVERTIME then call UnitDamageTarget( .Source, .Dummy, .DPS, true, true, ATK_TYPE, /* */ DMG_TYPE, WPN_TYPE ) // If it was not a dummy, detect how much damage was leftover and deal it. elseif .NotDummy then set .DPS = .DamageMax - .DamageDealt call UnitDamageTarget( .Source, .Dummy, .DPS, true, true, ATK_TYPE, /* */ DMG_TYPE, WPN_TYPE ) endif JASS:static if not DAMAGE_OVERTIME then call UnitDamageTarget( .Source, .Dummy, .DPS, true, true, ATK_TYPE, /* */ DMG_TYPE, WPN_TYPE ) else if .NotDummy then set .DPS = .DamageMax - .DamageDealt call UnitDamageTarget( .Source, .Dummy, .DPS, true, true, ATK_TYPE, /* */ DMG_TYPE, WPN_TYPE ) endif endif |
| 01-16-2010, 10:36 AM | #11 |
| 01-20-2010, 01:48 AM | #12 |