This code feels clunky, and hard to read. How can it improve?

Help and Feedback Code Review
#1

Hello! In January, I created this code that gives players XP, and points depending on how much time they got.

					if not won then
						won = true
						local trialTime = game:GetService("ReplicatedStorage"):WaitForChild("TimeTrials")
						if trialTime.Value < plr.TimeTrials[name].Value or plr.TimeTrials[name].Value == 0 then
							game:GetService("ReplicatedStorage").Music:FireClient(plr, "Win")
							plr.TimeTrials:FindFirstChild(name).Value = trialTime.Value
							plr.PlayerGui.EndScreen.Trials.Improve.Text = "Your improved your time!"
						else
							game:GetService("ReplicatedStorage").Music:FireClient(plr, "Lose")
							plr.PlayerGui.EndScreen.Trials.Improve.Text = "You did not improve your time. "
						end
						plr.PlayerGui.EndScreen.Trials.NewTime.Text = "Your time was ".. formatTime(trialTime.Value).. ". "
						if trialTime.Value < game.ServerStorage.Rewards[name].Gold:GetAttribute("Time") then
							plr:WaitForChild("leaderstats"):WaitForChild("Points").Value += game.ServerStorage.Rewards[name].Gold:GetAttribute("Points")
							plr:WaitForChild("LevelsSystem"):WaitForChild("Current").Value += game.ServerStorage.Rewards[name].Gold:GetAttribute("XP")
							plr.PlayerGui.EndScreen.Trials.Tier.Text = "You won Gold Tier!"
							plr.PlayerGui.EndScreen.Trials.Tier.TextColor3 = Color3.fromRGB(255, 196, 57)
							game:GetService("ReplicatedStorage").SendPoints:FireClient(plr, game.ServerStorage.Rewards[name].Gold:GetAttribute("Points"))
							game:GetService("ReplicatedStorage").SendXP:FireClient(plr, game.ServerStorage.Rewards[name].Gold:GetAttribute("XP"))
							badgeser:AwardBadge(plr.UserId, 2124624904)
							if mps:UserOwnsGamePassAsync(plr.UserId, 12460579) then
								plr.leaderstats.Points += game.ServerStorage.Rewards[name].Gold:GetAttribute("Points")
							end
						elseif trialTime.Value > game.ServerStorage.Rewards[name].Bronze:GetAttribute("Time") then
							plr:WaitForChild("leaderstats"):WaitForChild("Points").Value += game.ServerStorage.Rewards[name].Bronze:GetAttribute("Points")
							plr:WaitForChild("LevelsSystem"):WaitForChild("Current").Value += game.ServerStorage.Rewards[name].Bronze:GetAttribute("XP")
							plr.PlayerGui.EndScreen.Trials.Tier.Text = "You won Bronze Tier!"
							plr.PlayerGui.EndScreen.Trials.Tier.TextColor3 = Color3.fromRGB(95, 73, 21)
							game:GetService("ReplicatedStorage").SendPoints:FireClient(plr, game.ServerStorage.Rewards[name].Bronze:GetAttribute("Points"))
							game:GetService("ReplicatedStorage").SendXP:FireClient(plr, game.ServerStorage.Rewards[name].Bronze:GetAttribute("XP"))
							badgeser:AwardBadge(plr.UserId, 2124624907)
							if mps:UserOwnsGamePassAsync(plr.UserId, 12460579) then
								plr.leaderstats.Points += game.ServerStorage.Rewards[name].Bronze:GetAttribute("Points")
							end
						else
							plr:WaitForChild("leaderstats"):WaitForChild("Points").Value += game.ServerStorage.Rewards[name].Silver:GetAttribute("Points")
							plr:WaitForChild("LevelsSystem"):WaitForChild("Current").Value += game.ServerStorage.Rewards[name].Silver:GetAttribute("XP")
							plr.PlayerGui.EndScreen.Trials.Tier.Text = "You won Silver Tier!"
							plr.PlayerGui.EndScreen.Trials.Tier.TextColor3 = Color3.fromRGB(112, 112, 112)
							game:GetService("ReplicatedStorage").SendPoints:FireClient(plr, game.ServerStorage.Rewards[name].Silver:GetAttribute("Points"))
							game:GetService("ReplicatedStorage").SendXP:FireClient(plr, game.ServerStorage.Rewards[name].Silver:GetAttribute("XP"))
							badgeser:AwardBadge(plr.UserId, 2124624905)
							if mps:UserOwnsGamePassAsync(plr.UserId, 12460579) then
								plr.leaderstats.Points += game.ServerStorage.Rewards[name].Silver:GetAttribute("Points")
							end
						end						
					end

However, this code looks clunky, hard to read, and may be unreliable. If you can help improve it, please let me know. Thanks! WE

#2

try using variables like

local rs = game:GetService("ReplicatedStorage")

this will shrink your code

EDIT:variables are used to store data

4 Likes
#3

Thanks for that, but it is still pretty hard to read. All that really did was make a line shorter.

#4

Define functions outside this mess so it looks a lot cleaner and easier to read.
Example

function timeTrialCheck()
-- might be a better name idk
    if trialTime.Value < plr.TimeTrials[name].Value or plr.TimeTrials[name].Value == 0 then
            game:GetService("ReplicatedStorage").Music:FireClient(plr, "Win")
		    plr.TimeTrials:FindFirstChild(name).Value = trialTime.Value
		    plr.PlayerGui.EndScreen.Trials.Improve.Text = "Your improved your time!"
    else
		    game:GetService("ReplicatedStorage").Music:FireClient(plr, "Lose")
		    plr.PlayerGui.EndScreen.Trials.Improve.Text = "You did not improve your time. "
    end
end
1 Like
#5

Do write variables but please write out the actual service name

This is a mistake I see many new scripters do. Shrinking down these variables hurts readability in exchange for slightly less space taken up but
As you upscale your code it will get more and more cryptic.

When writing variables for services I suggest writing out the service names in full.

2 Likes
#6

A good rule of thumb is to create variables for everything you’re indexing multiple times. You should create variables for practically the whole script, as well as defining the Services.

For me, readability is best when using PascalCase. Also don’t shrink your code like turning Player into plr, this makes it harder for anyone (and future you) to read your code.

1 Like
#7

to add on to @Creeperman16487 it’s important to make your variable names specific for readability purposes:

local ReplicatedStorage = game:GetService("ReplicatedStorage")
1 Like
#8

Try organizing your script, for example:

--// Services

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

--// Variables

local winMessage = "you won!"
local loseMessage = "you lost!"

--// Requires

local messageTyper = require(game.ReplicatedStorage.MessageTyper)

--// Functions

local function hi()
      print("hello!")
end

--// Main Code

hi()

if foo then
      print("bar")
end

This is just an example.

#9

It seems as a lot of what you’re doing here can be better achieved using tables. It seems like a bunch of code that does the same but for different stuff. You can keep a table (array) of a bunch of tables which keep functions which can be a condition, I can send you the code I used for Name tags when I reach my PC and you can get an idea.

1 Like
#10

The script you typed looks neat, I suggest sticking to PascalCase for all variables, not just the services at the top of the script.

Also section markers, like --// Functions look bad and are over stylized that make the code harder to read.

https://roblox.github.io/lua-style-guide/#comments

#11

declare variables that you will use multiple times in different places at the top of your code;
for instance, you consistently used ReplicatedStorage and ServerStorage services throughout your code, a good idea might be to declare these at the top like this: local ReplicatedStorage = game:GetService("ReplicatedStorage"); so instead of your code looking like this: game:GetService("ReplicatedStorage").SendPoints:FireClient, it would look like this: ReplicatedStorage.SendPoints:FireClients(); that looks much better doesn’t it?

now for a script like this that encompasses many different things, its always important to make variable declarations of prominent references./

for instance: if you had a script that updates a GUI or something, its fine to use script.Parent.etc.Property = value, because the sole purpose of that script is to do that; but for a script like this which encompasses many elements, its better to make variable declarations - making sure to keep “similar” variables together: for instance:

a = scrip.Parent.A
a2 = a.A2

not

a = scrip.Parent.A
a2 = script.Parent.A.A2;

as you can see the second example looks pretty ugly in comparison to the first;

another thing is to indent and space your code;
as for indentation, you actually did pretty well!
however for spacing your code in a readable manner, you have a bit go.

its better to do:

    local Players = game:GetService(“Players”);--notice variable

    function ExplodeAll(message)
     if(message == ":Explode All") then
      local players = Players:GetPlayers();--notice: variable declaration

      for _, player in pairs(players) do
        local character = player.Character--variable declaration
        --notice this space
        local explosion = Instance.new("Explosion", character.HumanoidRootPart)
      end

      return
     end
                            --notice this space
     print("invalid command")
    end)

The code above is much easier to read that this below:

    function ExplodeAll(message)
     if(message == ":Explode All") then
      for _, player in pairs(game:GetService("Players"):GetPlayers()) do
        local explosion = Instance.new("Explosion", player.Character.HumanoidRootPart)
      end
      return
     end
     print("invalid command")
    end)

it might not look so bad since this is a small piece of code, however as we’ve seen from your post, it can become traumatizing pretty quickly