Feedback on my module called from local script

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 mainFrame = shopUI:WaitForChild("Frame")

local pointsFrame = mainFrame.Points.PointsFrame
local itemsFrame = mainFrame.Items.ItemsFrame.ScrollingFrame
local passesFrame = mainFrame.Passes.GamepassFrame
local viewFrames = mainFrame.ViewFrames

local categories = mainFrame.Categories.ButtonBar:GetChildren()

local COLOR_DEFAULT = Color3.fromRGB(74, 83, 85)
local COLOR_CHANGE = Color3.fromRGB(115, 188, 214)

local function hideFrames()	
	for _, viewFrames in ipairs(viewFrames:GetChildren()) do
		viewFrames.Visible = false
	end
 	pointsFrame.Visible = false
	itemsFrame.Visible = false
	passesFrame.Visible = false
end

local function viewFrames(frame)
	hideFrames()
	frame.Visible = true
end

local function changeColorDefault()
	for _, category in ipairs(categories) do
		category.ImageColor3 = COLOR_CHANGE
	end
end

local function changeColor(frame)
	changeColorDefault()
	frame.ImageColor3 = COLOR_DEFAULT
end

for _, category in ipairs(categories) do
	
	local button = category:FindFirstChild("Button")
	
	button.Activated:Connect(function()
		
    if button.Name == "Items" then
			ReplicatedStorage.Audio.ClickSound:Play()
			changeColor(category)
	        viewFrames(itemsFrame)
		
	elseif button.Name == "Passes" then
			ReplicatedStorage.Audio.ClickSound:Play()
			changeColor(category)
	        viewFrames(passesFrame)
		
	elseif button.Name == "Points" then
			ReplicatedStorage.Audio.ClickSound:Play()
			changeColor(category)
	        viewFrames(pointsFrame)
		 end
	end)
end

return hideFrames
#2

The module, on a logical level, seems pretty straightforward. Good job there!

However, feedback that points out only the good is not helpful, nor does it have the intended effect; no one asks for feedback on such a technical level only to hear they did well.

The first thing that sticks out is the inconsistency on how specific items are named. It appears the naming schemes of PascalCase and camelCase get used conversely without any obvious reasoning. At the very least, do not consider the naming style used conventional and lends itself to the problem of being confusing to most outsiders. As a general rule of styling Luau code in the same way Roblox internal style guide advocates for, the following style should get considered when naming items.

  • Use PascalCase for class and enum-like objects as well as Roblox services.
  • Use camelCase for local variables, public member values, and functions.
  • Since Luau does not support constants, denote any value that should get treated as an immutable pointer with an immutable value as LOUD_SNAKE_CASE. This naming scheme gets used by convention to annotate for other developers if a table, number, datatype, et cetera should not be augmented.
  • While not valuable in this context, private (and protected if practised) member values use camelCase prefixed with an underscore. This naming practise is to annotate the pointer’s immutability outside of the class’s declaration.

Another point to consider is initializing Roblox datatypes. Instead of assigning every frame to the same colour on the spot, consider assigning a constant local variable assigned to the said colour. Then, using this constant, assign each frame’s colour to that variable. When done correctly, it removes most of the performance overhead introduced when constructing the datatype object repeatedly.

It is safe to assume this is some module built to manage UI elements considering the given context. If this is the case, defer the side-effect found in this script to a LocalScript designed to manage the UI elements. If this gets decided against and these functions are not needed elsewhere, refactor the given ModuleScript into a LocalScript. Again, it makes no sense to import a function called “hide frames” and, when it is loaded, suddenly every text button is initialized. To aid the project’s readability, adopt a manager pattern that controls the entirety of one aspect of a game, in this case, the UI elements.

Like stated earlier, at a logical level, there are no complaints to be had when analyzed somewhat thoroughly.

I appreciate your frequent posts as of recent in code review! A developer that is willing to put themselves out there to get valuable feedback and accept criticism as it is; it’s a desirable trait! Just wanted to give credit where it’s due. If you have any further questions, either myself or another member of the community will be glad to help, friend.

1 Like
#3

Thank you for your feed back! Here’s the improved one…

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 mainFrame = shopUI:WaitForChild("Frame")

local pointsFrame = mainFrame.Points.PointsFrame
local itemsFrame = mainFrame.Items.ItemsFrame.ScrollingFrame
local passesFrame = mainFrame.Passes.GamepassFrame
local viewFrames = mainFrame.ViewFrames

local categories = mainFrame.Categories.ButtonBar:GetChildren()

local COLOR_DEFAULT = Color3.fromRGB(74, 83, 85)
local COLOR_CHANGE = Color3.fromRGB(115, 188, 214)

local function hideFrames()	
	for _, viewFrames in ipairs(viewFrames:GetChildren()) do
		viewFrames.Visible = false
	end
 	pointsFrame.Visible = false
	itemsFrame.Visible = false
	passesFrame.Visible = false
end

local function viewFrames(frame)
	hideFrames()
	frame.Visible = true
end

local function changeColorDefault()
	for _, category in ipairs(categories) do
		category.ImageColor3 = COLOR_CHANGE
	end
end

local function changeColor(frame)
	changeColorDefault()
	frame.ImageColor3 = COLOR_DEFAULT
end

for _, category in ipairs(categories) do
	
	local button = category:FindFirstChild("Button")
	
	button.Activated:Connect(function()
		
    if button.Name == "Items" then
			ReplicatedStorage.Audio.ClickSound:Play()
			changeColor(category)
	        viewFrames(itemsFrame)
		
	elseif button.Name == "Passes" then
			ReplicatedStorage.Audio.ClickSound:Play()
			changeColor(category)
	        viewFrames(passesFrame)
		
	elseif button.Name == "Points" then
			ReplicatedStorage.Audio.ClickSound:Play()
			changeColor(category)
	        viewFrames(pointsFrame)
		 end
	end)
end

return hideFrames

I don’t really get by “It makes no sense to import a function called “hide frames””

It’s a function I made so that whenever a frame opens ( on the activated events ), the other frames are needed to be un-visible.

#4

I didn’t quite say that it makes no sense to import that function from this ModuleScript for external use:

In another script, the following will occur:

-- We import a function and, without any indication, many side-effects
-- occur before the function ever gets invoked.
local hideFrames = require(Path.To.Module);
1 Like
#5

The hideframes function has different uses in different modules. It does’t have the same use in different modules.

For example:

Module 1 has a hide frames function for other stuff.

Module 2 has a hide frames function for other stuff.

#6

This is the side-effect in question I’ve been referring to.

1 Like