How could i improve this code?

The title says it all

local TweenService = game:GetService("TweenService")
local ContentProvider = game:GetService("ContentProvider")
------/
local Player = game:GetService("Players").LocalPlayer
local LoadingComponents = Player.PlayerGui:WaitForChild("SourceUI"):WaitForChild("MenuMain")
local Components = require(script:WaitForChild("Index"))
------/
local SoundData = {}
local WorldData = {}
local CurrentlyLoaded = 0
local Count = 0
------/
local InsetData = function()
	for _, Data in pairs(workspace:GetDescendants()) do
		if Data:IsA("Sound") then
			table.insert(SoundData, {Data, Data.Volume})
		end
		table.insert(WorldData, Data)
	end
end
------/
local StartLoading = function()
	for _, index in pairs(WorldData) do
		------/
		CurrentlyLoaded += 1
		LoadingComponents.AssetDisplay.Text = "Setting Up ".. index.ClassName.. "'s. ("..math.floor(_ / #WorldData * 100) .. "%)"
		------/
		if(_ == #WorldData) then
			LoadingComponents.AssetDisplay.Text = "Game has fully set."
		end
		task.wait(0.12)
		------/
	end
end
coroutine.wrap(function()
	while wait(3) do
		LoadingComponents.Tip.Text = "TIP : ".. Components.Tips[math.random(1, # Components.Tips)]
		for i = 0, 3 do
			LoadingComponents.Loading.Text = "LOADING" .. string.rep(".", i)
			wait(0.5)
		end
	end
end)()

InsetData()
StartLoading()
1 Like

Use task.wait() for all the waits.

1 Like

Like @D0RYU said, you should put this in the Help and Feedback > Code Review category.

For starters, assuming this is happening on the client, the script you are using isn’t even waiting to make sure the game has reached a point where it can load the rest of the game. You can use the https://developer.roblox.com/en-us/api-reference/function/DataModel/IsLoaded function and https://developer.roblox.com/en-us/api-reference/event/DataModel/Loaded event to make sure it has reached this stage for the client and wait for it to if it hasn’t. You can put this as the first executable line in the script:

if not game:IsLoaded() then game.Loaded:Wait() end -- waits until the game loads

You don’t even have to alter the rest of your code.

Next, your StartLoading function isn’t even loading the assets. All it’s doing is saying it is and then waiting 0.12 seconds in-between. This is fine if you only want the loading screen for show, but it you actually want to use a loading screen for what it’s intended you can use https://developer.roblox.com/en-us/api-reference/function/ContentProvider/PreloadAsync for loading the game assets instead. I’m guessing you want to use this though, because you reference both the https://developer.roblox.com/en-us/api-reference/class/TweenService and https://developer.roblox.com/en-us/api-reference/class/ContentProvider objects without ever utilizing them in your code.

You can ignore this advice, as truthfully if your code works and you are fine with it then that is okay. This is mainly pertaining the the organization of your code or lack thereof.

Code Organization

The strings you are using are mostly fine, as in the case of:

However, other strings are a bit less readable, as in the case of:

It can be easier to read and understand your code when you use String Formatting. This is especially true when you are re-using certain parts of the string and are only changing parts of it as the data changes. For example, you could use the above string and rewrite it as:

local setupString = "Setting Up %s's. (%i%%)" -- the string base format we use

--other code in-between

LoadingComponents.AssetDisplay.Text = setupString:format(index.ClassName, math.floor(_ / #WorldData * 100))

Now you only have to concern yourself with the variables in the string that concern you, such as the index.ClassName and percentage of the total completion.

This brings me to my next point, variable naming. This part is truly personal preference, but there are standard programming practices that are used to ensure organization. Variables should be named so the the person reading the code can understand what they are relating to. However, both the _ variable and the index variable in the for loop of your StartLoading function do not follow standard programming practice. Usually an underscore (_) is used in place of a variable name when that parameter value is not going to be used. Sort of like a garbage variable. Also, the index variable has a misleading name, as usually index is used to signify the value that ‘points to’ the actual value. However, you are using it as the actual value itself. This section was mostly just to inform you though, because most of what I’ve said will be made meaningless in the next paragraph.


In the for loop in the StartLoading function you are iterating through the WorldData table using pairs. However, because the WorldData variable is an array instead of a dictionary you should use ipairs. This logic doesn’t always hold true, as pairs is slightly faster than ipairs and should be used for more performance. However, that is only generally true if you do not need to iterate through the array in order. pairs concerns key-value pairs and ipairs concerns index-value pairs. To put it concisely, ipairs will iterate through an array in order while pairs provides no such guarantee. This is because of the way hash-table lookup works, although I do not know enough to adequately explain this. Because you are attempting to iterate through the table in a linear fashion, use ipairs. Otherwise your code could go from 1 to 75 to 32 to 100 ect.

You also don’t have to use coroutine.wrap since all you are doing is running a loop asynchronously. You can use task.spawn instead. What makes it worse is that you have no way of ending your while loop, as it has no conditions of execution except to wait a certain amount of time. You can add a boolean value for whether or not the game has finished loading and set it as the while loop’s condition. So, as long as the game has not loaded, you will still iterate. Then, once it has, the loop will stop executing. This can be written like this:

local isLoading = false

--other code in-between with the 'isLoading' value being set to false once everything has been loaded

task.spawn(function()
	while isLoading do
		-- code inside the while loop
	end
end)


This last piece of advice is pretty minimal. math.random, as I’m sure you know, has two parameters. These are the two numbers that the define the range where the function can generate a random integer. You are using it as:

This isn’t bad in any way, shape, or form. However, as seen on the documentation for the https://developer.roblox.com/en-us/api-reference/lua-docs/math library, the second parameter has a default value of 1, meaning that if you don’t provide a custom value it will be as if you put in 1. It doesn’t matter what order you put these two numbers in, so you can simplify this to this:

math.random(#Components.Tips)

Really it’s just less code to write to achieve the same effect. However, there is one downside to picking a random value like this, and that is that the value could be the same index as the current tip being shown. If you want to ensure that the value is always going to be a different one you can have a function and variable dedicated towards this purpose.

local currentTipIndex
local function GetRandomTip()
	local tipIndex = math.random(#Components.Tips)
	if tipIndex == currentTipIndex then
		return GetRandomTip()
	else
		currentTipIndex = tipIndex
		return Components.Tips[tipIndex]
	end
end


Resources:
Because I believe you are trying to make a loading screen, I highly recommend you check out roblox’s https://developer.roblox.com/en-us/articles/Custom-Loading-Screens tutorial. However, as I do not believe it provides a sufficient explanation, you can also use this video. The video used a server script, but you can use it in a local script all the same.
https://www.youtube.com/watch?v=3sJITgSCj_M