CloudSixteen / Clockwork

A roleplaying framework developed by Cloud Sixteen for the people.
http://CloudSixteen.com
MIT License
44 stars 44 forks source link

[CODE SMELL] For the love of god, don't search stuff by string #472

Closed ExtReMLapin closed 5 years ago

ExtReMLapin commented 5 years ago

There is this horrrrible code smell present everywhere in the code.

Stop searching stuff by string, i'm talking about all those functions like FindByID(string)

It's a terrible thing to do for the following reasons :

murray-elijah commented 5 years ago

As far as I'm aware, FindByID(string) is used solely to find items or entities by their string IDs, which have no real reason to be translated. I could be wrong with this, but if not then using FindByID() is perfectly valid. Regardless, I'm not the one to talk to about it. @VortixDev would be your best bet.

Aspect12 commented 5 years ago

I think FindByID is used for Factions, Classes, Commands, Libraries and a bunch of other stuff as well other than Items.

ExtReMLapin commented 5 years ago

which have no real reason to be translated.

Per my last mail message, people are retarded and don't understand shit, they see a string, they try to translate it.

Strings are for humans, integers for computers, that's the whole point of using enums (here globals ints since Lua doesn't support it )

Maybe i should not have put the "people are stupid, they gonna try to translate it" as the first reason, right. But the second reason is a 100% valid one, it's more than a "code smell", it's a design issue that can hit the performances.

murray-elijah commented 5 years ago

@Aspect12 My mistake.

@ExtReMLapin FindByID has been used in CW for as long as I can remember. While it's not the most efficient way of comparing, it's still the CW standard. From a realistic point of view, it's a lot of work for what's probably only a minor performance improvement, plus most people would likely stick to old versions from the C16 store regardless.

If you want to implement it, I guess fork the repo and do it, but a big change like that to the framework when the schemas are closed source won't see a lot of support and will probably break a good few things.

VortixDev commented 5 years ago

Hello, thanks for your report. Hopefully, I can address your concerns.

Translating strings ID strings should, to my knowledge, never need to be translated. From what I see, your main concern is that people mistakenly do this without knowing not to. I think the same argument could apply to other data types as well: there are plenty of operations that apply to numbers that don't apply to strings, and many of those operations would not need to be used if a number was used as the ID, such as multiplication. I cannot control what people do with their code, but it is important to note that Clockwork is a framework, and as such is designed for developers rather than end users (it is the schemas that are designed for end users). I appreciate that Clockwork documentation is poor, so developers who are struggling to understand aspects of Clockwork development are free to ask for help in our forums or our Discord (whose link can be found on the C16 forums).

Performance On the topic of performance, the use of enums would require the exact ID to be known in order to obtain a value. For example, you would need to pass an ITEM_WATER value into the function to obtain that item table. In the current system, if you know the exact ID, the lookup will be equivalent to a table lookup. That is, if the passed identifier is used as an index in the items table, it will just return the associated value - no string comparison necessary. The string comparison operations you're referring to are only necessary if the string ID passed into the function is not used as an index in the stored items table, meaning that a partial string was passed in (e.g. breen_s instead of breen_s_water). This is an added feature compared to an alike enum-based function, as you wouldn't be able to pass in a "partial enum", whereas you can pass in some subset of the string ID in the current implementation. So, any loss in performance would only appear if you compared different use cases of the function, rather than a like-for-like comparison.

Saving data Enums can be troublesome when saving data: if you use an enum for identification you need to ensure that the exact same enum is generated for the same item when the server re-launches. This is easy when you can ensure that the state of the system is the exact same as it was in previous launches, but if someone was to create a new item, it would throw off the number assignment mechanism. Suddenly, peoples' inventory contents would change from launch-to-launch. The problem could even present itself on a single launch instance, taking auto-refresh into account.

User usage The strings used for identification aren't only used internally by the framework: they are also used by users, through commands (e.g. chargiveitem, ordershipment).

The fittingness of an enum type Enums, from my experience, are better used in cases of constant content, and non-persistence. For example, I can't speak for other langauges, but in the cases of Python and Java, I don't believe enums can be dynamically extended. This means that the types, as they are implemented, would not work for storing IDs. As you say, one could have a numerically indexed table and have a variable associated with each number, but that leads onto the next point.

Principles of design By linking a readable ID to the data (having a string index to the stored data table) you allow a user to take a piece of data, and identify the item it's associated with. If you use a number, you lose the ability to do this, unless you have knowledge of exactly how the numerical index generator assigned IDs in the given launch instance. Sure, you may be able to decipher what the data is from the data itself, but there's no natural assurance of that: the variable names used for identification can only be used for development, and so you lose ease of debugging by dropping human readable identification.

Additionally, using global variables pollutes the global namespace with data that is not needed globally. One should, in this case, define the numbers in a table within an appropriate table. The table assigned by Clockwork for such things is Clockwork.item, so you could set up a Clockwork.item.itemEnums table. Personally, though, I would much prefer typing "breen_s_water" to Clockwork.item.itemEnums.BREEN_S_WATER. I do note that GMod does itself stores many, many enums as globals, but I am not a fan of this.

Conclusion On face value, the string IDs may appear pointless, but they are incredibly useful tools for data persistence, debugging, ease of programming, and user usage.

Hopefully, that clarifies things: I will leave this open in case you wish to respond, but feel free to close it if you feel satisfied by my explanation.