Thoughts on this simple code redeem system?

Help and Feedback Code Review
#1

  • What does the code do and what are you not satisfied with?
    This code listens for a RemoteFunction fired from the client when the TextBox has text entered inside. It checks if the entered text is a code, if the player has redeemed it already and handles accordingly. I am not exactly dissatisfied but instead wondering if there were was a better way to store which codes the player has redeemed already.

  • How (specifically) do you want to improve the code?
    Improve security of the system or save codes a better way (if there is one).

An explanation of the current system:

local redeemedCodes = {
    [Player object] = {
        "Codes"
    }
}

Server script:

local playersService = game:GetService("Players")
local dataStoreService = game:GetService("DataStoreService")
local codeData = dataStoreService:GetDataStore("CodeData")

local codeRemote = game:GetService("ReplicatedStorage").CodeRemote

local codes = {
	["SmallPoints"] = 50,
	["BigPoints"] = 100
}

local redeemedCodes = {}

codeRemote.OnServerInvoke = function(player, text)
	if codes[text] then
		if not redeemedCodes[player][text] then
			player.leaderstats.Points.Value += codes[text]
			
			redeemedCodes[player][text] = true
			
			return true
		else
			return false
		end
	else
		return false
	end
end

local function saveData(player)
	pcall(function()
		codeData:SetAsync(player.UserId, redeemedCodes[player])
	end)
end

playersService.PlayerAdded:Connect(function(player)
	local success, data = pcall(function()
		return codeData:GetAsync(player.UserId)
	end)

	if success and data then
		redeemedCodes[player] = data
	else
		redeemedCodes[player] = {}
	end
end)

playersService.PlayerRemoving:Connect(function(player)
	saveData(player)
end)

game:BindToClose(function()
	for _, player in ipairs(playersService:GetPlayers()) do
		saveData(player)
	end
end)

Local script:

local box = script.Parent

local codeRemote = game:GetService("ReplicatedStorage").CodeRemote

box.FocusLost:Connect(function(enterPressed)
	if enterPressed then
		local success = codeRemote:InvokeServer(box.Text) 
		
		if success then
			box.Text = "Code redeemed!"
			wait(1)
			box.Text = "Enter code"
		else
			box.Text = "Invalid code"
			wait(1)
			box.Text = "Enter code"
		end
	end
end)
2 Likes
#2

Seems fine, except for a few small nitpicks. For one, your Datastore calls aren’t actually doing anything with the pcall - so if datastoreservice errors, players may be able to redeem code multiple times. This isn’t mission critical in this specific case, but could be a big issue in other places.

Otherwise, there’s not much going on here, so not much do to wrong ¯\_(ツ)_/¯

#3

In your ‘saveData(player)’ function you call a pcall but do nothing with it.

#4

In Lua, calling pcall and discarding its return values is similar to a try statement with a trivial catch in many C-like programming languages; its only purpose is to suppress any runtime error.

Now, how @R_alatch handles this isn’t a good practice, as calls to interact with Roblox data stores that fail can at least be queued to be tried again at a later time, or some communication to the player that the save failed; however, what you are referring to itself is not indicative of a flaw in a vacuum.

#5

Looks ok but they are a few bad practices:

  1. In your .OnServerInvoke listener, you don’t need to return false. This is because whenever a function returns nothing, it automatically returns nil. To check if whatever you are returning is false you can use the ‘not’ operator which verifies the condition if it’s false or nil.

  2. Your saveData function seems to use an extraneous pcall which isn’t needed since you aren’t doing anything with the results of the pcall thread.

  3. Short circuit programming is encouraged! Even though it’s a micro - optimization it is faster than if statements. (Don’t overuse them if you think they might impact readability)

Use Case 1: In your PlayerAdded listener:

redeemedCodes[player] = success and data or {};

Use Case 2: In your local script in your FocusLost listener:

box.Text = success and 'Code redeemed' or 'Invalid code'
  1. Continuing on in your server script, in your PlayerRemoving function, you have extraneous function calling, since you are running a function to run another function. Instead you can do:
playersService.PlayerRemoving:Connect(saveData) -- Automatically passes the player argument to your function
  1. Subjective tip: PascalCase makes your code stand out more :))
#6

Thanks for the tips. Can you explain why you do = success and data? Why not just = data?

#7

We can break down this statements into chunks:

redeemedCodes[player] = success and data or {};

If success is not false or nil, then redeemedCodes[player] is set to data. If that’s not the case, the key is set to an empty table. Also, short circuit programming is faster than if statements as I mentioned in my previous reply. It prevents having multiple else statements and being repetitive as well.

1 Like