Moderator
M
Moderator
BPower:
11:02, 25th Feb 2016
Reason for re-review:
Nowadays the spell section is packed full of missile systems from different authors, therefore a more qualified
moderator comment than "this is neat stuff" is required. There is keen competition, hence your new
review may not be as good as it was before.
Criticism:
I don't know why a DC was taken into consideration in the past, obviously not out of objective reasons.
I depreciate the rating form 5/5 to 2,5/5. As we give out integer ratings you'll get a 3/5.
The entire system is good for own usage. For public submission
I would like to see a proper API documentation.
12:32, 8th Apr 2010
TriggerHappy:
I have not yet tested the system.
I have tested the system and the demo map is really lacking.
If you want to show people the power of your system include some really awesome eyecandy examples.
The_Reborn_Devil:
Due to Trigger's demotion I am now the only spell mod and thus I can finally review your system.
Hmm, it would seem my review is gone, but the system is great.
Status: Approved
Rating: Highly Recommended [Director's Cut]
hvo-busterkomo: After evaluating this system I do not think it meets the standards to be a Director's Cut. However, in respect to The_Reborn_Devil, I have left it as Highly Recommended.
11:02, 25th Feb 2016
Reason for re-review:
Nowadays the spell section is packed full of missile systems from different authors, therefore a more qualified
moderator comment than "this is neat stuff" is required. There is keen competition, hence your new
review may not be as good as it was before.
Criticism:
- A system released for public usage needs a proper documentation, at least about its API.
You didn't write a single line of comments for other users. Compare it to the JassHelper
without Jass Helper Manual or the WorldEditor with neither blizzard.j nor common.j.
--- - It's very unusual to use a timer interval, which can't be multiplied with an integer to a result of 1.
The most common timer interval in JASS is 1/32 which equals 0.03125.
You usepublic static real period = 0.03175
which is ~1/31.49
--- TRUE
andFALSE
are constants fortrue
andfalse
.
It's not required to use them. I guess you know that.
---GetUnitState(theUnit, UNIT_STATE_LIFE) > 0.405
could be optimized toGetWidgetLife(theUnit) > 0.405
---- I guess it's your very own style of writting code, but your functions are arranged in a way, that the
Jass Helper must generate a lot of pseudo-code to obtain a valid function order.
Just look into MissileUnitHit. Functions are placed precise in the wrong order. Jass Helper fixes that, but for which price?
--- - The unit enumeration radius is inaccurate, but that's the case in most missile systems.
You must add the maximum collision size used in the map to get all units in collision range.
--- - You hide units which are below terrain level, very neat, but where do you restore the full range
of the locust effect after showing the unit? Btw In that function you have a unit leak.
--- set .destHitGroup[GetHandleId(theDest)] = 1
is good, not perfect. Handle ids get recycled once there
is not reference to their object and the object is removed from the map. Therefore it must be
set .destHitGroup.destructable[GetHandleId(theDest)] = theDest
.
One of many reasons why Vexorians Table is compared to Bribes not perfect in API.
---public method checkMissileHit takes nothing returns nothing
does nothing, but takes space.
---- The math used in missile movement is cool. It's unfair to only criticise your work.
--- - Array handle variables should be nulled once no longer needed.
--- - Missile cast helper should be a seperate library named CastHelper. We have that better executed namely SpellEffectEvent.
--- - Overall a bit unread-able code and even harder to use for someone who didn't code the system by him/her-self.
I don't know why a DC was taken into consideration in the past, obviously not out of objective reasons.
I depreciate the rating form 5/5 to 2,5/5. As we give out integer ratings you'll get a 3/5.
The entire system is good for own usage. For public submission
I would like to see a proper API documentation.
12:32, 8th Apr 2010
TriggerHappy:
I have not yet tested the system.
- You might as well comment out ParabolaZ2 as it serves the same purpose as the first one. The only reason is should even be in the library is to give a better explanation as to how the first one works. so yeah.. just comment it out (/* */).
- Your globals are private, don't give them prefixes (CM_NAME).
- You could store
(2. * bj_PI * CM_PERIOD)
in a constant instead of doing the operation each time. - Is there really a need for the local in enumDestsInRange?
- You are using a group per instance, either A) recycle them or B) use a global group.
- Is there a reason why you're creating the timer in onInit instead of just initializing it?
- It would be more efficient if you placed the contents of all your update functions directly in the move method. Yes it's going to be harder to read but just make some comments explaining them. Because currently you are doing a bunch of unnecessary function calls.
- The system name is horrible. Just by using a system not made by blizzard already indicates that it's custom. You might was well call it Missile.
I have tested the system and the demo map is really lacking.
If you want to show people the power of your system include some really awesome eyecandy examples.
- Still need to recycle groups.
- Your comments explaining the interface use the wrong grammar. It's ran, not runned.
- You still need to remove the prefixes on your globals.
- Why don't you just make locRect static? As of now you are creating and destroying rects when you could just be re-using the same one.
- You realize you are destroying enumGroup twice, right?
The_Reborn_Devil:
Due to Trigger's demotion I am now the only spell mod and thus I can finally review your system.
Hmm, it would seem my review is gone, but the system is great.
Status: Approved
Rating: Highly Recommended [Director's Cut]
hvo-busterkomo: After evaluating this system I do not think it meets the standards to be a Director's Cut. However, in respect to The_Reborn_Devil, I have left it as Highly Recommended.