Is this a good method of limiting datastore usage?

Help and Feedback Code Review
#1

I am currently in the middle of writing a basic admin commands script as a mini-project to help expand my knowledge of the Lua programming language and the Roblox scripting environment.

I would appreciate some feedback on the following code. My aim here was to limit the number of times the datastore is accessed directly, so once a users admin status (true or false) is confirmed, they are added to a dictionary which is checked before the datastore. The following code exists within a module script.

-- IsPlayerAdmin Supporting Structures
-- Table is exposed to allow adding/removing admins via commands
-- TODO: Encapsulate table to a modification function
module.AdminConfirmation = {}

-- IsPlayerAdmin Function
function module:IsPlayerAdmin(player)
	
	-- Ensure the given parameters are of the correct type
	assert(typeof(player) == "Instance", "[ASSERTION FAILED] PlayerEvaluation->IsPlayerAdmin [1]")
	
	-- If the player is in the table, return the associated boolean value
	if self.AdminConfirmation[player.Name] ~= nil then return self.AdminConfirmation[player.Name] end
	
	-- The player has not been checked yet, so check the datastore
	local success, result = pcall(function()
		return AdminStore:GetAsync(player.UserId)
	end)
	
	-- If the datastore was accessed successfully
	if success then
		-- If the player is an administrator
		if result then
			-- Add to table so the datastore does not need to be checked again
			self.AdminConfirmation[player.Name] = true
			-- Return true (they are an admin)
			return true
		-- If the player is not an administrator
		else
			-- Add to table so the datastore does not need to be checked again
			self.AdminConfirmation[player.Name] = false
			-- Return false (they are not an admin)
			return false
		end
	-- The datastore was not accessed successfully
	else
		-- Output a warning to the console
		warn("pcall was not successful at PlayerEvaluatiton->IsPlayerAdmin, assumed false! Error: " .. result)
		-- Assume false due to error
		return false
	end
	
end
1 Like
#2

Issue with current method

Looking at this I can see your aim but you will soon get into problems with memory as you have no cleanup operations (as far as the code you have posted).

The second issue and this is a genral point (one that I see a lot in admin scripts) is that you are using the players name. Stick to using the players id as this will never changes in any circumstance.

Feedback on general process

My main feedback is that what you are doing is including a cache between the data store and your module. This can be seen as a separate process and in most system is its own entity that other functionaity builds from. I would recommend building a new module that manages this database caching.

It would do the same task as the code above but with the addition of including a process for removing cached data that is stale.

This is often done in two ways.

  • A background thread that runs on a timer to perform a cache cleanup.
  • On accessing data there is a chance of running a cache cleanup ie math.random(0, 100) == 2 run cleanup

Other

GetAsync should only allow a string but often I see people use ints for keys which I assume would then be converted to a string. Either way I would use
AdminStore:GetAsync(tostring(player.UserId))

When using pcall you can pass the arguments for the function so you can change the code to
local success, result = pcall(AdminStore.GetAsync, AdminStore, tostring(player.UserId)

I hope this helps.

#3

Thank you so much for such a detailed reply, I’ll be sure to use your advice!

I was using the player’s name in the table and the ID in the datastore, but I will exclusively use the ID to make things less complicated.

Do you think hooking the player disconnected event and manually clearing that specific player’s data on disconnect would be a good idea to maintain the size of the table, or would I be better off fully clearing it as you suggested?

I assume it gets implicitly converted from an integer to a string, but I will manually convert it to a string because this is probably good practice.

Interesting, thank you! I had seen pcall being utilised that way before but was unsure of what it meant.

Overall, thank you so much!

1 Like
#4

This is more to do with some admin scripts may alter the player name.

I think that is a very good idea to use the PlayerRemoving event. Use events where possible in code at all times.

To explain this in a bit more detail. When using : you are passing the table which the function is part of implicitly.

local tbl1 = {Name = 'tbl1'}
function tbl1:foo() print(self.Name .. ' foo') end
function tbl1.bar(self) print(self.Name .. ' bar') end

local tbl2 = {Name = 'tbl2'}

tbl1:foo() -- implicitly defined self table1 foo
tbl1.bar(tbl1) -- explicitly defined self tbl1 bar

-- using self can be unsafe
tbl1.foo(tbl2) -- prints tbl2 foo
tbl1.bar(tbl2) -- prints tbl2 bar

-- some fun stuff :)
local part = Instance.new('Part')
part.Parent = workspace
wait(5)

script.Destroy(part) -- Destroy the part using script
1 Like