Skip to content

Increased possible amount of apartment id's #133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 27, 2025
Merged

Increased possible amount of apartment id's #133

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion server/main.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ local function CreateApartmentId(type)
local AparmentId = nil

while not UniqueFound do
AparmentId = tostring(math.random(1, 9999))
AparmentId = tostring(math.random(1, 1000000))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use a magic number here? Wouldn't it be better to have this as apartmentIdLimit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You talk about "magic numbers", but in the previus code you already use it.
I'm just following your GREAT coding practicies

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah

local result = MySQL.query.await('SELECT COUNT(*) as count FROM apartments WHERE name = ?', { tostring(type .. AparmentId) })
if result[1].count == 0 then
UniqueFound = true
Expand Down
Loading