Increased possible amount of apartment id's #133
Conversation
|
Why not making it a config? |
Theres no need to make a config option for generation of apartment ids, they are used only in the backend, and people will just mess it up. Why do you think its a good idea to make it a config? |
|
|
||
| while not UniqueFound do | ||
| AparmentId = tostring(math.random(1, 9999)) | ||
| AparmentId = tostring(math.random(1, 1000000)) |
There was a problem hiding this comment.
Why use a magic number here? Wouldn't it be better to have this as apartmentIdLimit
There was a problem hiding this comment.
because people dont know what it does, would try changing the values and would probably mess it up, the number is high enough so the limit is never reached
There was a problem hiding this comment.
You talk about "magic numbers", but in the previus code you already use it.
I'm just following your GREAT coding practicies
There was a problem hiding this comment.
It's already a magic number so why create a var just for this instance when the entire resource has 20+ magic numbers inside of it.
The better option if your looking for uniqueIDs would be to instead loop over trying to create a uniqueID with a limit of 9999 entries would be to simply query from the database the number of entries and use that value + 1 to generate it. And now you don't have to worry about having a infinite loop serverside.
There was a problem hiding this comment.
Just want to say I have not wrote a single line of code to contribute to QBCore these are not my coding practises. I don't have the authority to merge PRs. I just saw that this could be changed to a variable instead of a magic number.
My review is not blocking this work at all just a simple suggestion.
There was a problem hiding this comment.
Understandable, what I meant by "your" coding practices is the QBCore frameworks, worded incorrectly. I didn't want to put it into the config as a variable since people might mess it up. I'm not looking forward to spending more than 10 minutes of my time coding a better system like maximus suggested for QBCore.
So thats why I use a "magic number", it has no meaning to change it as it doesn't affect the gameplay, and it can only make the server un-playable if you set it low, as some people who don't read might
There was a problem hiding this comment.
Eh these coding practices are just outdated and probably wouldn't be that way if they were written in this day. Thank you for the PR otherwise.
Describe Pull request
Increase apartmentId generation from 1-9999 to 1-1000000 (1mil)
If your PR is to fix an issue mention that issue here
If over 10000 unique characters are created players are stuck in a loading screen since there are only 10 000 possible apartment ids therefore no new apartments can be made after that.
Questions (please complete the following information):