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.