Any tips I can use to improve this piece of code?

Hello Developers, I’m an intermediate dev who recently just created a script for the purpose of teleporting players to another game as well as bringing some data in them for a dungeon-based game.

This snippet of code works as intended, but I’m curious to see what type of feedback I get on this code.
I’m mostly interested to see if I’ve created any flaws or blunders while creating this.

Apologies in advance for the amount of commenting, this is not code I created for someone else in order to help them with their game. So I left a lot of comments just in case my code ever needs to be changed in the future.

I’m looking for all types of input regardless of how small it is, I believe all input matters no matter what.


------------
------------------
---------------------
--this will NOT WORK IN STUDIO 
--Made by RoGale
--Deleting my credit notes makes you a scripting pirate(thief)
---------------------
------------------
------------

-- You can adjust these values as needed
------------------------------------------

local SetAsynctMaxAttempts = 5
local SetAsyncRetryDelay = .5
local SetAsyncRetryBackOff = 1--used to help lower stress on async by making it so it will wait 1 additional second every time it fails

local QueueTime = 2 * 60 --the amount of time the data queue stays open before closing in minutes. Increasing this number could result in errors when using asyncs on the service. You should implement a rate control system to help with the number of requests as this code is not optimized for large-scale games/projects

local TeleportMaxAttempts = 1
local TeleportRetryDelay = 1.5
local TeleportRetryBackOff = 1.25

-----------------



local TeleportService = game:GetService("TeleportService")
local MemoryStoreService = game:GetService("MemoryStoreService")


local module = {
	--Main functions(use these)
	["StartRaid"] = function(Data : SharedTable ,...)--First attempts to sumbit data to a mem store, will retry until max attempts have reached which it will then throw error and return the error, otherwise do the same with teleporting players aswell as clearing up any players that fail to be teleported
		assert(Data)
		assert(Data.Players)
		assert(Data.GameId)--id of the game that should be reserved, can be found by using "print(game.Id)"

		local PlayerTable = Data.Players--table that contains each player following the numeric index, [1] = "Player1",[2] = "Player2",[3] = "Roblox"... you get the point
		local ReservedServerID
		local Queue
		for i=1,5 do
		local Success,Error = pcall(function() 
				 ReservedServerID = TeleportService:ReserveServer(Data.GameId)
				 Queue = MemoryStoreService:GetSortedMap(tostring(ReservedServerID))--creates a memstore using the reserve servers id as the key,this makes it so when the server starts up it can easily pull data from the store within 5 lines
			end)
			if Success then
				break
			else
				if Error then
					warn(Error)
					task.wait(1.5)
				end
			end
		end

		local CurrentAsyncAttempts = 0
		local AsyncErrorLog = {}
		local AsyncSuccess = false

		for i=1, SetAsynctMaxAttempts do
			local Succes, Error = SetAsync(ReservedServerID,Data)
			if Succes then
				AsyncSuccess = true
				break
			else
				if Error then
					table.insert(AsyncErrorLog,"Failed to Set-Async Data. Error: ' "..Error.." '")
				end
				local DelayTime = SetAsyncRetryDelay + (CurrentAsyncAttempts * SetAsyncRetryBackOff)
				print("Set Async Failed, Retrying in: "..DelayTime.." seconds.")
				CurrentAsyncAttempts += 1
				task.wait(DelayTime)
			end
		end
		if AsyncSuccess == true then
			local TeleportErrorLog = {}
			for _,Player:Player in Data.Players do
				table.insert(TeleportErrorLog,Player.Name)
				task.spawn(function()
					local CurrentTeleportAttempts = 0
					local TeleportSucces = false

					for i=1, TeleportMaxAttempts do
						local Succes, Error = TeleportPlayer(Player,ReservedServerID,Data.GameId)
						if Succes then
							TeleportSucces = true
							break
						else
							if Error then
								table.insert(TeleportErrorLog[Player.Name],"Failed to teleport player. Error: ' "..Error.." '")
							end
							local DelayTime = SetAsyncRetryDelay + (CurrentTeleportAttempts * TeleportRetryBackOff)
							print("Teleport failed, Retrying in: "..DelayTime.." seconds.")
							CurrentTeleportAttempts += 1
							task.wait(DelayTime)
						end
					end
					if TeleportSucces == true then
						print(TeleportErrorLog[Player.Name])
					else
						error(TeleportErrorLog[Player.Name])
					end	
				end)
			end
		else
			error(AsyncErrorLog)
		end
	end,

	-- Other Functions(you should not really use these due to the nature of how they are designed...)
	["ForceStartRaid"] = function(Data : SharedTable ,...)--Force teleports players aswell as the party, returns error if either fails.
		--I aint coding this bruh, you do it.
	end,
}

function SetAsync(key:string,Data)
	assert(key)
	assert(Data)
	local Succes, Error = pcall(function() 
		local Queue = MemoryStoreService:GetSortedMap(key)
		Queue:SetAsync("PartyData",Data,QueueTime)
	end)
	return Succes,Error
end
function TeleportPlayer(Player:Player,ReserveID:string,GameID:number)
	assert(Player)
	assert(ReserveID)
	assert(GameID)
	local Succes,Error = pcall(function()
		local TeleportOptions = Instance.new("TeleportOptions")
		TeleportOptions.ShouldReserveServer = false
		TeleportOptions.ReservedServerAccessCode = ReserveID
		TeleportService:TeleportAsync(GameID,{Player},TeleportOptions)
	end)
	return Succes, Error
end
return module

Please let me know how I can improve my code, I also plan on using the feedback given to help adjust my coding style to be more professional and clean to look at. As of right now, if it’s not obvious this is spaghetti code.

3 Likes

I mean, if the script works as intended with no errors and you are able to read and understand it, then there is no reason to change anything.

1 Like

All of these are opinionated:

  • Getting of services should be before all else, in alphabetical order.

  • Constants immediatly after in SCREAMING_SNAKE_CASE

  • Spacing between characters (e.g i=1,5i = 1, 5), between explicit type specifiers (Player:PlayerPlayer: Player) and blocks:

assert(key)
assert(Data)

local Succes, Error = pcall(function() 
	local Queue = MemoryStoreService:GetSortedMap(key)
	Queue:SetAsync("PartyData",Data,QueueTime)
end)

return Succes,Error
  • No need to equality check boolean variables (if TeleportSucces == true then literally serves zero purpose, if TeleportSucces then is more readable, compact and efficient)

  • Local variables, functions in camelCase. (Objects, Services and Enums always in PascalCase)

  • All scripts ending on a new line

  • I prefer to use string interpolation in most cases

    print("Teleport failed, Retrying in: "..DelayTime.." seconds.")
    

    Replaced with (ignored the embeded code not formatting properly with string interoplation):

    print(`Teleport failed, Retrying in: {DelayTime} seconds.`)
    
module["StartRaid"] = function(Data : SharedTable ,...)

end

Can be more cleanly written as:

function module.StartRaid(Data: SharedTable, ...)

end

You made a spelling mistake on line 131 :tongue: SuccesSuccess.
My linter is warning me local variables PlayerData and Queue on lines 39 and 41 are unused.

Comments should only exist to explain why something is being done, not what it is actually doing (that much should be self documentating). Try to keep comments to a minimum and do not let lines exceed 100 characters in width.

Something like

local PlayerTable = Data.Players--table that contains each player following the numeric index, [1] = "Player1",[2] = "Player2",[3] = "Roblox"... you get the point

Can be more easily expressed with an explicit type instead of a comment.

local playerTable: {Player} = Data.Players

Code review is inteded for the submittion of already working code.

I like to follow the Roblox Lua Style guide

3 Likes

Hmm Yes, it does seem this one is quite a bit scuffed, however, I should have taken this post a few days ago as I decided to a more revised one. This one was extremely scuffed and rushed as I was trying to get this done before going to sleep.

In my revised version all my services have been defined at the top, and yes you are right about boolean variable checks, I’m not entirely sure why I needed == true part. My original code was extremely haystacked and unsightly to look at lol. In my revised code, I went ahead and just moved all the retry attempts into the actual function itself instead of calling the function over and over again, this made the main part of the code much shorter and cleaner to look at
Ex: if SetAsync(Parameters) then... you get the point.

After a brisk skim of Roblox Lua Style guide you provided, I think I will adopt this into my coding style. The rules provided are very well designed. Thank you for your insight, this advice will not be forgotten in future use :slight_smile:

1 Like