Modular Code Headache

Help and Feedback Code Review
1

Hello all. I’m currently making a “Website Manager” game, where you create a website, and manage it. You get the idea. I recently decided to rewrite the whole game code (which isn’t that much) because I felt there were too many scripts and it was too confusing. I wrote everything in a single script (mostly for fun, and one for the client and one for the server) but right now it feels very confusing. I made the code as modular as possible so I’d like your feedback on whether it’s actually optimal or not. Thanks for reading and possibly helping!

Note: The game is in very early stages, there’s a few placeholders and loopholes for doing things.

Server:

----- Services -----

local CollectionService = game:GetService("CollectionService")
local ServerScriptService = game:GetService("ServerScriptService")
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local Players = game:GetService("Players")

----- Modules -----

local Modules = game:GetService("ServerScriptService"):WaitForChild("Modules")
local ProfileService = require(Modules.ProfileService)

----- Remotes -----

local Remotes = ReplicatedStorage:WaitForChild("Remotes")
local RemoteFunctions = Remotes:WaitForChild("RemoteFunctions")
local RemoteEvents = Remotes:WaitForChild("RemoteEvents")
local BindableEvents = Remotes:WaitForChild("BindableEvents")
local BindableFunctions = Remotes:WaitForChild("BindableFunctions")

local RequestData = RemoteFunctions:WaitForChild("RequestData")
local ManageEmployees = RemoteEvents:WaitForChild("ManageEmployees")
local ManageWebsites = RemoteEvents:WaitForChild("ManageWebsites")
local ManageCampaigns = RemoteEvents:WaitForChild("ManageCampaigns")

----- Tables and Information -----
--[[EmployeeName = {
	Age,
	Salary,
	Level,
	Skill,
	CON,
	Gender
	}
]]
local ProfileTemplate = {
	Cash = 0,
	Employees = {
		["John Skywalker"] = {
			"John Skywalker",
			19,
			10000,
			15,
			"Programmer"
		}
	},
	Websites = {},
}

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

local ProfileStore = ProfileService.GetProfileStore("randomkey" , ProfileTemplate)

local Profiles = {}



----- Initialization -----

----- Functions -----

function UpdateCash(Profile, Amount)
	Profile.Data.Cash = Amount
	print(Profile.Data.Cash)
end

function HireEmployee(Player, Employee)
	table.insert(Profiles[Player].Data["Employees"], Employee)
	ManageEmployees:FireClient(Player, "Hire", Employee)
end

function FireEmployee(Player, Employee)
	table.remove(Profiles[Player].Data.Employees[Employee])
	ManageEmployees:FireClient(Player, "Fire", Employee)
end

local function LoadEmployees(Player)
	local EmployeeList = Profiles[Player].Data.Employees

	print("Loading " .. Player.Name .. "'s employees...")

	--Send list to client for loading on gui
	print(EmployeeList)
	local Employee = "John Skywalker"
	ManageEmployees:FireClient(Player, "Load", EmployeeList)
	--FireEmployee(Player, Employee)	
end
----- Website -----

	--[[
	local website = {
	
	[1]	["Name"] = dataGiven[1], -- Website Name
	[2]	["Type"] = dataGiven[2], -- Website Type - "Social Media", "Shopping Platform", "Browser Game"
	[3]	["OnlineUsers"] = 5,
	[4]	["Users"] = 69,
	[5]	["Features"] = {"feature1", "feature2"},
	[6]	["FeatureLevels"] = {10,10},
	[7]	["Framework"] = 1,
	[8]	["CloudServers"] = 1,
	[9]	[Marketing Campaigns] = {{campaign1, 100buckaroos, newspaper, 19secsleft},{campaign2, newspaper, 100buckaroos, 19secsleft}}
	}
	]]--

----- Website -----

local function GetWebsite(Player, WebsiteName)
	return Profiles[Player].Data.Websites[WebsiteName]
end

local function LoadWebsites(Player)
	
	local WebsiteList = Profiles[Player].Data.Websites

	print("Loading " .. Player.Name .. "'s websites..")

	print(WebsiteList)
	for _, Website in pairs(WebsiteList) do
		ManageWebsites:FireClient(Player, "Write", Website)
	end
end

local function CreateWebsite(Player, WebsiteInfo)
	local Website = {
		WebsiteInfo[1],
		WebsiteInfo[2],
		1,
		1,
		{"Quinta do Tio Manel", "Quinta do Tio Jaquim"},
		{10,10},
		"DingDong",
		1,
		{
			["1312312312312"] =	{
				"1312312312312",
				"Yo",
				"Noway",
				"420"
			}
		},
	}
	Profiles[Player].Data.Websites[Website[1]] = Website
	print("Created website " .. Website[1])
	ManageWebsites:FireClient(Player, "Create", Website)
end

local function RemoveWebsite(Player, WebsiteName)
	table.remove(Profiles[Player].Data.Websites[WebsiteName])
end

local WebsiteActions = {
	["Create"] = CreateWebsite,
	["Remove"] = RemoveWebsite,
}

local function ManageWebsite(Player, Action, Website)
	WebsiteActions[Action](Player, Website)
end

----- Connections -----

ManageWebsites.OnServerEvent:Connect(ManageWebsite)

----- Campaigns -----

local function CreateCampaign(Info)
	local Campaign = 
		{
			os.time(),
			Info[1],
			Info[2],
			420
		}
	
end

local CampaignActions = {
	["Create"] = CreateCampaign
}

local function ManageCampaign(Player, Action, Campaign)
	CampaignActions[Action](Campaign)
end

local DataActions = {
	["Website"] = GetWebsite
}


local function DataRequest(Player, DataType, Data)
	return DataActions[DataType](Player, Data)
end

----- Initialization -----
local function LoadProfile(Player)
	local Profile = ProfileStore:LoadProfileAsync("Player_" .. Player.UserId)
	if Profile ~= nil then
		Profile:AddUserId(Player.UserId) -- GDPR compliance
		Profile:Reconcile() -- Fill in missing variables from ProfileTemplate (optional)
		Profile:ListenToRelease(
			function()
				Profiles[Player] = nil
				-- The profile could've been loaded on another Roblox server:
				Player:Kick("Please rejoin the experience.")
			end
		)
		if Player:IsDescendantOf(Players) == true then
			Profiles[Player] = Profile
			-- A profile has been successfully loaded:
			warn(Player.Name .. "'s profile was loaded successfully.")
			warn("Initializing Data")
			LoadEmployees(Player)
		else
			-- Player left before the profile loaded:
			Profile:Release()
		end
	else
		-- The profile couldn't be loaded possibly due to other
		-- Roblox servers trying to load this profile at the same time:
		Player:Kick("Please rejoin the experience.")
	end
end

local function PlayerAdded(Player)
	LoadProfile(Player)
	repeat wait() until Profiles[Player].Data ~= nil
	LoadWebsites(Player)
	wait(10)
	print(Profiles[Player].Data.Websites)
end

----- Connections -----
RequestData.OnServerInvoke = DataRequest

Players.PlayerAdded:Connect(PlayerAdded)

Players.PlayerRemoving:Connect(
	function(Player)
		local Profile = Profiles[Player]
		if Profile ~= nil then
			Profile:Release()
			warn(Player.Name .. "'s profile was released successfully.")
		end
	end
)

----- Loops -----

Client:

----- Services -----

local CollectionService = game:GetService("CollectionService")
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local Players = game:GetService("Players")

----- Modules -----

----- Remotes -----

local Remotes = ReplicatedStorage:WaitForChild("Remotes")
local RemoteFunctions = Remotes:WaitForChild("RemoteFunctions")
local RemoteEvents = Remotes:WaitForChild("RemoteEvents")
local BindableEvents = Remotes:WaitForChild("BindableEvents")
local BindableFunctions = Remotes:WaitForChild("BindableFunctions")

local RequestData = RemoteFunctions:WaitForChild("RequestData")
local EmployeeManage = RemoteEvents:WaitForChild("ManageEmployees")
local ManageCampaigns = RemoteEvents:WaitForChild("ManageCampaigns")

----- Instances -----

local Main = script.Parent
local Templates = ReplicatedStorage:WaitForChild("UI"):WaitForChild("Templates")
local PlayerGui = script.Parent
local EmployeesGUI = Main:WaitForChild("Employees")
local MainUI = Main:WaitForChild("UI")
local MainBottom = MainUI:WaitForChild("Bottom")

----- Tables and Information -----

local function ClearScreen()
	for _, ScreenGui in pairs(script.Parent:GetChildren()) do
		if ScreenGui:IsA("ScreenGui") and ScreenGui.Name ~= "UI" then
			ScreenGui.Enabled = false
		end
	end
end

local function OpenWindow(WindowName)
	ClearScreen()
	Main:FindFirstChild(WindowName).Enabled = true
end

----- Employees -----

--[[Employee = {
	Name,
	Age,
	Salary,
	Level,
	Skill,
	CON,
	Gender
	}
]]

local EmployeeFrameTemplate = Templates:WaitForChild("EmployeeList")
local EmployeesMain = EmployeesGUI:WaitForChild("Main")
local ListFrame = EmployeesMain:WaitForChild("List")
local EmployeeButton = MainBottom:WaitForChild("Employees")

local function CreateEmployeeFrame(Employee)
	local Frame = EmployeeFrameTemplate:Clone()
	Frame.Name = Employee[1]
	Frame.Username.Text = Employee[1]
	Frame.Age.Text = Employee[2]
	Frame.Salary.Text = Employee[3]
	Frame.Level.Text = Employee[4]
	Frame.Status.Text = "Working"
	Frame.Visible = true
	Frame.Parent = ListFrame
end

local function RemoveEmployeeFrame(Employee)
	local EmployeeName
	print(typeof(Employee))
	if typeof(Employee) == "table" then
		EmployeeName = Employee[1]
	end
	if typeof(Employee) == "string" then
		EmployeeName = Employee
	end
	print(EmployeeName)

	ListFrame:FindFirstChild(EmployeeName):Destroy()
	print(EmployeeName .. " was fired for being a bad boy.")
end

local function LoadEmployees(EmployeeList)
	print("List is")
	print(EmployeeList)
	for _, Employee in pairs(EmployeeList) do
		print(Employee)
		CreateEmployeeFrame(Employee)
	end
end

local EmployeeActions = {
	["Load"] = LoadEmployees,
	["Hire"] = CreateEmployeeFrame,
	["Fire"] = RemoveEmployeeFrame
}

local function ManageEmployees(Action, Employee)
	EmployeeActions[Action](Employee)
end

local function OnEmployeeButtonClicked()
	OpenWindow("Employees")
end

--- Connections ---

EmployeeManage.OnClientEvent:Connect(ManageEmployees)
EmployeeButton.MouseButton1Click:Connect(OnEmployeeButtonClicked)

----- Websites -----

local ManageWebsites = RemoteEvents:WaitForChild("ManageWebsites")
local WebsiteGUI = PlayerGui:WaitForChild("Website")
local WebsitePage = WebsiteGUI:WaitForChild("Main")
local OpenWebsite = MainBottom:WaitForChild("Website")
local WebsiteButtonList = MainUI:WaitForChild("WebsiteList")
local ButtonTemplate = Templates:WaitForChild("WebsiteButtonTemplate")

local WebsiteList = {}
local CurrentWebsite = {}



local function RemoveWebsite(Website)
	WebsiteButtonList:FindFirstChild(Website[1]):Destroy()
end




--- Connections ---


----- Create Website -----

local CreateWebsiteGUI = Main:WaitForChild("CreateWebsite")
local NameBox = CreateWebsiteGUI.Main.WebsiteName
local CreateWebsiteWindowButton = MainUI:WaitForChild("CreateWebsite")


local function CheckName(Name)
	if string.len(Name) > 3 then
		return Name
	else
		return false
	end
end


local function OnCreateWebsite(ButtonName)
	local WebsiteInfo = {}
	warn(NameBox.Text)
	local GoodName = CheckName(NameBox.Text)
	if GoodName then
		--Good Name
		WebsiteInfo[1] = NameBox.Text
		WebsiteInfo[2] = ButtonName
		--Fire Event for website creation
		ManageWebsites:FireServer("Create", WebsiteInfo)
		--Close Creation Window
		CreateWebsiteGUI.Enabled = false
	else
		--Bad Name
		warn(NameBox.Text)
		warn(GoodName)
	end
	--Clear Name Box
	NameBox.Text = ""
end



----- Campaigns -----

--[[

Campaign = {

	[1] = Id
	[2] = Type
	[3] = Budget
	[4] = TimeLeft
	
}

]]
local MarketingFrame = WebsitePage.Marketing
local BudgetFrame = MarketingFrame.Top.Budget
local CampaignList = MarketingFrame:WaitForChild("ActiveCampaigns")
local CashBox = BudgetFrame:WaitForChild("CashBox")
local CampaignTemplate = Templates:WaitForChild("CampaignTemplate")

local MarketingSelection

function CreateCampaign()
	local MBudget = tonumber(CashBox.Text)
	if MarketingSelection then
		if type(MBudget) == "number" then
			if MBudget % 1 == 0 then
				print("Creating " .. MarketingSelection .. " campaign.")
				local CampaignInfo = {}
				CampaignInfo[1] = MarketingSelection
				CampaignInfo[2] = MBudget	
				ManageCampaigns:FireServer("Create", CampaignInfo)
			end
		end
	end
end
function CreateCampaignFrame(Campaign)
	local Frame = CampaignTemplate:Clone()
	Frame.Name = Campaign[1]
	Frame.name.Text = Campaign[2] .. " Campaign" 
	Frame.Budget.Text = Campaign[3] .. "$"
	Frame.TimeLeft.Text = 420
	Frame.TimeLeft.Activated.Value = true
	Frame.Visible = true
	Frame.Parent = CampaignList
end

local CampaignActions = {
	["Create"] = CreateCampaignFrame
}

local function ManageCampaigns(Action, Campaign)
	CampaignActions[Action](Campaign)
end


BudgetFrame.CreateCampaign.MouseButton1Click:Connect(CreateCampaign)



local function WriteWebsitePage(Site, Page)
	print(Site, Page)
	CurrentWebsite = Site
	--Clear the page
	for _, Frame in pairs(WebsitePage:GetChildren()) do
		if Frame:IsA("Frame") and Frame.Name ~= "Top" then
			Frame.Visible = false
		end
	end
	local Website
	if typeof(Site) == "string" then
		print("It's a string")
		Website = WebsiteList[Site]
		print(Website)
	elseif typeof(Site) == "table" then
		WebsiteList[Site[1]] = Site
		Website = Site		
	end
	if Website ~= nil then
		print(Website[1])
		if not WebsiteButtonList:FindFirstChild(Website[1]) then
			local Button = ButtonTemplate:Clone()
			Button.Name = Website[1]
			Button.Text = Website[1]
			Button.Parent = WebsiteButtonList
		end
		local Pages = {
			["Stats"] = function() 
				WebsitePage.Stats.Top.Left.OnlineUsers.Text = Website[3]
				WebsitePage.Stats.Top.Middle.Users.Text = Website[4]
				WebsitePage.Stats.Visible = true
			end,
			["Features"] = function() 
				local Features = {}
				local FeatureLevels = {}
				Features = Website[5]
				FeatureLevels = Website[6]

				WebsitePage.Features["1"].FName.Text = Features[1] or ""
				WebsitePage.Features["2"].FName.Text = Features[2] or ""
				WebsitePage.Features["3"].FName.Text = Features[3] or ""
				WebsitePage.Features["1"].Level.Text = FeatureLevels[1] or ""
				WebsitePage.Features["2"].Level.Text = FeatureLevels[2] or ""
				WebsitePage.Features["3"].Level.Text = FeatureLevels[3] or ""
				WebsitePage.Features.Visible = true
			end,
			["Marketing"] = function() 
				WebsitePage.Marketing.Visible = true
				print(Website[9])
				for _, Campaign in pairs(Website[9]) do
					print(Campaign[2])
					CreateCampaignFrame(Campaign)
				end
				--Load active campaigns
			end,
		}
		if Page then
			Pages[Page]()
		else
			Pages["Stats"]()
		end
		WebsitePage.Name = Website[1]
		WebsitePage.Top.WebsiteName.Text = Website[1]
		WebsitePage.Visible = true
		WebsiteGUI.Enabled = true
	end
end

----- Debug -----

local function CheckButtons()
	for _, Button in pairs(script.Parent:GetDescendants()) do
		if Button:IsA("TextButton") then
			Button.MouseButton1Click:Connect(function()
				local Name = Button.Name
				warn(Button.Name .. " was clicked.")
				if Button.Name == "Close" then
					ClearScreen()
				elseif Button.Parent == WebsiteButtonList then
					WebsitePage.Visible = true
					WriteWebsitePage(Name)
					CurrentWebsite = Name
				elseif Name == "Stats" or Name == "Features" or Name == "Marketing" then
					WriteWebsitePage(CurrentWebsite,Name)
				elseif Button == CreateWebsiteWindowButton then
					CreateWebsiteGUI.Enabled = true
				elseif Name == "Mobile Ads" or Name == "Text Ads" or Name == "Newspaper" then
					MarketingSelection = Name
					for _, Frame in pairs(Button.Parent.Parent:GetChildren()) do
						if Frame.Name == "Two" or Frame.Name == "Three" or Frame.Name == "Four" then
							Frame.BorderColor3 = Color3.fromRGB(27, 42, 53)
						end
					end				
					Button.Parent.BorderColor3 = Color3.fromRGB(0,255,0)
				elseif Button.Text == "Select" then
					OnCreateWebsite(Name)
				end
			end)
		end
	end
end

CheckButtons()

WebsiteButtonList.ChildAdded:Connect(CheckButtons)


local WebsiteActions = 
	{
		["Create"] = CreateWebsite,
		["Write"] = WriteWebsitePage,
		["Remove"] = RemoveWebsite
	}

local function ManageWebsite(Action, Website, Page)
	WebsiteActions[Action](Website, Page)
end


function CreateWebsite(Website)
	WebsiteList[Website[1]] = Website
	WriteWebsitePage(Website, "Stats")
	CurrentWebsite = Website
end

ManageWebsites.OnClientEvent:Connect(ManageWebsite)
1 Like
2

I’m impressed at this, especially when it comes to modular scripts. You’ve organized it with comments, and you laid out an example of the dictionaries! Bravo :clap:

4 Likes
3

Never have sections
They never provide any value to understanding the code. For example, the “Services” section is useless to indicate because one can quickly tell the following are services. Worse, it’s a standard convention to have services first! Hence, the comment provides no value to the reader.

Sections may be a sign that it could be its module. The “Remotes” section could be a module that exports all the remotes:

local Remotes = ReplicatedStorage:WaitForChild("Remotes")
local RemoteFunctions = Remotes:WaitForChild("RemoteFunctions")
local RemoteEvents = Remotes:WaitForChild("RemoteEvents")

return {
    RequestData = RemoteFunctions:WaitForChild("RequestData"),
    ManageEmployees = RemoteEvents:WaitForChild("ManageEmployees"),
    ManageWebsites = RemoteEvents:WaitForChild("ManageWebsites"),
    ManageCampaigns = RemoteEvents:WaitForChild("ManageCampaigns"),
}

The primary problem I’m noticing is that you’re taking the idea of putting everything in one script too far. It’s correct to limit the number of client and server scripts you have. Those scripts primarily do initialization work (e.g connecting to events). But, organize code by using module scripts.

Another example is all the GUI-related functionality in their modules.

Hoist variables to the top
I notice more module-scope variables are being initalized throughout the module. It’s always best practice to put all your constants and module-scope variables at the top. That way, we always know where to find the definition of a variable. Having them scattered tends to cause more confusion.

Good things
Besides the sections, the comments provided are excellent. They help document how tables are supposed to be shaped and provide the why to the code. The variables are also well, so it makes sense.

Exception to good naming

Why do Campaign and Employee tables use numerical indexes instead of string indexes? Intuitively, string indexes would be more informative. Numerical indexes strip that information away.

Overall, this is OK—you’re onto something good ;).

1 Like
4

To be honest, I never use a normal table for stats or properties. I rather just use a whole dictionary with strings to name the property. It just saves time and makes it look less confusing.
Just preferences though. ¯\_(ツ)_/¯

1 Like