[vJASS] [System] StackItems

[SanKakU]

no text.

 

[edo494]

Also AddItemCharges is very very misleading name, because you actually only add one charge, yet the function seems like it proposes option to add N charges

Edit: That wasnt very nice of me was it. For those curious, check below the quote

 

[SanKakU]

no text.

 

[looking_for_help]

So you refused to read it because there are no tabs

I would also refuse to read it without tabs.

Also we have Coding Conventions that resources have to follow, you might want to read that.

even though I went through the trouble of adding comments

It shouldn't be trouble for you to add comments to your own submission. Especially since I can only count 6 comment lines which is really few. The Jass section is for high quality resources, so you might be willing to spend some time on discussion and feedback.

About the resource itself:

• The meachnism is very unefficient, you repeat a lot of actions in your two helper functions.
• Why does AddItemCharges always return false? That makes the return value quite useless.
• The library lacks a lot functionality - what if I want to remove charges, add more than one charge, get the current number of charges etc.
• What if a unit carries multiple equal items (max charges of first item exceeded)? How can I adjust the corresponding charges individually?
• Return values of natives should be stored in local variables when called multiple times.
• Why you use ConvertItemType when there is already a constant for that?
• Same with bj_MAX_INVENTORY vs magic number 6
• Function names like GetInventoryOnce/GetInventoryAgain are not descriptive and should be actually private.
• The function name AddItemCharges is also very misleading. Actually you merge two items which is very different.
• The resource lacks a proper documentation

 

[SanKakU]

no text.

 

[edo494]

You cannot choose how many charges are added. Like I told the other guy, it's not one charge being added, it's however many there are of the item you pick up. Removing charges makes no sense for a stacking system. Why can't the user add their own get number of charges trigger? I'm sorely confused why you are asking about this. Maybe it's related a wrong assumption on your part?

This is very very limiting tho

Hmm, now I'm starting to think you really hate natives. Why?

The thing is, magical constants are harder than named values when it comes to understanding the code and how it works.

Everything that is not part of API should be private, its part of the inner workings of the system, not something that should necessarily be visible/modifiable/callable, so it should be private. The name is a whole different story

 

[SanKakU]

no text.

 

[edo494]

adding N stacks to itemtype inside unit, removing N stacks of itemtype inside unit.

This is actually very very confusing, I say "Add charges", so I would expect it to require unit, item type and how many charges, not unit and item to such the charges from. This still boils down to the function name tho, and is very limiting, because even if you change the function name, it doesnt change the fact that you only allow one way of adding charges to unit, by sucking other item's charges

 

[SanKakU]

no text.

 

[edo494]

well, I dont really think you were or are aggressive. Just regular debate

 

[SanKakU]

no text.

 

[edo494]

I dont know anything about nonpowerups leaking, but powerups are the one that leak when not destroyed, despite being consumed by unit

 

[looking_for_help]

Hmm, yes, maybe I am being too redundant here. I will give this some thought, and in the meantime, if someone else would like to share their opinion on this, that would be cool.

I don't think there is much to think about here, because if a function always returns false, the return value has no information and can be omitted. But you changed that already so, thats ok.

This system ignores max stock object data as that is a shop thing, it has nothing to do with picking up items and stacking them, and there is no such thing in there as max charges so I don't really know what you're going on about. There is a number of charges data entry, but that's irrelevant. You do not require additional item slots for the same charged item on the same unit. That's the whole point of an item stacking system, or at least that should be what it's for.

Thats your opinion how stacks should work, but its not like that even in ladder Wc3. Orc healing salve items don't stack up infinitly for example, and so might many map makers also don't want their items to be infinitly stackable, e.g. for balancing reasons. A stack system should be able to handle such basic requirements.

You might choose a more object oriented approach to tackle there kind of problems.

I have observed this practice in nearly the entire thing. Guess I got more work to do? Where did you get that quote, I don't see it on the link you provided?

Thats common sense. Its usually more efficient, but if you can provide a reason why this doesn't apply for your specific case here, then ok.

Hmm, now I'm starting to think you really hate natives. Why? Not to tell that you're wrong, but if you're right, could you tell me how you know you are?

Its not about hating natives. Its about code readability and maintainability.

If you write ConvertItemType(1), its not clear what you mean with that. Of course you can explain it in a seperate comment (which was your argument), but why distract a user in the first place? A user might read the code and ask himself "why did he use ConvertItemType, and not the constant"?

Then he reads your wall of text of a comment and will still not 100% be sure why exactly you used it there. Is it because of some strange Wc3 bug that makes it neccessary here (wouldn't be the first time)? There have been encountered bugs with convert natives which can be abused for additional functionality. I see no advantage in not using the constant but only disadvantages. It violates the principle of least astonishment and therefore the constant should be used.

Same applies by the way to the magic numbers in your code like 6 for inventory size. You know, there is the native UnitInventorySize, because units can have inventory sizes different than 6.

Also you might consider (nested) spellbooks as well in your library.

I'm having a hard time understanding your reasoning here. I want other users to feel like they can just make their own system and have it interact directly with this one if they so choose. If I made it public I could allow them to do that, but that wasn't your suggestion. And I dislike the public function idea, but nothing is final and it's definitely a good option. Certainly much more sensible than making them private. I honestly don't see what's the point of making them public since if a person wanted to rename the functions, nothing is stopping that person and that's literally all making it public does.

I didn't say public, I said private. Its all about encapsulation and hiding implementation details to keep the interface clean.

 

[SanKakU]

no text.

 

[looking_for_help]

All making it private does is give the name a random suffix in compilation so that any code cannot access the name. As long as users don't access the name of the function accidentally then the same effect is applied as if it were private. That's why I made sure to make the names for the functions rather long and they seem unique enough to me. If anything, the library name itself is a bit lacking in uniqueness, but it's a simple matter to rename it.

There is no point in choosing "long" and potentially "unique" names for functions that don't belong to the interface of the system. Just make them private because thats exactly what the private modifier is made for.

Isn't this more of the 'do spell book items systems!' theme?

I'm starting to think I should rename the library to something else so that you'll stop bugging me about adding support for custom-UI inventories.

This has nothing to do with custom UI systems. A units inventory size is defined by the inventory ability, which can be modified in the object editor such that the unit has a different inventory than 6. And to get that size is exactly the purpose of the native I posted. You should do some research on how those things work.

Actually, the more constants you use, the more slowly your code compiles.

What?

Of course not, where did you get that from? If you make such statements provide a prove. Using the constant will compile even faster because a constant is just one identifier node in the Jass AST while a function call with one argument will potentially evaluate to (at least) two AST nodes and aditional argument type checking.

Also, even if that would be true, the speed difference would be 100% insignificant.

 

[SanKakU]

no text.

 

[looking_for_help]

You can make the inventory be less than six, but you stated more. You've been posting at least a few things in this thread that are the opposite of true.

No, I said different than 6:

Same applies by the way to the magic numbers in your code like 6 for inventory size. You know, there is the native UnitInventorySize , because units can have inventory sizes different than 6.

So stop twisting my words.

I don't have to provide proof, it's already been documented, you just have to know where to look. I do not think I am using very many BJs in my map but I have a few, now when I comment out include "cj_antibj_base.j" I get a compile code duration of 57 seconds. When I put the line back in, the duration is 53 seconds.

You posted this as a vJass library, you forgot? There is no include in vJass and if you just use ITEM_TYPE_CHARGED instead the convert native compilation time isn't affected at all.

If you think that's insignificant, then I'm done wasting my time on this resource for your pleasure.

You don't seem to open for constructive critique at all. Why should the typical vJass user care about an include that isn't related at all to this library? That just doesn't make sense.

 

[BPower]

Some basics thoughts about your code:

• What happens when I use GrantChargesToUnit and that unit has a full inventory. Is the result acceptable?
• For better user safety the exitwhen condition of above mentioned function should be moved two lines up and be a >= comparison.
• You could initialize local integer index directly to 0.
• Why is there a local item handle itemIndex?
• The function adds items to units, but the word item is not included. What about the name AddItemCharges, eventually AddUnitItemCharges?
• In my opinion function MergeTwoItems shouldn't take a boolean as arguments, which only purpose is to stop the whole operation if false.
  Side note: b != false can be optimized to not b
• The function name is a bit irritating as it takes only 1 item handle as argument. ( my opinion )
• CheckA, CheckB and MergeTwoItems can be overall shortened and optimized.
• You should include API and short description into your library ( outcommented ). Prefered at the top.
• StackChargesLib is far from beeing the best name I could think of. Again try
  to include the word item, as the whole system is just about items.

 

[SanKakU]

no text.

 

[looking_for_help]

Your own words are twisted, I had nothing to do with that. Not reading anymore of your posts.

You claimed I talked about inventory sizes bigger than 6 which is just not the case.

Anyway, this discussion is getting ridiculous. Just three things:

• Read the Code Convention rules. You have to indent your code properly and you have to provide a proper documentation.
• Read the Jass Submission rules. Argumenting with cJass features (include) like you did is not valid because you are not supposed to submit cJass resources in the first place. Especially if its a purely vJass resource.
• Read the Site rules. You may a) keep a civil tone and b) not invent things about other users.

Finally, you should not submit resources if you can't deal with constructive critique. Comments like "This is done against my better judgement" or about code indentation are unnecessary. If you disagree with feedback, enter the discussion and provide well-founded arguments why you disagree.

At this point, I think finishing up this resource is going to be a waste of time unless you want to petition to change the policy about the indenting being requred for resources. everytime i get around to discussing this with someone like you, I want to show you my updated code, but I don't want to go through the hassle of being harrassed or butchering my own code before posting it.

Since you seem to refuse to follow at least basic rules like proper code indentation I will graveyard this in 7 days if nothing significantly changes here ...

 

[Bribe]

Sankaku, I used to think you were trolling with the rejection of indentation, but then I see this:

"I indented the code because apparently that's still a thing after all these years."

I recall when you first published this resource, you basically inlined a bunch of BJ functions. Please bro, your troll level is over 9000.

 

[SanKakU]

Argumenting with cJass features (include) like you did is not valid because you are not supposed to submit cJass resources in the first place. Especially if its a purely vJass resource.

I didn't do that? I put in that comment about the possibility of cJass being in the code because of missing call and set words.

Yeah, since you didn't close the thread yet, I'm just going to delete the content of the posts...I will not take kindly to lies and bullying.

 

[Nestharus]

SanKakU, I would like to personally thank you for submitting this resource. It saddens me that you removed it.

I understand that you did things in the resource with your best judgement. You worked on the resource that you liked and did things with it that you also liked. This is perfectly ok. However, when you do submit it as a public resource, you do have to adhere to community standards, even if you disagree with them : (. Once you try to make a resource public, it's no longer about what you personally like, but rather about making your resource compatible with the needs of others : ).

looking_for_help was trying to relay this, but he might have been a bit heavy handed, giving the impression of bullying. If you would like to share your resource with the community, please consider what that community expects ^_-. If you'd prefer to show your resource as it is in your own vision, possibly sharing any discoveries you might have encountered, feel free to make use of The Lab =). The Lab is a much more informal place.


I hope that this experience doesn't deter you from making future contributions to THW.

 

[Ralle]

SanKakU,

looking_for_help posted no twisted words. He is very precise in his phrasing. I see nothing bad here on his part.

I do however see you accepting a lot of critique and changing your resource for the better and suddenly you get angry and remove it all. This is not very constructive. I know it can feel like a personal attack when people criticize your creation. Imagine how much critique I get about this site. I will not remove the site because I get angry, at least I haven't yet .
It sounded like your resource did not need much more work to be ready for approval, a pity you gave up, you were so close to the finishing line.

 

[looking_for_help]

I didn't do that? I put in that comment about the possibility of cJass being in the code because of missing call and set words.

Actually, the more constants you use, the more slowly your code compiles.

...

I don't have to provide proof, it's already been documented, you just have to know where to look. I do not think I am using very many BJs in my map but I have a few, now when I comment out include "cj_antibj_base.j" I get a compile code duration of 57 seconds. When I put the line back in, the duration is 53 seconds.

Nothing more to say.

Yeah, since you didn't close the thread yet, I'm just going to delete the content of the posts...I will not take kindly to lies and bullying.

I didn't close the thread because I hoped you will continue working on the resource.

If you consider code indentation bullying, this is the wrong place to complain about. I mean, of course such things are always somehow subjective. Same with naming conventions. But thats why they are called conventions, because not necessarily everybody agrees with them, but its better than having different styles in every new resource. And tesh already indents your code automatically when hitting enter, so I don't really get the problem with this.

I would like to encourage you to keep working on this resource, it has potential. You shouldn't take the critique personal, its just that there are some basic rules (like proper code indentation) resources have to follow.

 

[BPower]

I'm a bit disappointed of your reaction and the deletion of the thread content for users.
Along with the other moderators and users, I invested time and checked your resource and gave tips for further development.
Now my invested time on this thread seems wasted to me.

Don't get so angry Ralle

Good comment Nestharus.

 

[BPower]

You have one week to change your mind.

Scheduling this to graveyard in 7 days. We can still un-graveyard it, once you wish to work on
your resource again.