QuestUtils v1.4

[Moderator]

23:57, 29th Sep 2012
Magtheridon96: Approved. This is a good wrapper.

Tips:
- You never really need to null anything if you're going to overwrite it directly afterwards. (.q and .qi)
-

local thistype this
set instance = instance + 1
set this = instance

->

local thistype this = instance + 1
set instance = this

By the way, the only reason I stressed on convention is because this is solely a wrapper, and API is pretty much the only thing that can be assessed. Convention is always important especially in APIs, not because I want it to follow convention, but because everyone does. If everything follows different conventions, it's going to be really confusing for people to manage their maps and their code is going to look really strange. If everything is uniform, it's far easier.

I'm sorry I did not make all these conventions explicit, but we often take them for granted :/

 

[Nestharus]

QuestUtils is a very very bad name for that struct. Name it Quest instead... :\


With QuestUtils, that tells me that you are creating instances of Quest Utilities, which doesn't make a whole lot of sense. It appears that you are actually creating Quests, which is why the struct should be named Quest.

edit

//! runtextmacro Sound("Completed","Completed")
//! runtextmacro Sound("Discovered","Discovered")
//! runtextmacro Sound("Failed","Failed")
//! runtextmacro Sound("Hint","Hint")
//! runtextmacro Sound("ItemAcquired","ItemAcquired")
//! runtextmacro Sound("Secret","Secret")
//! runtextmacro Sound("Updated","Updated")
//! runtextmacro Sound("Warning","Warning")

Those should probably be in a separate resource/snippet. The user may want to customize their own sounds, so you should not couple sounds with the quest objects. The quest objects should be very basic (what wc3 does) but in an OO and simpler to use format (as you have).

edit
other mehods that don't belong in this resource

    static method hint takes string hintText, boolean forPlayer returns nothing
        if forPlayer then
            call DisplayTimedTextToPlayer(GetLocalPlayer(),0,0,30,hintText)
        else
            call BJDebugMsg(hintText)
        endif
        call SoundHint()
    endmethod
    
    static method warning takes string warningText, boolean forPlayer returns nothing
        if forPlayer then
            call DisplayTimedTextToPlayer(GetLocalPlayer(),0,0,30,warningText)
        else
            call BJDebugMsg(warningText)
        endif
        call SoundWarning()    
    endmethod

edit
It doesn't appear that you wrote struct instancing correctly :\. Right now, you can't destroy quests or quest items correctly, so you should probably fix your allocation/deallocation.

edit
This shouldn't be changing the text

    method completed takes QuestUtils index, boolean changeText, string text returns nothing
        if changeText then
            call .setText(index, text)
        endif
        call QuestItemSetCompleted(.qi, true)
        call FlashQuestDialogButton()
        call SoundCompleted()
    endmethod

Also, move all of these methods to method operators. Instead of setText etc, do text=. The completed method should be changed to complete. Example -> call quest.complete(questItem). You should be using quest items rather than arbitrary quest indexes.

So API needs to be changed, allocation/deallocation needs to be fixed, and a few things need to be removed : ).

 

[mckill2009]

Ima heed most of your suggestions, except changing some API names...I see what I can do with methods changing to operators coz the difference I can see in this is without '()' when it returns a boolean like 'isCompleted'...thanks anyway...

 

[Magtheridon96]

But if you're using a method operator for one and a method for another, it's not intuitive at all D:

edit
Also,

local thistype this  
set instance = instance + 1
set this = instance

->

local thistype this = instance + 1
set instance = this

 

[mckill2009]

But if you're using a method operator for one and a method for another, it's not intuitive at all D:

Fine, Ima change it next...thanks...

about the EDIT, I purposely did that coz Ima put Quest Limit...

this doesnt even needed 'set .qi = null'

 

[Magtheridon96]

But if you don't null the variable, the handle will not be recycled until the variable is overwritten, so in a map with 100 quests initially, and then 2-5 quests at a time during the rest of the game, you would have about 95-98 handle leaks :<

Yes, this is an extreme case, but "set .qi = null" shouldn't hurt :<

 

[mckill2009]

Magtheridon96: It's a standard for corresponding method operators to match, so:
completed & completed=
failed & failed=

isCompleted and the rest should be changed to match their corresponding method operators to follow this unwritten, yet abided-by convention.

here we go again, what's wrong with you dude?, there's nothing wrong there!!!, you're writing BS again!, I'm not a robot to follow your s*** naming games!

 

[Nestharus]

method operators on the user end represent variables. If they have different names, then it's like you are make a variable a different named depending on what you are doing. It's extremely confusing in the API to do that. If you have 2 method operators that both operate on a variable (setter/getter), they must both be the same name. This is standard practice in c++, java, c#, etc. I don't know of a programming language where this isn't standard practice.

magtheridon96 is correct in demanding that your method operators have the same name when they operate on the same variable. By not doing that, you simply confuse the user as now the variable names are changing depending on what you are doing.

Let's say you have a variable named completed. However, when you read it, suddenly you have to use isComplete. So 1 variable with 2 names depending on what you are doing. No language does this with its regular variables, and you should not do this with method operators (variables with custom defined behavior).

vJASS itself also follows this standard. You have [] and []=.

If you don't follow this standard, you are not following vJASS convention.

If you refuse to do the above, the resource can simply be rejected for not following basic vJASS conventions inherent in the vJASS language.

 

[mckill2009]

This is WC3 not c++ nor any major programming languages and standards for naming to follow those programs is NOT mentioned in the spell rules...
http://www.hiveworkshop.com/forums/faq/spellrules/#faq_spelllang

Perhaps it's time to change the rules before someone do a nasty rejection...

EDIT:
Also why he didnt mention it on the first place?, this is a short code, and he should see it...if I update this one, what's next?...remove the limit coz nobody will create more than 100 quest in a game?, that's BS...

 

[Nestharus]

Well, the general conventions have been enforced even though they weren't in the rules, but they should be in the rules ;\.

for example, constants in all caps, functions as HelloWorld, methods as helloWorld, structs as HelloWorld, etc. All of this has been enforced, although those conventions aren't in the rules. GUI naming conventions have also been enforced.

 

[mckill2009]

Put a link where it has been enforeced as a rule...it's not even in the 'Resource Submission Rules & Guidelines' General rules...

EDIT:
Besides a moderator can be wrong...although it doesnt hurt to change it but making suggestions one by one after the spell is updated is just like jerking around which is very very very bad for customers...

EDIT2:
Constants are NOT in caps for some of my systems, and many of those should be in constant but are not, but still they have been approved, why? coz they are not in the rules...

He may have the full decision to reject the spell or not, but I have the right to protest coz 'it's not in the rules'...

 

[Nestharus]

It can be added to the rules first thing tomorrow, lol.

 

[mckill2009]

so proves my point, until then, this doesnt need to be changed unless I did something againts the spell rules, coz what I see here is nothing wrong with his suggestion, but so does my code...

 

[Nestharus]

You might as well do it now though since the rules are going to be changed before this gets approved >.<.

There's no point to this fight :\.

 

[mckill2009]

oh yes Im doin it now and I dont call this a fight coz Im just reasoning out my point of view ...

 

[Magtheridon96]

Following convention is a good thing because your scripts would be clear, uniform and easy to read/use.

The spell rules are going to be updated very soon, as they're outdated. REALLY out-dated. They've been the same since 2009 I believe.

 

[mckill2009]

ever wonder why it doesnt change?, coz a moderator may have full power to implement it all, if he doesnt like the 'name',
he will quote the rules that you must change to this name which is very bad...coz sometimes people want this name
but is forced to change it coz of some selfish acts...

 

[Nestharus]

Names can be debated by examining examples from JASS and vJASS. Whichever name is closer to the standards set forth by those 2 conventions is the name that wins.

 

[mckill2009]

edit
Awaiting suggested changes to follow convention.

what's next about this?...anyway I've already reported this and state my thoughts...for now Ima change it, but what's next?...

EDIT:
your last suggestion Ima follow in the next updates, sorry for my temper coz Im just quoting something that isnt in the rules but still you implemented it...

 

[ExecutorWar]

Wow this is very useful i can use this on my project.

 

[Nestharus]

QuestUtils is still a terrible name for the struct.. lol

Example of my point
QuestUtils questUtils = QuestUtils.create(...)

so we're creating quest utilities now? Do those create quests or what? Or do we pass quests into them?

The struct name is extraordinarily confusing.

If it was all purely static, the struct name would be ok, but you're doing instancing there, so you're really creating quests, not quest utilities'.

edit
this isn't in the rules anywhere, but it's a followed principal among all experienced OO programmers in every language. Never create classes that have plural names, like class Lists, or class Utilities. You can do class List and class Utility, but not plural. The name should also reflect the class. QuestUtilities is both plural and it doesn't reflect the class, meaning that it breaks both of these principles : p.

Now, there are some special cases of this, like when the class specifically contains a set of item a's, then you can actually give it a plural name, that is As or something. Generally, this is done with the actual variables though, not the class itself.