Would this work?

Help and Feedback Code Review
#1

I’m making an emote reward system that rewards a player a new emote every other minute. I have 3 scripts, 1 for saving and rewarding the emotes, and the other 2 are for playing the emote and the emote list itself.

SCRIPT 1 (Script, ServerScriptStorage):

local DataStore = game:GetService("DataStoreService")
local playerData = DataStore:GetDataStore("PlayerData")

local RepliStorage = game:GetService("ReplicatedStorage")
local emoteEvent = RepliStorage.sendEmote
local emotesFolder = RepliStorage.Animations:GetChildren()

function onPlayerJoin(player)
	local playerUserId = player.UserId
	local data = playerData:GetAsync(playerUserId)
	
	local folderEmote = Instance.new("Folder")
	folderEmote.Name = "Emotes"
	folderEmote.Parent = player
	
	local Data = playerData:GetAsync(player.UserId) or {}
	
	for i, emote in pairs(Data[1]) do
		if emotesFolder:FindFirstChild(emote) then
			emotesFolder[emote]:Clone().Parent = folderEmote
		end
	end

	local playerGui = player:WaitForChild("PlayerGui")
	local countdownVal = playerGui.Emotes.Holder.countDown

	if countdownVal.Value == 0 then
		emoteEvent:FireClient(player)
	end
end

function onPlayerLeave(player)
	local succ, err = pcall(function()
		local ownedEmotes = player.Emotes:GetChildren()
		local emoteNames = {}

		for i, emote in pairs(ownedEmotes) do
			table.insert(emoteNames, emote.Name)
		end

		local savedData = emoteNames
		playerData:SetAsync(player.UserId, savedData)
	end)
	if not succ then
		warn("Could not save. Retrying...")
	end
end

emoteEvent.OnServerEvent:Connect(function(player, emoteN)
	local attempts = 5
	
	if emoteN and emotesFolder:FindFirstChild(emoteN) and attempts > 0 and not player.Emotes:FindFirstChild(emoteN) then
		local pEmotes = player.Emotes
		emotesFolder[emoteN]:Clone().Parent = pEmotes
		
		attempts = 5
	elseif attempts == 0 then
		print("Retrying in 1 minute.")
	end
	
	repeat
		attempts -= 1
		warn("Retrying the process...")
	until not player.Emotes:FindFirstChild(emoteN) or attempts == 0
	attempts = 5
end)

game.Players.PlayerAdded:Connect(onPlayerJoin)
game.Players.PlayerRemoving:Connect(onPlayerLeave)

game:BindToClose(function()
	for i, plr in pairs(game.Players:GetChildren()) do
		onPlayerLeave(plr)
	end
end)

SCRIPT 2 & 3 (Both LocalScript, Inside Gui):

local player = game.Players.LocalPlayer
local char = player:WaitForChild("Character")
local hum = char:WaitForChild("Humanoid")
local animationId = script.Parent.AnimationValue

local holder = script.Parent.Holder
local emoteList = holder.EmoteList

local animateEmote = {}

local function playEmote(animID)
	local animation = hum:WaitForChild("Animator"):LoadAnimation(animID)
	animation.Priority = Enum.AnimationPriority.Action4
	animation.Looped = true
	
	if animation.IsPlaying then
		animation:Stop()
	else
		for i, playingEmote in pairs(animateEmote) do
			playingEmote:Stop()
			table.remove(animateEmote, i)
		end
		animation:Play()
		table.insert(animateEmote, animation)
	end
	char.HumanoidRootPart.Anchored = true
end

for i, v in pairs(emoteList:GetChildren()) do
	if v:IsA("Frame") then
		v.Interact.MouseClick:Connect(function()
			animationId.Value = v.AnimationID.Value
			task.wait(0.1)
			playEmote("rbxassetid://"..animationId.Value)
		end)
	end
end

--DEFINING
local repSto = game:GetService("ReplicatedStorage")
local tweenServ = game:GetService("TweenService")
local folderEmote = game.Players.LocalPlayer:WaitForChild("Emotes")
local emoteFolder = repSto.Animations
local sendEmote = repSto.sendEmote

local emote = script.Emote
local holder = script.Parent.Holder
local emoteList = holder.EmoteList
local listNum = emoteList:GetChildren()
local emoteText = holder.TextLabel

--COUNTDOWN
local internalClockMin = 60
local countDown = 120
local countValue = holder.countDown

while task.wait(1) do
	countDown -= 1
	clockMin = countDown/internalClockMin
	countValue.Value = clockMin
	emoteText.Text = "next emote in "..string.format("%.2f", clockMin).." minutes..."
	if countDown <= 0 then countDown = 60 end
end

--EMOTES LIST
function dupeEmote()
	for i, v in pairs(listNum) do
		local count = #listNum
		if count > 4 then
			if v.EmoteNumber.Value == 5 then
				v:Destroy()
			end
		end
	end
	
	local emoteB = {}
	
	for i, emote in pairs(folderEmote:GetChildren()) do

		local btn = script.Emote:Clone()
		local emoteN = emote.Name
		btn.EmoteName.Text = emoteN

		table.insert(emoteB, btn)
	end

	table.sort(emoteB, function(a, b)
		return a.EmoteName.Text < b.EmoteName.Text
	end)

	for i, btn in pairs(emoteB) do
		btn.Parent = emoteList
	end
end

dupeEmote()
folderEmote.ChildAdded:Connect(dupeEmote)
folderEmote.ChildRemoved:Connect(dupeEmote)

Would this actually function as a good & working way of what I’m trying to accomplish? Any feedback and recommendations is GREATLY appreciated :slight_smile:

Looping this Check
#2

This is a valid way to format in a Lua-friendly way.

It might be helpful to modulate both client and server DataStoreService management for easy reference, and consistency. In script 2 you defined char.HumanoidRootPart, but this could technically return nil. Instead, you can use char.PrimaryPart or hum.RootPart to directly get the HumanoidRootPart.

Do your MouseClick interactions just happen once? Maybe it might be best to use :Once() instead.

I suggest removing unused variables such as the index values in most of the in pair loops just for cleanliness.

All in all, this looks like the correct way to manage an emote menu. Any further optimization may oversaturate it - the biggest change would be creating the aforementioned DataStoreService module. By modulating your DataStores it gives you a single source to funnel requests linearly to update player data, avoiding race conditions.

Roblox has a plentiful amount of code samples which you can use in replacement of your current data method. You can find their version of player data management here.

2 Likes