Code smell - mix of GUI and game logic

Help and Feedback Code Review
#1

The following code is under a TextLabel that displays a count of live Robot clones (kept in the Robots folder in the workspace). The way it is coded now works, but the logic of changing the player’s level in their leaderstats is kind of buried under the GUI display logic. That smells like bad code organization to me. But I’ve only been scripting Roblox for a short while so I’m not sure how to improve this. For example, would the use of Bindable or Remote Events make sense here? Any suggestions for improvement would be appreciated. Thanks in advance.

textLabel = script.Parent
player = game:GetService("Players").LocalPlayer

ROBOTSTOLEVELUP = 50

while true do
	task.wait(1)
	clones = workspace.Robots:GetChildren()
	playerCloneCount = 0
	for _, clone in ipairs(clones) do
		id = clone:GetAttribute("PlayerUserId")
		if id == player.UserId then
			playerCloneCount += 1
			if playerCloneCount >= ROBOTSTOLEVELUP then
				if player.leaderstats.Level.Value < 2 then
					player.leaderstats.Level.Value = 2
				end
			end
		end
	end
	textLabel.Text = "Robots: " .. playerCloneCount .. " of " .. #clones
end
#2

You can use workspace.ChildAdded and workspace.ChildRemoved instead of a loop if you want the GUI/variable to be updated when a clone is added and removed.

I’d also recommend making all variables local, it makes your code easier to read and sometimes run a bit faster. You do need to know about/manage their scope if you do that though, so suit yourself.

Having your GUI update in the same place as where you change your variable is fine, though if you’d like you could do:

local level = player.leaderstats.Level.Value
level.Changed:Connect(function()
	local cloneCount = #workspace.Robots:GetChildren()
	textLabel.Text = "Robots: "..playerCloneCount.." of "..cloneCount 
end)
2 Likes
#3

Thank you for the excellent suggestions. I can’t believe I forgot to make the variables local. Fortunately I haven’t been doing that in my other scripts. I made some changes to the code, which I’ve posted below. I also moved it to StarterPlayerScripts rather than having it buried under the TextLabel. I’m much happier with the new version.

local player = game:GetService("Players").LocalPlayer

local playerCloneCount = 0

local ROBOTSTOLEVELUP = 50

local function updateCloneCount(child, value)
	local id = child:GetAttribute("PlayerUserId")
	if id == player.UserId then
		playerCloneCount += value
		local totalCloneCount = #workspace.Robots:GetChildren()
		local textLabel = player.PlayerGui.ScreenGui.RobotsTextLabel
		textLabel.Text = "Robots: " .. playerCloneCount .. " of " .. totalCloneCount
		if playerCloneCount >= ROBOTSTOLEVELUP then
			if player.leaderstats.Level.Value < 2 then
				player.leaderstats.Level.Value = 2
			end
		end
	end
end

workspace.Robots.ChildAdded:Connect(function(child)
	updateCloneCount(child, 1)
end)

workspace.Robots.ChildRemoved:Connect(function(child)
	updateCloneCount(child, -1)
end)
1 Like
#4

Unfortunately, this approach is not working properly when a player dies and then spawns back into the game. I can’t come up with a workable solution so I may have to go back to the while loop approach.

#5

This is where I’m at with the code:

local player = game:GetService("Players").LocalPlayer

local textLabel = script.Parent

local ROBOTSTOLEVELUP = 50

while true do
	task.wait(1)
	local clones = workspace.Robots:GetChildren()
	local playerCloneCount = 0
	for _, clone in ipairs(clones) do
		local id = clone:GetAttribute("PlayerUserId")
		if id == player.UserId then
			playerCloneCount += 1
			if playerCloneCount >= ROBOTSTOLEVELUP then
				if player.leaderstats.Level.Value < 2 then
					player.leaderstats.Level.Value = 2
				end
			end
		end
	end
	textLabel.Text = "Robots: " .. playerCloneCount .. " of " .. #clones
end
1 Like
#7

I believe this is from ScreenGui.ResetOnSpawn being set to true. To fix this, you can set ScreenGui.ResetOnSpawn to false or add a call to updateCloneCount. I’d recommend adding the call, and also using a function that just updates the gui and clone count, instead of updating them based on function parameters.

local player = game:GetService("Players").LocalPlayer

local playerCloneCount = 0

local ROBOTSTOLEVELUP = 50

local function updateCloneCountAndGui()
	local clones = workspace.Robots:GetChildren() --This is your old code put into the function
	local playerCloneCount = 0
	for _, clone in ipairs(clones) do
		local id = clone:GetAttribute("PlayerUserId")
		if id == player.UserId then
			playerCloneCount += 1
			if playerCloneCount >= ROBOTSTOLEVELUP then
				if player.leaderstats.Level.Value < 2 then
					player.leaderstats.Level.Value = 2
				end
			end
		end
	end
	textLabel.Text = "Robots: " .. playerCloneCount .. " of " .. #clones
end

updateCloneCount(child) -- Call when the code starts

workspace.Robots.ChildAdded:Connect(function()
	updateCloneCountAndGui()
end)

workspace.Robots.ChildRemoved:Connect(function()
	updateCloneCountAndGui()
end)

@IvyInPairs, I don’t think they’re making changes to leaderstats in the script, just getting the value of a leaderstat. Your solution would solve that problem though :+1:

My bad! You’re right:

“player.leaderstats.Level.Value = 2”

Using remote events does add a vulnerability, ideally you would use a server script to set the level. Basically, use the same code you have for ‘updateCloneCountAndGui’ but without the Gui part from the server inside a connection to Players.PlayerAdded using the player parameter of that event.

1 Like
#8

Thank you so much for all these insights. I think the code smell came about when I added the “leveling up” code to what should have been a strictly local GUI updating script. So I’ve removed that aspect from the code and made sure to call the update function when the code starts to take care of the ResetOnSpawn situation:

local Players = game:GetService("Players")

local player = Players.LocalPlayer
local textLabel = script.Parent

local function updateCloneCount()
	local clones = workspace.Robots:GetChildren()
	local playerCloneCount = 0
	for _, clone in ipairs(clones) do
		local id = clone:GetAttribute("PlayerUserId")
		if id == player.UserId then
			playerCloneCount += 1
		end
	end
	textLabel.Text = "Robots: " .. playerCloneCount .. " of " .. #clones
end

updateCloneCount()

workspace.Robots.ChildAdded:Connect(function()
	updateCloneCount()
end)

workspace.Robots.ChildRemoved:Connect(function()
	updateCloneCount()
end)

Now I need to figure out how I want to handle leveling up outside of this GUI-only client code.

#9

I’ve got the GUI updating working fine now. It updates the text labels for the Robot count, Zombie count and Soldier count. And when level 2 is reached (based on how many user-created Robots are alive at once) it makes a “teleport” button visible.

Here are my various scripts. The basic idea of the game is that you create a robot clone by touching a “magic diamond”. If a robot touches a magic diamond then a Zombie clone gets created. And if a zombie touches a magic diamond then a Soldier clone gets created.

RobotsCount script:

local Players = game:GetService("Players")

local player = Players.LocalPlayer
local textLabel = script.Parent

local function updateCloneCount()
	local clones = workspace.Robots:GetChildren()
	local playerCloneCount = 0
	for _, clone in ipairs(clones) do
		local id = clone:GetAttribute("PlayerUserId")
		if id == player.UserId then
			playerCloneCount += 1
		end
	end
	textLabel.Text = "Robots: " .. playerCloneCount .. " of " .. #clones
end

updateCloneCount()

workspace.Robots.ChildAdded:Connect(function()
	updateCloneCount()
end)

workspace.Robots.ChildRemoved:Connect(function()
	updateCloneCount()
end)

TeleportButtonScript:

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

local button = script.Parent
local player = Players.LocalPlayer
local targetPlaceID = 7335388677

local levelChangedEvent = ReplicatedStorage:WaitForChild("LevelChangedEvent")
local teleportEvent = ReplicatedStorage:WaitForChild("TeleportEvent")

levelChangedEvent.OnClientEvent:Connect(function(level)
	if level >= 2 then
		button.Visible = true
	else
		button.Visible = false
	end
end)

button.Activated:Connect(function()
	teleportEvent:FireServer(targetPlaceID)
end)

MagicDiamondManager (server script):

local Players = game:GetService("Players")
local ServerStorage = game:GetService("ServerStorage")

local magicDiamondsFolder = workspace:WaitForChild("Magic_Diamonds")
local magicDiamonds = magicDiamondsFolder:GetChildren()

local COOLDOWN = 2
local ROBOTS_TO_LEVEL_UP = 5 --50

local function updateLeaderstats(player)
	player.leaderstats.Points.Value += 1
	local clones = workspace.Robots:GetChildren()
	local playerCloneCount = 0
	for _, clone in ipairs(clones) do
		local id = clone:GetAttribute("PlayerUserId")
		if id == player.UserId then
			playerCloneCount += 1
		end
	end
	if playerCloneCount >= ROBOTS_TO_LEVEL_UP then
		if player.leaderstats.Level.Value < 2 then
			player.leaderstats.Level.Value = 2
		end
	end
end

local function onDiamondTouched(otherPart, diamond)
	if diamond:GetAttribute("Enabled") then
		local character = otherPart.Parent
		local humanoid = character:FindFirstChildWhichIsA("Humanoid")
		local player = Players:GetPlayerFromCharacter(character)
		local clone
		if humanoid then
			local name = humanoid.Parent.Name
			if name == "Robot" then
				clone = ServerStorage.Zombie:Clone()
				clone.Configuration.AttackDamage.Value = math.random(10, 20)
				clone.Configuration.PatrolRadius.Value = math.random(20, 200)
				diamond.Color = BrickColor.new("Really red").Color
			elseif name == "Zombie" then
				clone = ServerStorage.Soldier:Clone()
				clone.Configuration.AttackDamage.Value = math.random(10, 20)
				clone.Configuration.AttackDelay.Value = 0.5
				clone.Configuration.ClipCapacity.Value = 8
				clone.Configuration.PatrolRadius.Value = math.random(200, 400)
				clone.Configuration.ReloadDelay.Value = 3
				diamond.Color = BrickColor.new("Lime green").Color
			elseif name == "Soldier" then
				clone = ServerStorage.Robot:Clone()
				clone.Configuration.PatrolRadius.Value = math.random(400, 2000)
				--local dialog = getRobotDialog()
				--dialog.Parent = clone.Head
				diamond.Color = BrickColor.new("Medium stone gray").Color
			else -- Local Player clones a robot.
				clone = ServerStorage.Robot:Clone()
				clone.Configuration.PatrolRadius.Value = math.random(40, 400)
				--local dialog = getRobotDialog()
				--dialog.Parent = clone.Head
				diamond.Color = BrickColor.new("Medium stone gray").Color
			end
			local particleEmitter = diamond.Parent.DiskWithParticles.ParticleEmitter
			clone.HumanoidRootPart.Position = diamond.Position
			if player then
				clone:SetAttribute("PlayerUserId", player.UserId)
			end
			local foldername = clone.Name .. "s"
			clone.Parent = workspace[foldername]
			if player then
				updateLeaderstats(player)
			end
			diamond:SetAttribute("Enabled", false)
			particleEmitter.Enabled = false
			wait(COOLDOWN)
			diamond:SetAttribute("Enabled", true)
			diamond.Color = BrickColor.new("Medium stone gray").Color
			particleEmitter.Enabled = true
		end
	end
end
for _, magicDiamond in ipairs(magicDiamonds) do
	local diamond = magicDiamond.Diamond
	local particleEmitter = diamond.Parent.DiskWithParticles.ParticleEmitter
	diamond:SetAttribute("Enabled", true)
	particleEmitter.Enabled = true
	diamond.Touched:Connect(function(otherPart)
		onDiamondTouched(otherPart, diamond)
	end)
end

LeaderboardServerEvents (server script):

local ReplicatedStorage = game:GetService("ReplicatedStorage")

local PlayerPoints = game:GetService("DataStoreService"):GetDataStore("PlayerPoints")
local PlayerLevel = game:GetService("DataStoreService"):GetDataStore("PlayerLevel")

local levelChangedEvent = ReplicatedStorage:WaitForChild("LevelChangedEvent")

local STARTING_POINTS = 0
local STARTING_LEVEL = 1

game.Players.CharacterAutoLoads = false

local function onHumanoidDied(humanoid, character, player)
	player.leaderstats.Points.Value = STARTING_POINTS
	player.leaderstats.Level.Value = STARTING_LEVEL
	player:LoadCharacter()
end

local function onCharacterAdded(character, player)
	local humanoid = character:WaitForChild("Humanoid")
	humanoid.Died:Connect(function()
		onHumanoidDied(humanoid, character, player)
	end)
end

local function setupLeaderboard(player)
	local folder = Instance.new("Folder")
	folder.Name = "leaderstats"
	folder.Parent = player
	local points = Instance.new("IntValue")
	points.Name = "Points"
	points.Parent = folder
	local success, savedPlayerPoints = pcall(function()
		return PlayerPoints:GetAsync(player.UserId)
	end)
	if success then
		if savedPlayerPoints then
			points.Value = savedPlayerPoints
		else
			points.Value = STARTING_POINTS
		end
	end
	local level = Instance.new("IntValue")
	level.Name = "Level"
	level.Parent = folder
	-- Notify the client so GUI elements can be toggled, for example, when the level changes.
	level.Changed:Connect(function(newValue)
		levelChangedEvent:FireClient(player, newValue)
	end)
	local success, savedPlayerLevel = pcall(function()
		return PlayerLevel:GetAsync(player.UserId)
	end)
	if success then
		if savedPlayerLevel then
			level.Value = savedPlayerLevel
		else
			level.Value = STARTING_LEVEL
		end
	end
end

local function onPlayerAdded(player)
	setupLeaderboard(player)
	player.CharacterAdded:Connect(function(character)
		onCharacterAdded(character, player)
	end)
	player:LoadCharacter()
end
game.Players.PlayerAdded:Connect(onPlayerAdded)

local function onPlayerRemoving(player)
	local success, errorMessage = pcall(function()
		PlayerPoints:SetAsync(player.UserId, player.leaderstats.Points.Value)
	end)
	if not success then
		warn(errorMessage)
	end
	local success, errorMessage = pcall(function()
		PlayerLevel:SetAsync(player.UserId, player.leaderstats.Level.Value)
	end)
	if not success then
		warn(errorMessage)
	end
end
game.Players.PlayerRemoving:Connect(onPlayerRemoving)

And, finally, here is a link to the game (under development) if you want to check it out:
https://www.roblox.com/games/7323952275/Jaxxon-Jargons-Robot-Cloning

P.S. I really appreciate all the great feedback I’ve gotten!

[Edited to incorporate the fix from the next post.]

#10

I figured out my off-by-one problem. I was updating the leaderstats before I created the clone when I need to do it after. The change looks like this:

			local foldername = clone.Name .. "s"
			clone.Parent = workspace[foldername]
			if player then
				updateLeaderstats(player)
			end

Now everything works the way I intended.