MooseCharge

[KILLCIDE]

@Cokemonkey11 or @Frotty

Could you guys leave a comment on this? I can't really tell if this is "approvable" based on the judgement.

 

[Chaosy]

A few things.

1. It has a lot of non-standard dependencies which is OK but not ideal
2. There is a huge config file but the main code file still has a lot of configeration
3. There is too much documentation, it completely ruins readability. It is very discouraging to me. I suggest good naming of functions and variables so that a comment for every single variable and function is not needed. I'd also argue that most wurst users are somewhat experienced so explaining that changing a units x/y values is updating the position.
4. The concept is a complete mess, which leads to confusing code. It tries to do so many things at once.

Code seems to follow a good structure with user of the cascade operator and proper indention.
I did not take a super-deep look but it seems like the spell performs well in game and mainly suffers from things that are not directly tied to the spell itself but rather how it is built.

I think it would be wrong to put it in the substandard section since you can see that a lot of dedication has gone into the spell and it performs well in game.
If mod rating was still a thing I'd suggest a lower score until some of the structure is fixed.

@KILLCIDE
You could just take a look at the spell review since Codemonkey was the one that reviewed it.

 

[KILLCIDE]

Yeah I tried reading it, but the lingo is far beyond my understanding

 

[Cokemonkey11]

My view is that this isn't good enough quality to suggest for re-use (but on the other hand I have that view about lots of approved spells).

@DD_legionTN I'm happy to do some code reviews for you if you're interested to get this approved and it isn't.

 

[DD_legionTN]

Sorry for not being reply quickly and thanks for the review @Chaosy.

I'm trying to figure out what would be the good way to comment to turn it into more readable code. But I'm unsure when should I putting comment and when shouldn't. So if you guys could give some input I would be greatly appreciated.

@Cokemonkey11 Yes, please do. I would like to know what the mistake I've made Oh and, I would reply to your contest judging at here.

Overall I had a hard time reviewing all of this.

Sorry for that

There's just so much code and the signal-to-noise ratio is exhausting

It that means too much documentation?

My main suggestion is to improve your comment quality. Get feedback and focus on this.

This is exactly the problem I had atm.

some leaks - can easily make stack trace with some exploratory testing. there is even some commented out resource management code...

Well if you mean for the group iteration, then yes. That time I still pretty new to Wurst and doesn't know how to iterate units with group
Commented out (if you mean the iterator.close()) is because of overflow warning of "double free" of group when using iterator.close()

some resource management feels a bit fragile even if it looks correct. You can improve this by keeping your functions small and simplifying resource management as much as possible. I shouldn't have to scroll up and down to see where a dynamic resource is fetched/freed.

which are them?

no configuration for damageTarget impl is sad, given how much work you put into making everything configurable

Do you mean configuration for dealing damage to enemy/building?

very, very verbose comments, mostly of the form "now do this". Say why, not what.

Ok I'm pretty much have no comment on this...

Here's an excellent example of why not to use magic numbers:

//issued order id not equal to "undefense" (582056)
if c.contains and c.sp == SpellPhase.Channel and GetIssuedOrderId() != 852056

Well I'm still not familiar with stdlib enough at that time (even for now), now I've know that there is undefend in Order.wurst

I think the concept was a bit simplistic - it's more the theme/jokes that are nice here.

The contest submission from my team is purely joke

Visual effects a bit basic

Yes because originally I should be finding better sfx (and also with the original plan that should have some attachment points like horn/leg), but well I may just stick with current model

and default config is a bit crazy,

Right, would like to ask what kind of config should be expected? Since I just listed all config that popped out in my mind.

5x knockback multiplier...?

Since if I use lower multiplier seem to have too short dash/no dash at all... I'm still unsure how it works...


I've linked the source code to my github and also updated library credits that I've used in this spell, but atm there is no code changes as recently I'm busy with my real life. Hopefully I could find some free time and update the spell soon.

 

[MyPad]

Nitpicks:

• The package used by the spell could be placed on the description for ease of access.
• The model could be realigned around the x and y-axes.

Notes:

• Groups are not being recycled properly. (Error message prompted)

Status:

Set to Awaiting Update (7-day period)​

 

[Cokemonkey11]

@DD_legionTN can you please send me a PM asking for code review and link to code on github, bitbucket, or gitlab?

You can include some high-level questions there but I will most likely only be able to answer 2-3.

 

[DD_legionTN]

Sorry for late the late response. Recently I'm busy with my real life and have not much time to apply the required changes for this resource. Hopefully I could have free time soon once I've finished my stuff.

@Cokemonkey11 The link to github is already in the description (I've updated it few months before)

 

[Cokemonkey11]

I am also very busy, so if you are interested in the review then please send a PM, otherwise we'll move on

 

[MyPad]

Any Wurst reviewer here? I would need some insights on this resource.

 

[Cokemonkey11]

I'm not sure if @DD_legionTN is asking for review of this one anymore?

 

[MyPad]

Ah, so that's the case? I see. I will leave this resource as is.