How would I improve the efficiency/readability of my code?

Stelrex · May 4, 2019, 8:31pm

Hello! A few days ago I created a quest system, where you walk up to a NPC and you can click if you’d like to interact. It’s all working as it should. But I’m looking for some help on how I could improve upon it. Right now it’s pretty limited and messy.

Things I’d like to improve:

The Efficiency of it
The Readablity
Ability to add more onto the dialog

Any help would be much appreciated!

Code

local Module = {}

local ReplicatedStorage = game:GetService("ReplicatedStorage")
local UserInputService = game:GetService("UserInputService")
local MouseButton1Pressed = UserInputService:IsMouseButtonPressed(Enum.UserInputType.MouseButton1)
local UIs = ReplicatedStorage:WaitForChild("UIs")
local Player = game.Players.LocalPlayer
local PlayerGui = Player.PlayerGui
local Distance = 12

local Quests = {
	Quest_Bridge_Dialog = {
		Greeting = "Uh-oh! It looks like this bridge has some missing boards..",
		Quest = "Can you find some boards to repair the bridge?",
		YesReply = "Great! Come back to me when you've found them!",
		NoReply = "Oh well. Maybe later?",
		FoundReply = "Good job! Now you can cross the bridge!",
		NotFoundReply = "Come back when you've found the boards."
	},
	
	Quest_NotBridge_Dialog = {
		Greeting = "Uh-oh! I'm Quest_Not_Bridge",
		Quest = "Can you find some boards to repair the bridge?",
		YesReply = "Great! Come back to me when you've found them!",
		NoReply = "Oh well. Maybe later?",
		FoundReply = "Good job! Now you can cross the bridge!",
		NotFoundReply = "Come back when you've found the boards."
	}
}

function Module.CheckQuest(Level)
	local IsInteracting = false
	local hasFoundItem = false
	
	local Values = {		
	}
	
	for _, Quest in pairs(game.Workspace[Level]:WaitForChild("Quests"):GetChildren()) do
		Values[Quest.Name .. "_IsActive"] = false
	end
	
	while wait() do
		local IsNearNPC = false
		if not IsInteracting then
			for _, Quest in pairs(game.Workspace[Level]:WaitForChild("Quests"):GetChildren()) do
				local Collider = game.Workspace[Level]:WaitForChild("Quests")[Quest.Name]:WaitForChild("NPC")
				if Player:DistanceFromCharacter(Collider.PrimaryPart.Position) <= Distance then 
					IsNearNPC = true
					if PlayerGui:FindFirstChild("QuestDialogUi") == nil then
						local DialogUiClone = UIs:WaitForChild("QuestDialogUi"):Clone()
						DialogUiClone.Parent = PlayerGui
						local Connection 
						Connection = UserInputService.InputBegan:Connect(function(Input) -- Listens for MouseButton1Click
							if Input.UserInputType == Enum.UserInputType.MouseButton1 and IsInteracting == false then
								Connection:Disconnect()
								Player.Character.Humanoid.WalkSpeed = 0
								Player.Character.Humanoid.JumpPower = 0 --Freezes the player
								IsInteracting = true	
								DialogUiClone:WaitForChild("InfoText").Visible = false
								DialogUiClone:WaitForChild("Dialog").Visible = true --Shows the Dialog
								
						 		if Values[Quest.Name .. "_IsActive"] == true then --Checks if Quest is Active, if so, Checks if Found
									if hasFoundItem then  
										for i = 1, string.len(Quests[Quest.Name .. "_Dialog"]["FoundReply"]) do	
    										DialogUiClone.Dialog:WaitForChild("DialogText").Text = DialogUiClone.Dialog:WaitForChild("DialogText").Text..string.sub(Quests[Quest.Name .. "_Dialog"]["FoundReply"],i,i)	
    										wait(.05)
										end									
									else
										for i = 1, string.len(Quests[Quest.Name .. "_Dialog"]["NotFoundReply"]) do	
    										DialogUiClone.Dialog:WaitForChild("DialogText").Text = DialogUiClone.Dialog:WaitForChild("DialogText").Text..string.sub(Quests[Quest.Name .. "_Dialog"]["NotFoundReply"],i,i)	
    										wait(.05)
										end									
									end
									wait(3)
									Player.Character.Humanoid.WalkSpeed = 16
									Player.Character.Humanoid.JumpPower = 50 --Unfreezes the player
									DialogUiClone:Destroy()
								elseif Values[Quest.Name .. "_IsActive"] == false then --Checks if Quest is InActive, if so, loads Greeting
									DialogUiClone.Dialog:WaitForChild("DialogText").Text = ""
									for i = 1, string.len(Quests[Quest.Name .. "_Dialog"]["Greeting"]) do	
    									DialogUiClone.Dialog:WaitForChild("DialogText").Text = DialogUiClone.Dialog:WaitForChild("DialogText").Text..string.sub(Quests[Quest.Name .. "_Dialog"]["Greeting"],i,i)	
    									wait(.05)
									end		
									DialogUiClone.Dialog:WaitForChild("Next").Visible = true
									local NextClick 
									NextClick = DialogUiClone.Dialog:WaitForChild("Next").MouseButton1Click:Connect(function()
										NextClick:Disconnect()
										DialogUiClone.Dialog:WaitForChild("Next").Visible = false
										DialogUiClone.Dialog:WaitForChild("DialogText").Text = ""
										for i = 1, string.len(Quests[Quest.Name .. "_Dialog"]["Quest"]) do	
    										DialogUiClone.Dialog:WaitForChild("DialogText").Text = DialogUiClone.Dialog:WaitForChild("DialogText").Text..string.sub(Quests[Quest.Name .. "_Dialog"]["Quest"],i,i)	
    										wait(.05)
										end
										DialogUiClone.Dialog:WaitForChild("Choices").Visible = true
										local YesClick
										YesClick = DialogUiClone.Dialog.Choices:WaitForChild("Yes").MouseButton1Click:Connect(function()
											YesClick:Disconnect()
											DialogUiClone.Dialog:WaitForChild("Choices").Visible = false
											Values[Quest.Name .. "_IsActive"] = true
											DialogUiClone.Dialog:WaitForChild("DialogText").Text = ""
											for i = 1, string.len(Quests[Quest.Name .. "_Dialog"]["YesReply"]) do	
    											DialogUiClone.Dialog:WaitForChild("DialogText").Text = DialogUiClone.Dialog:WaitForChild("DialogText").Text..string.sub(Quests[Quest.Name .. "_Dialog"]["YesReply"],i,i)	
    											wait(.05)
											end
											wait(3)
											DialogUiClone:Destroy()
											Player.Character.Humanoid.WalkSpeed = 16
											Player.Character.Humanoid.JumpPower = 50 --Unfreezes the player
											IsInteracting = false
										end)
										local NoClick
										NoClick = DialogUiClone.Dialog.Choices:WaitForChild("No").MouseButton1Click:Connect(function()
											NoClick:Disconnect()
											DialogUiClone.Dialog:WaitForChild("Choices").Visible = false
											DialogUiClone.Dialog:WaitForChild("DialogText").Text = ""
											for i = 1, string.len(Quests[Quest.Name .. "_Dialog"]["NoReply"]) do	
    											DialogUiClone.Dialog:WaitForChild("DialogText").Text = DialogUiClone.Dialog:WaitForChild("DialogText").Text..string.sub(Quests[Quest.Name .. "_Dialog"]["NoReply"],i,i)	
    											wait(.05)
											end
											wait(3)
											DialogUiClone:Destroy()
											Player.Character.Humanoid.WalkSpeed = 16
											Player.Character.Humanoid.JumpPower = 50 --Unfreezes the player
											IsInteracting = false
										end)	
									end)
								end
							end	
						end)
					end	
				elseif Player:DistanceFromCharacter(Collider.PrimaryPart.Position) > Distance and not IsNearNPC then -- Checks if UI is loaded and if the distance is too far, if so, removes UI
					if PlayerGui:FindFirstChild("QuestDialogUi") ~= nil then
						PlayerGui.QuestDialogUi:Destroy()
					end
				end	
			end
		end
	end
end

return Module

Place File

QuestSystem.rbxl (34.1 KB)

The ModuleScript is in PlayerScripts,
A UI is in ReplicatedStorage,
Everything else is in Workspace

Operatik · May 4, 2019, 10:11pm

Tips & Tricks

There’s some code you can rewrite:
game:GetService("Players") is in my preferences, usually for a sure-shot that it is the service. – probably a habit of mine.
game.Workspace can be turned into workspace – it’s a global.

Sometimes, I use local function foo() end for some functionalities that repeats in the blocks of code and then add them to the main function. Passing some arguments to change it might even make the readability improved.

Other things seems to be in place. Nothing wrong here. All good to go.

Stelrex · May 4, 2019, 11:02pm

Do you think it’d be possible to loop some stuff or something of the sort to take down all the dialog interaction?

Happy DevForum Birthday!!

Operatik · May 4, 2019, 11:45pm

Not from what I currently know. Perhaps you can try something with that. Save an original copy and work with the cloned script. Make sure to disable one to test.

ExtremeBuilder15 · May 4, 2019, 11:59pm

It seems to me like you need to find ways to break up elements of your giant CheckQuest function into smaller functions. Right now, it’s hard to tell what’s going on because it’s a huge block of if statements and for loops.

For example, where you create a UIS Connection, you could have all of that logic in a separate function somewhere else in your code, and simply connect the UIS event to that function.

rachjumper · May 5, 2019, 12:02am

I’d break up some of the bigger chunks but other then that my code looks pretty similar
Think of it like separating a wall of text into paragraphs

ForbiddenJ · May 13, 2019, 6:02am

Yeah. There are plenty of things that can be improved with that code. Here is a non-exhaustive list of things to think about:

Variables

You should space apart the variables based on categories. For example: Services, constants, instance references, variable values, etc.
Consider renaming Module to mod? It’s shorter.

Module.CheckQuest function

(Biggest readability issue contributor) There is a massive click event function inside the CheckQuest function. It takes more than half the space of the function it is in! It would be better if the function was carefully pulled out and named a separate script-level function, and the function passed in to the event by name. Especially considering the CheckQuest function runs only once. You’ll need to make some variables script-scope.
The function runs only once. Consider adding a boolean value and an if-statement to block additional runs of the function, so you still have only one loop.
The function is named weirdly for a function that only runs once and constantly runs a loop. I’d think it means checking the status of a quest. Consider changing it to something like “RunQuestBehaviorLoop”.
The function is dependent on the game’s data model not doing odd behaviors. For example, relying on the presence of the QuestDialogUi to determine whether to (re)connect the mouse input event. I suggest storing the information of the quest interaction state in variables instead.
You do not need to use WaitForChild on anything from this function’s DialogUiClone object. Clones made on clients stay as-is after cloning is finished. Nothing more replicates.

and/or Lua construct simplifies code

This block can be compacted a lot:

if hasFoundItem then  
	for i = 1, string.len(Quests[Quest.Name .. "_Dialog"]["FoundReply"]) do	
		DialogUiClone.Dialog:WaitForChild("DialogText").Text = DialogUiClone.Dialog:WaitForChild("DialogText").Text..string.sub(Quests[Quest.Name .. "_Dialog"]["FoundReply"],i,i)	
		wait(.05)
	end									
else
	for i = 1, string.len(Quests[Quest.Name .. "_Dialog"]["NotFoundReply"]) do	
		DialogUiClone.Dialog:WaitForChild("DialogText").Text = DialogUiClone.Dialog:WaitForChild("DialogText").Text..string.sub(Quests[Quest.Name .. "_Dialog"]["NotFoundReply"],i,i)	
		wait(.05)
	end									
end

With a special and/or Lua construct, you can do this instead:

local replyIndex = hasFoundItem and "FoundReply" or "NotFoundReply"
for i = 1, string.len(Quests[Quest.Name .. "_Dialog"][replyIndex]) do	
	DialogUiClone.Dialog.DialogText.Text = DialogUiClone.Dialog.DialogText.Text .. string.sub(Quests[Quest.Name .. "_Dialog"][replyIndex],i,i)	
	wait(.05)
end

Additional Notes

These are not all the things that can be changed, but it’s a fair bit.

Stelrex · May 13, 2019, 11:52am

Thank you so much for your reply.
I’ll work on adjusting some things later today.
I do have a question though, is there any benefit to naming things “mod” instead of “Module” other than it is shorter? I’ve tried to do that, but when skimming through code, it’s harder for me to understand what’s going on if everything abbreviated.

ForbiddenJ · May 13, 2019, 5:38pm

The main benefit is that the term is a lot easier to type over and over. I hate having to type entire words to refer to variables that are used more than a dozen times in the same script. That’s why I prefer short variable names.

Also, if you do your naming schemes consistently throughout your game, like naming modules’ tables “mod”, or use the single letter “r” for variables that are to be returned at the end of functions. You’re more likely to remember what they mean.