How can I improve this dialogue code?

Help and Feedback Code Review
1

The way this code works is that it will use proximity prompts, and values to create a dialogue system. I am mainly looking for how I can improve it. It is sloppy and I worry that the multiple connections could cause memory leaks. I am also not sure how to tackle making the dialogue changes depending on certain conditions (for example, if you talk to them the first time you are able to buy a item, if you bough the item, the character says different things).

for _,v in ipairs(DialoguePrompts:GetChildren()) do
	print(v)
	if v:FindFirstChild("ProximityPrompt") then
		print("is a prompt")
		v:WaitForChild("ProximityPrompt").Triggered:Connect(function(Plr)
			print("Spoke")
			if Speaking == false then
				DialogueFrame.Visible = true
				Speaking = true

				ContextActionService:BindAction(
					FREEZE_ACTION,
					function() return Enum.ContextActionResult.Sink end,
					false,
					unpack(Enum.PlayerActions:GetEnumItems())
				)

				local DialogueString = v:WaitForChild("NPC")
				local Options = DialogueString:GetChildren()


				local ExitButton = ScrollerFrame.EX:Clone()
				ExitButton.Parent = ScrollerFrame
				ExitButton.Name = "Exit"
				ExitButton.Visible = true
				ExitButton.Text = "[Leave Dialogue]"
				ExitButton.LayoutOrder = 999
				Exited = ExitButton.Activated:Connect(function()
					Options = nil
					Responded = true
					ExitButton:Destroy()
					Exited:Disconnect()
				end)

				DialogueName.Text = v:FindFirstChild("ProximityPrompt").ObjectText
				repeat
					Responded = false
					--print(Options)
					task.spawn(function()
						AnimateUI.typeWrite(DialogueTextBox,DialogueString.Value,0.01)
					end)
					--DialogueTextBox.Text = DialogueString.Value
					for index,v in ipairs(Options) do
						local Responce = ScrollerFrame.EX:Clone()
						Responce.LayoutOrder = index
						Responce.Name = "Responce".. index
						Responce.Text = v.Value
						Responce.Parent = ScrollerFrame
						Responce.Visible = true
						Respondedf = Responce.Activated:Connect(function()
							DialogueString = v:FindFirstChild("NPC")
							Options = DialogueString:GetChildren()
							
							if v.Value == "[Sell]" then
								print("Sell")									
							elseif v.Value == "[Purchase]" then
								print("Purchase!")
								if v:FindFirstChild("PurchasedItem") then
									local Purchased = v:FindFirstChild("PurchasedItem").Value
									local Succeeded = PurchaseRemote:InvokeServer(Purchased)
									if Succeeded then
										DialogueString = v:FindFirstChild("NPC")
									else
										DialogueString = v:FindFirstChild("Failure")
									end
								end
							end
							Responded = true
							for i,v in ipairs(ScrollerFrame:GetChildren()) do
								if string.find(v.Name,"Responce") then
									print(v)
									v:Destroy()
								end
							end

							Respondedf:Disconnect()
						end)
					end

					RunService.RenderStepped:Wait()
					repeat RunService.Stepped:Wait() until Responded == true
				until Options == nil
				Speaking = false
				ContextActionService:UnbindAction(FREEZE_ACTION)
				for i,v in ipairs(ScrollerFrame:GetChildren()) do
					if v.Name ~= "EX" and not v:IsA("UIListLayout") then
						print(v)
						v:Destroy()
					end
				end
				DialogueFrame.Visible = false
			end			
		end)
	end
end
2 Likes
2

There are only two connections that would leak, and that’s the v:WaitForChild("ProximityPrompt").Triggered and when you bind FREEZE_ACTION. The first is because it never gets disconnected (it is OK as long the connection is made once). The action you bind, I’m not sure when it gets unbound and I suspect it can be bound twice in a row (which is an error).

Concerning handling state, simply just having a variable that keeps track if you talked to them would be OK. You’d place this in the for-loop. You could also have a function to query if a specific item is bought and use it in the dialogue.

To make the code a little simpler, you can change the if Speaking == false into a guard statement.

if Speaking then
    return
end
--...

It’s possible because you’re only doing anything when speaking is false.

I would also run the code through a formatter first to make it visually clearer what’s happening.

Also when you’re setting up the DialogueTextBox, you yield twice at the end.

RunService.RenderStepped:Wait() -- Get rid of this one.
repeat RunService.Stepped:Wait() until Responded == true

Using clearer variable names instead of just v would help.

Those are the tips I can think of right now. Hopefully, they help a little in cleaning up your code.

3 Likes