Suggestions on how to improve: Quest System

Help and Feedback Code Review
#1

Hey there. I’m Drinkinix. I’ve been making a Quest System for a few days, and have wondered if there is any way to improve it. Below is a part of my script.

-- Quest System

function UpdateKillValue()
	script.Parent.QuestUI.FreddyQuest.TextLabel.Text = "Freddy's Quest: ".. 
        script.Parent.QuestUI.FreddyQuest.Value.Value.. "/10 People Killed"
end

function IfKilled10()
	if script.Parent.QuestUI.FreddyQuest.Value.Value == 10 then
	script.Parent.QuestUI.FreddyQuest:TweenPosition(UDim2.new(0.5,0,0.13,0,'Out','Sine',1))
	game.Players.LocalPlayer:WaitForChild("leaderstats").Diamonds.Value = 
        game.Players.LocalPlayer:WaitForChild("leaderstats").Diamonds.Value + 20
	wait(1)
	script.Parent.QuestCompleted:TweenPosition(UDim2.new(-0.045,0,-0.045,0,'Out','Sine',3))
	end
end

script.Parent.QuestUI.FreddyQuest.Value:GetPropertyChangedSignal('Value'):Connect(function()
	UpdateKillValue()
	IfKilled10()
end)

This script basically sees whether or not a value in the GUI has changed. If it has, then it runs the two functions that I have written. Descriptive feedback would be appreciated!

  • Drinkinix
6 Likes
#2

First off, I would use variables more liberally. A good rule of thumb is that if you’re calling the same thing more than once, stick it in a variable.

For example, you’re using script.Parent.QuestUI.FreddyQuest and game.Players.LocalPlayer a lot. Instead, you should attach these and other values to variables and use those. Implementing this, and cleaning up some indentation, would make your code look something like this:

-- Quest System
local freddyQuest = script.Parent.QuestUI.FreddyQuest
local value = freddyQuest.Value
local player = game.Players.LocalPlayer

function UpdateKillValue()
	freddyQuest.TextLabel.Text = "Freddy's Quest: ".. 
        value.Value.. "/10 People Killed"
end

function IfKilled10()
	if value.Value == 10 then
	    freddyQuest:TweenPosition(UDim2.new(0.5,0,0.13,0,'Out','Sine',1))
	    player:WaitForChild("leaderstats").Diamonds.Value = 
            player:WaitForChild("leaderstats").Diamonds.Value + 20
	    wait(1)
	    script.Parent.QuestCompleted:TweenPosition(UDim2.new(-0.045,0,-0.045,0,'Out','Sine',3))
	end
end

value:GetPropertyChangedSignal('Value'):Connect(function()
	UpdateKillValue()
	IfKilled10()
end)

This is already looking a lot neater I would say. Next up, I’d like to quickly address your usage of :WaitForChild(). What this method does is waits until an instance exists and then returns it. This means that you shouldn’t be using it twice on the same instance, unless there is a chance that the instance will have been deleted since your last check.
For example, in your code there is

player:WaitForChild("leaderstats").Diamonds.Value = 
    player:WaitForChild("leaderstats").Diamonds.Value + 20

What we would rather do is:

local leaderstats = player:WaitForChild("leaderstats")
leaderstats.Diamonds.Value = leaderstats.Diamonds.Value + 20

We could even wrap leaderstats.Diamonds into its own variable, since that is all we are changing in this function, and move the :WaitForChild() to the top of the script, leaving your code looking like this:

-- Quest System
local freddyQuest = script.Parent.QuestUI.FreddyQuest
local value = freddyQuest.Value

local player = game.Players.LocalPlayer
local leaderstats = player:WaitForChild("leaderstats")

function UpdateKillValue()
	freddyQuest.TextLabel.Text = "Freddy's Quest: ".. 
        value.Value.. "/10 People Killed"
end

function IfKilled10()
	if value.Value == 10 then
	    freddyQuest:TweenPosition(UDim2.new(0.5,0,0.13,0,'Out','Sine',1))
            local diamonds = leaderstats.Diamonds
            diamonds.Value = diamonds.Value + 20
	    wait(1)
	    script.Parent.QuestCompleted:TweenPosition(UDim2.new(-0.045,0,-0.045,0,'Out','Sine',3))
	end
end

value:GetPropertyChangedSignal('Value'):Connect(function()
	UpdateKillValue()
	IfKilled10()
end)

Next we’re going to go into the algorithm your code is running, but first I want to clear up the value variable. Your Value instance is just named “Value”, making it difficult to intuit what it actually contains. From context, I’m going to assume it is the number of enemies killed so far, and rename it. In addition I’m going to change game.Players to game:GetService("Players"), as this is the convention for top level singleton services:

-- Quest System
local freddyQuest = script.Parent.QuestUI.FreddyQuest
local value = freddyQuest.enemiesKilled

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

function UpdateKillValue()
	freddyQuest.TextLabel.Text = "Freddy's Quest: ".. 
        enemiesKilled.Value.. "/10 People Killed"
end

function IfKilled10()
	if enemiesKilled.Value == 10 then
	    freddyQuest:TweenPosition(UDim2.new(0.5,0,0.13,0,'Out','Sine',1))
            local diamonds = leaderstats.Diamonds
            diamonds.Value = diamonds.Value + 20
	    wait(1)
	    script.Parent.QuestCompleted:TweenPosition(UDim2.new(-0.045,0,-0.045,0,'Out','Sine',3))
	end
end

enemiesKilled:GetPropertyChangedSignal('EnemiesKilledValue'):Connect(function()
	UpdateKillValue()
	IfKilled10()
end)

Next up, the organisation of your algorithm is a bit peculiar. It works, but it’s a bad workflow to put your conditionals into the function - it would be better to replace IfKilled10() with a function that simply completes the quest, and adds 20 diamonds to the players leaderstats pool. The if statement would then be in the :GetPropertyChangedSignal() function:

-- Quest System
local freddyQuest = script.Parent.QuestUI.FreddyQuest
local enemiesKilled = freddyQuest.enemiesKilled

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

function updateKillValue()  -- I also changed this to camelCase
	freddyQuest.TextLabel.Text = "Freddy's Quest: ".. 
        enemiesKilled.Value.. "/10 People Killed"
end

function giveReward()
	freddyQuest:TweenPosition(UDim2.new(0.5,0,0.13,0,'Out','Sine',1))
        local diamonds = leaderstats.Diamonds
        diamonds.Value = diamonds.Value + 20
	wait(1)
	script.Parent.QuestCompleted:TweenPosition(UDim2.new(-0.045,0,-0.045,0,'Out','Sine',3))
end

enemiesKilled:GetPropertyChangedSignal('Value'):Connect(function()
    updateKillValue()
    if enemiesKilled.Value == 10 then
        giveReward()
    end
end)

This is the most I can give you with the knowledge I can glean about your game - I hope it’s helpful!

2 Likes
#3

There are a few problems with the corrections

local value = freddyQuest.enemiesKilled

should be

local enemiesKilled = freddyQuest.enemiesKilled

Since it is used at the end of the code.
Speaking of which:

enemiesKilled:GetPropertyChangedSignal('EnemiesKilledValue'):Connect(function()
    updateKillValue()
    if enemiesKilled.Value == 10 then
        giveReward()
    end
end)

Should be

enemiesKilled:GetPropertyChangedSignal("Value"):Connect(function()
    updateKillValue()
    if enemiesKilled.Value == 10 then
        giveReward()
    end
end)

Additionally, this could be coded differently with the Changed event since, for ValueObjects, their Changed events are overridden to detect whenever the Value’s value property has been changed.

enemiesKilled.Changed:Connect(function() 
    updateKillValue()
    if enemiesKilled.Value == 10 then
        giveReward()
    end
end)
2 Likes
#4

Woops, I must have gotten the names mixed up there, thanks for spotting it.

Concerning the event, in this case both would work however it’s good practice to use :GetPropertyChangedSignal(), which is why I did not suggest your change.