Is this an efficient/good way to pick a random item with rarities?

Help and Feedback Code Review
, ,
#1

Hey! I saw many people overcomplicating this, so I tried to make my own system for this.
Now the question is, is this efficient/good or not? It should be pretty easy to understand.

  • Efficient, it’s pretty good!
  • Not efficient, will tell you why in comments.

0 voters

Code:

local Items = { --ItemsTable, lower = rarer
	{"FirstItem", 1};
	{"SecondItem", 2};
	{"ThirdItem", 3};
	{"FourthItem", 4};
	{"FifthItem", 5};
	{"SixthItem", 6};
	{"SeventhItem", 7};
	{"EigthItem", 8};
	{"FifthiestItem", 50};
}

local function getNums() --Gets the lowest and highest numbers of the table[2]
	local highest = 0
	local lowest = math.huge
	for i,v in next, Items do
		if v[2] > highest then
			highest = v[2]
		end
		if v[2] < lowest then
			lowest = v[2]
		end 
	end
	return {highest, lowest}
end

local function getRandomItem()
	local lowest = getNums()[2] --Get the lowest number for math.random
	local highest = getNums()[1] --Get the highest number for math.random	
	local randomNumber = math.random(lowest, highest) --Set the randomNumber to math.random the lowest and highest number
	for i, item in next, Items do --Loop through the Items table
		if randomNumber <= item[2] then --If the math.random number is smaller or equal to the value, then return that item
			return item[1] --Return that item
		end
	end
end

for i = 1, 10000 do --Loops 10000 and executes that function. Prints the item for testing
	print(getRandomItem())
end

Thanks!
Edit: Also, if you want to know, it does work fine when testing it.

2 Likes
#2

I’m not well versed with weighted random pickers but I notice there are two issues that should be addressed. First is to stop using “in next” and instead use “ipairs” because you’re working with a contiguous array, not a dictionary, and the former is weird and bad for readability. Second is to not call getNums twice in getRandomItem. Pick one of these two options:

local lowest, highest = table.unpack(getNums())

local nums = getNums()
local highest = nums[1]
local lowest = nums[2]

It’d probably be more readable as well if the lower number was returned first instead of the second. On that note: you know you can return more than one thing right? You could then use the first option I supplied above for alternatives but without the table unpacking and without wrapping your returns in a table.

local function foobar()
    return 1, 2
end

local first, second = foobar()
print(first, second)
2 Likes