Feedback on my module used for functions. Is it clean, is it efficient?

Help and Feedback Code Review
,
1 
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local Players = game:GetService("Players")

local player = Players.LocalPlayer
local playerGui = player:WaitForChild("PlayerGui")

local shopUI = playerGui:WaitForChild("ShopUI")
local sideUI = playerGui:WaitForChild("SideUI")
local upgradeUI = playerGui:WaitForChild("UpgradeUI")
local clickSound = ReplicatedStorage.Audio.ClickSound

local shopMain = shopUI:WaitForChild("Frame")

local categories = shopMain.Categories.ButtonBar
local itemFrame = shopMain.Items.ItemsFrame
local pointsFrame = shopMain.Points.PointsFrame
local passes = shopMain.Passes.GamepassFrame
local viewFrames = shopMain.ViewFrames

local COLOR_NEW = Color3.fromRGB(115, 188, 214)
local COLOR_DEFAULT = Color3.fromRGB(76, 83, 85)

local MEDKIT_LEVEL = 10
local CROWBAR_LEVEL = 25
local BACKPACK_LEVEL = 50
local KEY_DETECTOR_LEVEL = 100

local functions = {}

function functions.changeCategoryColor(categoryFrame, frame)
	clickSound:Play()
	for _, categoryFrame in ipairs(categories:GetChildren()) do
		categoryFrame.ImageColor3 = COLOR_DEFAULT
	end
	frame.ImageColor3 = COLOR_NEW
end

function functions.changeButtonColor(sideUIbutton)
	for _, buttons in ipairs(sideUI:GetChildren()) do
		buttons.ImageColor3 = COLOR_DEFAULT
	end
	sideUIbutton.ImageColor3 = COLOR_NEW
end

function functions.viewItemFrame(frame)
	clickSound:Play()
	for _, viewFrame in ipairs(viewFrames:GetChildren()) do
		viewFrame.Visible = nil
	end
	itemFrame.Visible = nil
	frame.Visible = true
end

function functions.viewCategoryFrame(frame)
	clickSound:Play()
	for _, viewFrame in ipairs(viewFrames:GetChildren()) do
		viewFrame.Visible = nil
	end
	pointsFrame.Visible = nil
	itemFrame.Visible = nil
	passes.Visible = nil
	frame.Visible = true
end

function functions.viewBack(frame)
	clickSound:Play()
	for _, viewFrame in ipairs(viewFrames:GetChildren()) do
		viewFrame.Visible = nil
	end
    itemFrame.Visible = true
end

function functions.viewScreenGUI(screenGui)
	clickSound:Play()
	local guisToUnView = {upgradeUI, shopUI}
	for _, guis in ipairs(guisToUnView) do
		guis.Enabled = false
	end
	screenGui.Enabled = true
end

function functions.levelHandle(medKitName, crowbarName, backpackName, KeyDetectorName)
	
	player.leaderstats.Level.Changed:Connect(function()
		
	for _, frame in ipairs(itemFrame.ScrollingFrame:GetChildren()) do
	   if frame:IsA("ImageLabel") then
				
				if frame.Name == medKitName then
					if player.leaderstats.Level.Value >= MEDKIT_LEVEL then
					frame.LockedFrame:Destroy()
					frame.BuyFrame.Visible = true
				end	
					
				elseif frame.Name == crowbarName then
					   if player.leaderstats.Level.Value >= CROWBAR_LEVEL then
					   frame.LockedFrame:Destroy()
					   frame.BuyFrame.Visible = true
					end
					
				elseif frame.Name == backpackName then
					   if player.leaderstats.Level.Value >= BACKPACK_LEVEL then
					   frame.LockedFrame:Destroy()
					   frame.BuyFrame.Visible = true
					end
					
				elseif frame.Name == KeyDetectorName then
					   if player.leaderstats.Level.Value >= KEY_DETECTOR_LEVEL then
					   frame.LockedFrame:Destroy()
					   frame.BuyFrame.Visible = true
					end
			    end		
			end
		end
	end)
end

function functions.updateText(text)
	text.Text = tostring(player.leaderstats.Points.Value)
end

return functions

Here’s what I want reviewed.

Script efficiency, will my script impact performance?
Are my variables efficient?

Here’s what I don’t want reviewed.

Script readbility.

Just a quick note to help you save up some time!

2

I’ll throw my two cents worth of commentary on this:

if frame.Name == medKitName then
	if player.leaderstats.Level.Value >= MEDKIT_LEVEL then
	frame.LockedFrame:Destroy()
	frame.BuyFrame.Visible = true
end

This is the only thing that caught my attention. From my understanding of the “Client-Server” model of Roblox, checking something such as a user’s level should be handled server-side. Otherwise, a hacker could easily bypass level restrictions by simply removing the “LockedFrame” themselves, using an injected script.

Other than that, to me your code looks very solid. :+1:

2 Likes
3

Preface
The more I look at the code the more it smells. There is too much going on at once and the functions don’t really belong together. It appears as if these functions should be placed in their own modules.

For example, you defined a function named “viewBack” with a parameter named “frame” even though you never use it.

Another, “viewScreengUI” has a parameter for a screenGui instead of a string name of it and furthermore, you create a table named “guisToUnView” despite the fact it will always remain the same in the runtime. That’s wasted time creating and destroy a table whenever this function is invoked for no reason.

I want to continue saying that, you should never optimize your code unless it is truly needed. Readability always comes first before doing these micro-optimizations. And, looking at the code, it really needs improvements on organization and readability. You should be following PascalCase (since Roblox uses it), improve your code’s modularity, and in general clean it up. In the future, this code will not be maintainable at all.

But, I can still provide helpful insight through the mess for the future.

Security gap
I want to firstly extend on what @darthbl0xx said, unless you have checks to stop an exploiter from buying items they shouldn’t be able to buy…This is quite a massive gap in security. You should never, ever trust the client to be innocent at all.

Constants
Your constants are probably in the wrong location…

local COLOR_NEW = Color3.fromRGB(115, 188, 214)
local COLOR_DEFAULT = Color3.fromRGB(76, 83, 85)

local MEDKIT_LEVEL = 10
local CROWBAR_LEVEL = 25
local BACKPACK_LEVEL = 50
local KEY_DETECTOR_LEVEL = 100

I’m talking about these constants. These should all be moved to its own, global module script that you can require. Mainly because these constants both apply on the server and client (adhering to DRY). For example, placing this module in replicated storage:

-- Semicolons function the same as commas in table constructors.
local CONSTANTS =
{
    COLOR_NEW = Color3.fromRGB(115, 188, 214);
    COLOR_DEFAULT = Color3.fromRGB(76, 83, 85);

    MEDKIT_LEVEL = 10;
    CROWBAR_LEVEL = 25;
    BACKPACK_LEVEL = 50;
    KEY_DETECTOR_LEVEL = 100;
}

-- Don't want anyone to modify the constants,
-- Metatables can help.
local ErrorMsg = "Attempted to modify %s index. Constants are immutable!"
local metatable =
{
    __newindex = function(key)
        error(ErrorMsg:format(key))
    end;
    __metatable = "locked";
}

return setmetatable(CONSTANTS, metatable)

Now, both of your servers and client can access constants. There is now a single point of truth if you will for constants. Also, less memory is used to store the same constants and less painful to find them.

Note

In case you don’t know, the server is actually a client as well. It’s just that it manages everything, sort of as a special client. For example, it replicates everything it has to everyone else.

If you require this from any client, it will not get the same table from another client.

Repeating code
This code block appeared way too much in the function functions.levelHandle:

if player.leaderstats.Level.Value >= [some constant] then
frame.LockedFrame:Destroy()
frame.BuyFrame.Visible = true

Put it in its’ function like so:

local function destroyLockedFrame(frame, threshold)
    if player.leaderstats.Level.Value >= threshold then
    frame.LockedFrame:Destroy()
    frame.BuyFrame.Visible = true
end

It’s easier on you whenever you modify the code. So, the efficiency gained for you. Plus, organized nicer.

Look-up tables
I notice at the function functions.levelHandle(..) you compare against one case a lot of times to run certain blocks of code with if ... else if .... Now, this is inefficient in terms of execution speed because you end up comparing values until you reach the correct case. A solution to this is to use look-up tables. This exchanges memory for increased execution speed. Additionally, it is more organized anyways :>.

First, let’s make it easier on ourselves and curry the new function we made above. It’s so that we only need to pass the frame and not the frame as well.

local function curriedDestroyLockedFrame(threshold)
    return function(frame)
        DestroyLockedFrame(frame, threshold)
    end
end

The lookup table itself:

local lookUpTable =
{
    [medKitName] = curriedDestroyLockedFrame(MEDKIT_LEVEL);
    [crowbarName] = curriedDestroyLockedFrame(CROWBAR_LEVEL);
    [backpackName] = curriedDestroyLockedFrame(BACKPACK_LEVEL);
    [KeyDetectorName] = curriedDestroyLockedFrame(KEY_DETECTOR_LEVEL);
}

In code:

function functions.levelHandle(medKitName, crowbarName, backpackName, KeyDetectorName)
    local lookUpTable =
    {
        [medKitName] = curriedDestroyLockedFrame(MEDKIT_LEVEL);
        [crowbarName] = curriedDestroyLockedFrame(CROWBAR_LEVEL);
        [backpackName] = curriedDestroyLockedFrame(BACKPACK_LEVEL);
        [KeyDetectorName] = curriedDestroyLockedFrame(KEY_DETECTOR_LEVEL);
    }

    player.leaderstats.Level.Changed:Connect(function()
        for _, frame in ipairs(itemFrame.ScrollingFrame:GetChildren()) do
            if frame:IsA("ImageLabel") then
                if frame:IsA "ImageLabel" then
                    lookUpTable[frame.Name](frame)
                end
            end
        end
    end
end

Not only it’s more readable, but it’s also quicker through the loop as it simply does a lookup.

Closure
Practice makes perfect and you still have a lot to practice. I suggest you start drawing the architecture of the code beforehand so it’s better organized. It’s at the point which you should start learning to program and not how to code in the language.

2 Likes
4

This really helps, wasn’t quite sure I was using arguments for nothing. Thank you, I will optimize my code right now and be right back once done.

5

Yes, I know. I don’t want to send a remote event every time the leaderstats level is changed. Even if they pass the locked frame, they can’t buy it since that’s handled on the server. :wink:

6

I do disagree with you on the readability but I have made changes.

1 Like