How to write clean code

Resources Community Tutorials
#1

Hello, today i’ll share you my experience with writing redable code, you will learn few techniques of making your code more redable, also we will talk about positives & negatives of writing clean code

1. Positives & Negatives

Soo, first of all why we want to write redable code? See most of the time you will work with others, and those others must understand your code to make successful game, also you might make breaks from scripting and forget about what specific script does

Pros:

  • Others can work with you
  • You can take breaks and understand your code
  • Fixing bugs is 100x easier
  • You can analyze code and optimize it without problem

Cons:

  • On client cheaters might understand your code well and make exploits faster

Still don’t worry about cheaters, you can annoy them in different ways than making their struggle to read your scripts :}

2. Example

Soo now the time for example, here we have script for giving player coins every time he touches part

local sp = script.Parent
local db = {}

sp.Touched:Connect(function(hit)
if hit.Parent:FindFirstChild("Humanoid") then
if game.Players:GetPlayerFromCharacter(hit.Parent) then
if not db[game.Players:GetPlayerFromCharacter(hit.Parent)] then
db[game.Players:GetPlayerFromCharacter(hit.Parent)] = 0
elseif tick() - db[game.Players:GetPlayerFromCharacter(hit.Parent)] >= 5 then
game.Players:GetPlayerFromCharacter(hit.Parent).Gold.Value = game.Players:GetPlayerFromCharacter(hit.Parent).Gold.Value + 5
end
end
end
end)

Script above is hard-coded and unredable and it’s only changing one value with debounce!!! imagine writing 10x longer script, it would be almost impossible!

To fix this, we can simply format our code, you can use formatting tool or use Tab to move code in every scope one step forward, our end result looks like this:

local sp = script.Parent
local db = {}

sp.Touched:Connect(function(hit)
	if hit.Parent:FindFirstChild("Humanoid") then
		if game.Players:GetPlayerFromCharacter(hit.Parent) then
			if not db[game.Players:GetPlayerFromCharacter(hit.Parent)] then
				db[game.Players:GetPlayerFromCharacter(hit.Parent)] = 0
			elseif tick() - db[game.Players:GetPlayerFromCharacter(hit.Parent)] >= 5 then
				game.Players:GetPlayerFromCharacter(hit.Parent).Gold.Value = game.Players:GetPlayerFromCharacter(hit.Parent).Gold.Value + 5
			end
		end
	end
end)

As you can see, this code is at least redable, but imagine coming back from long break and seing it, it might take few minutes, soo to fix this we need to change it a little bit more

First of those changes would be soft-coding, we can’t change anything like cooldown or gold value, soo we will create settings for that, thanks to them we don’t need to read through main script to adjust it

local sp = script.Parent
local db = {}

local cd = 5
local gv = 10

sp.Touched:Connect(function(hit)
	if hit.Parent:FindFirstChild("Humanoid") then
		if game.Players:GetPlayerFromCharacter(hit.Parent) then
			if not db[game.Players:GetPlayerFromCharacter(hit.Parent)] then
				db[game.Players:GetPlayerFromCharacter(hit.Parent)] = 0
			elseif tick() - db[game.Players:GetPlayerFromCharacter(hit.Parent)] >= cd then
				game.Players:GetPlayerFromCharacter(hit.Parent).Gold.Value = game.Players:GetPlayerFromCharacter(hit.Parent).Gold.Value + gv
			end
		end
	end
end)

But still, those settings might be not that redable, we can use types to fix that

local variable: type = value

Soo we applied types to our settings, and we have:

local sp = script.Parent
local db = {}

local cd: number = 5
local gv: number = 10

sp.Touched:Connect(function(hit)
	if hit.Parent:FindFirstChild("Humanoid") then
		if game.Players:GetPlayerFromCharacter(hit.Parent) then
			if not db[game.Players:GetPlayerFromCharacter(hit.Parent)] then
				db[game.Players:GetPlayerFromCharacter(hit.Parent)] = 0
			elseif tick() - db[game.Players:GetPlayerFromCharacter(hit.Parent)] >= cd then
				game.Players:GetPlayerFromCharacter(hit.Parent).Gold.Value = game.Players:GetPlayerFromCharacter(hit.Parent).Gold.Value + gv
			end
		end
	end
end)

But wait, isn’t something missing? we know what kind of value we should write into settings, but can we really understand them?

Most beginners make one crucial mistake, they use shortcurts, see shortcurts are bad idea because we might not understand them, soo instead of writing two letters we should name our variables with clear names

local part = script.Parent
local debounce = {}

local cooldown: number = 5
local goldvalue: number = 10

part.Touched:Connect(function(hit)
	if hit.Parent:FindFirstChild("Humanoid") then
		if game.Players:GetPlayerFromCharacter(hit.Parent) then
			if not debounce[game.Players:GetPlayerFromCharacter(hit.Parent)] then
				debounce[game.Players:GetPlayerFromCharacter(hit.Parent)] = 0
			elseif tick() - debounce[game.Players:GetPlayerFromCharacter(hit.Parent)] >= cooldown then
				game.Players:GetPlayerFromCharacter(hit.Parent).Gold.Value = game.Players:GetPlayerFromCharacter(hit.Parent).Gold.Value + goldvalue
			end
		end
	end
end)

Now we have pretty much understandable script, but there are few things we can do to optimize it a bit and make it look nicer

3. Polishing code

First of all, our variables look bad, we can use programming cases to make variable names look a lot better, here are 3 main cases: Programming - What are the different Case Style?

Today i’ll use PascalCase

local Part = script.Parent
local Debounce = {}

local Cooldown: number = 5
local GoldValue: number = 10

Part.Touched:Connect(function(hit)
	if hit.Parent:FindFirstChild("Humanoid") then
		if game.Players:GetPlayerFromCharacter(hit.Parent) then
			if not Debounce[game.Players:GetPlayerFromCharacter(hit.Parent)] then
				Debounce[game.Players:GetPlayerFromCharacter(hit.Parent)] = 0
			elseif tick() - Debounce[game.Players:GetPlayerFromCharacter(hit.Parent)] >= Cooldown then
				game.Players:GetPlayerFromCharacter(hit.Parent).Gold.Value = game.Players:GetPlayerFromCharacter(hit.Parent).Gold.Value + GoldValue
			end
		end
	end
end)

Small change but if we have a lot of variables, it might be usefull

Another important thing we will use is how we will use variables, see you shouldn’t use them if something is repeated once or twice, but if something repeats a lot, it might help our redability

local Debounce = {}

local Cooldown: number = 5
local GoldValue: number = 10

script.Parent.Touched:Connect(function(hit)
	if hit.Parent:FindFirstChild("Humanoid") then
		local player: Player = game.Players:GetPlayerFromCharacter(hit.Parent)
		if player then
			if not Debounce[player] then
				Debounce[player] = 0
			elseif tick() - Debounce[player] >= Cooldown then
				player.Gold.Value = player.Gold.Value + GoldValue
			end
		end
	end
end)

If you are tired of my bad english, then you should know we will make ths code even better, by sorting if statements

local Debounce = {}

local Cooldown: number = 5
local GoldValue: number = 10

script.Parent.Touched:Connect(function(hit)
	local player: Player = game.Players:GetPlayerFromCharacter(hit.Parent)
	if player then
		if not Debounce[player] then Debounce[player] = 0 end
		if tick() - Debounce[player] >= Cooldown then
			player.Gold.Value = player.Gold.Value + GoldValue
		end
	end
end)

See some if statements are logically unnecesary and we can remove them, for instance checking if hit’s parent have humanoid is useless if we have no moving NPC in our game

Another thing about if statements is that we can use one line if statement to detect if something is not OK and change it

Note: After sorting this we can see that this script had bug, we forget to add cooldown update, soo we can add it right now

Apart of this, we must think now, what is useless here? one lined if statement is useless, it causes memory leaks and it should be run only once, we will add new format to clean out stuff

local Debounce = {}

local Cooldown: number = 5
local GoldValue: number = 10

local function OnPartTouched(hit: BasePart)
	local player: Player = game.Players:GetPlayerFromCharacter(hit.Parent)
	if player then
		if tick() - Debounce[player] >= Cooldown then
			player.Gold.Value = player.Gold.Value + GoldValue
			Debounce[player] = tick()
		end
	end
end

local function OnPlayerAdded(player: Player)
	Debounce[player] = 0
end

local function OnPlayerRemoving(player: Player)
	if Debounce[player] then
		Debounce[player] = nil
	end
end

script.Parent.Touched:Connect(OnPartTouched)
game:GetService("Players").PlayerAdded:Connect(OnPlayerAdded)
game:GetService("Players").PlayerRemoving:Connect(OnPlayerRemoving)

As you can see our code is a lot more redable and optimized rn, the last thing we can add is use roblox’s methoods and comments to shorten code

NOTE: You should always name functions that you connect events to like those event names
Example: OnPlayerAdded, OnPlayerRemoving, OnPartTouched

Let’s compare our two scripts:

Before sorting:

local sp = script.Parent
local db = {}

sp.Touched:Connect(function(hit)
if hit.Parent:FindFirstChild("Humanoid") then
if game.Players:GetPlayerFromCharacter(hit.Parent) then
if not db[game.Players:GetPlayerFromCharacter(hit.Parent)] then
db[game.Players:GetPlayerFromCharacter(hit.Parent)] = 0
elseif tick() - db[game.Players:GetPlayerFromCharacter(hit.Parent)] >= 5 then
game.Players:GetPlayerFromCharacter(hit.Parent).Gold.Value = game.Players:GetPlayerFromCharacter(hit.Parent).Gold.Value + 5
end
end
end
end)

After sorting:

local Debounce = {}

-- Settings
local Cooldown: number = 5
local GoldValue: number = 10

-- Gives set amount of gold to player that touched the part
local function OnPartTouched(hit: BasePart)
	local player: Player = game.Players:GetPlayerFromCharacter(hit.Parent)
	if player then
		if tick() - Debounce[player] >= Cooldown then
			player.Gold.Value += GoldValue
			Debounce[player] = tick() --/ Update cooldown
		end
	end
end

-- Adds player to debounce list
local function OnPlayerAdded(player: Player)
	Debounce[player] = 0
end

-- Removes player from debounce list
local function OnPlayerRemoving(player: Player)
	if Debounce[player] then
		Debounce[player] = nil --/ Prevent memory leaks
	end
end

script.Parent.Touched:Connect(OnPartTouched)
game:GetService("Players").PlayerAdded:Connect(OnPlayerAdded)
game:GetService("Players").PlayerRemoving:Connect(OnPlayerRemoving)

Tip that i forget to add is that you should use space:

  • Between yielding functions
  • Between functions
  • When you think it will look better
  • Between important comments (you decide which)

I wish i helped and that you understood this tutorial, i should use those tips to improve my english xd, anyways have a nice day, goodbye!

16 Likes
#2

To prevent memory leaks you should always use :Destroy() and other equivalent functions over setting instance to nil. Setting object to nil doesn’t remove it from memory.
Debounce has been assigned table class for which table.remove(Debounce, player) should be used.

4 Likes
#3

You are wrong, i don’t destroy player, but i remove reference from table, i can’t destroy reference because it’s not roblox’s object, destroying instance itself doesn’t remove it’s references

by doing:

Debounce[player] = nil
print(Debounce[player]) -- nil

i remove it from table

3 Likes
#4

These pretty much explain why you should not do that, and like I stated before, your instance remains in the table as a nil value, it does not get erased from the memory.

1 Like
#5

I tested it with milions of iterations in OOP, and no memory leaks happened, if you are right then you should remove index from table and destroy player, i believe it only happens when working with players due to cleaning made by roblox is incorrect

2 Likes
#6

Technically speaking this is true but the Player object gets destroyed automatically after the player has left the experience so it is not necessary in this context.

3 Likes
#7

Player object does get destroyed, but that destroyed object is unrelated to the table because inside table you save a variable data which can be anything (in this case it is player information), and even though we’re saving player data in the table you need to keep in mind that this information is in no way attached to player object, so after player leaves the game this variable is not destroyed because it is defined and saved inside some script which is outside of player object, meaning that you have to manually table.remove() data when player leaves the game.

#8

Bruh? i mean in this case i don’t store player, i reference player as number soo when i nulify index it will be GCed, i don’t have player stored inside the table, i have only number, soo nulifying it is the way to clear it

#9

This is not true here because it is a dictionary. Setting a value of a dictionary to nil will cause the reference to the key to also be removed. However if this was an array and you would manually set it to nil the index would indeed still store ‘nil’ that’s why you use table.remove for arrays.

as you can see in the code below key1 is set to nil and both the key and value are removed and the table takes less space

local httpService = game:GetService("HttpService")

local dictionary = {
	key1 = true,
	key2 = true
}

print(#httpService:JSONEncode(dictionary)) --> 25
dictionary.key1 = nil
print(#httpService:JSONEncode(dictionary)) --> 13

The proper way to do this for an array would be using table.remove like you said

local httpService = game:GetService("HttpService")

local t = {
	true,
	true
}

print(#httpService:JSONEncode(t)) --> 11
table.remove(t, 1)
print(#httpService:JSONEncode(t)) --> 6

And it is only a memory leak if you set the index of the array manually like you would using a dictionary

local httpService = game:GetService("HttpService")

local t = {
	true,
	true
}

print(#httpService:JSONEncode(t)) --> 11
t[1] = nil
print(#httpService:JSONEncode(t)) --> 11

To sum it all up OP is using the first example here using the dictionary which is not a memory leak.

3 Likes
#10

This post is much more informational and your code is easier to read in comparison to OP.
I’ve just tested myself, it is indeed key values as you’ve said, in this case using nil works.

1 Like
#11

The problem is that this is not about memory leaks but about writing clean code xd, i added comment as example of what you can comment

1 Like
#12

I see that myself now after testing the code, however for readability it is difficult to distinguish Debounce[player] from an array. If there was at least a comment or player information was set as local variable such as local keyValue = player it would be much easier to understand that we’re working here with a Dictionary and not an Array table.

2 Likes
#13

See, in roblox it’s pretty easy to distinguish arrays, lists and dictionaries

local array = buffer.create(5) -- array with size of 5 bytes
local list = {1,true,"hey"}
local dict = {
    ["Orange"] = 5,
    ["Lemon"] = 3
}
#14

This is exactly what I’m talking about. It is very easy to understand, a readable code format.

Personally I never use Dictionaries, I store all objects in an Array by numbers which match required item in a ServerStorage. For example if Potion item in ServerStorage have an integer value which is 101, then in Array list I need to table.find(Array, 101) in order to find the needed item, and vice versa.

1 Like
#15

The point of a garbage collector is to sweep all allocated objects and free them if they don’t have any more non-cyclic references. Roblox instances will also get freed when they no longer have any references and are parented to nil.

Since you remove the reference to the player object after it gets removed by the engine, it can be collected. It might not happen immediately, because the collector will only choose to run when its most efficient.

This would only be true if it isn’t the last index in the array portion. Otherwise, it is indistinguishable to having no value at all.

local Array = {1, 2, 3, 4, 5}
Array[5] = nil -- {1, 2, 3, 4}
#16

no, no. the real best method is to write all of your scripts at 3 in the morning on a saturday while being fueled by nothing but energy drinks and then to rewrite it the next morning after sleeping while wondering how you could manage to prepare such fine spaghetti in a single script

2 Likes
#17

This is good, but the example code has too much indentation. Programmers should avoid lots of indentation.

Example:
The code is easier to read with no indentation than before.

-- Gives set amount of gold to player that touched the part
local function OnPartTouched(hit: BasePart)
	-- Variable naming should be kept consistent e.g. PascalCase (which Roblox uses)
	-- Typing the variable to Player isn't needed as GetPlayerFromCharacter returns the Player type already
	local Player = game.Players:GetPlayerFromCharacter(hit.Parent)
	if not Player then return end -- Return nil if there is no player

	-- If cooldown is still active, return nil
	if tick() - Debounce[Player] < Cooldown then return end

	-- If all conditions are met, run the code
	Player.Gold.Value += GoldValue
	Debounce[Player] = tick() --/ Update cooldown
end
3 Likes
#18

Good tutorial! I like how modularized you made this code and provided important commenting. Using an instance as a dictionary’s key is a clever pattern.

Some points I wanted to add:

  • services should always be fetched first via GetService at the top of the script. Here I notice you use a mix of game:GetService("Players") and game.Service. My recommendation would be:
local Players = game:GetService("Players")
  • include a --!strict directive if you are going to use types!
  • some of the type annotations here are unneeded local Cooldown = 5 would infer that Cooldown is a number, no need to annotate this. The Debounce table however could benefit from a type annotation; I like to use a Map<K, V> type to specify my dictionaries
type Map<K, V> = { [K]: V }

local Debounce: Map<Player, number> = {}
  • prefer os.clock() over tick() for debounces. It typically has a faster resolution.
3 Likes
#19

This is a good tutorial, but there is a big thing that has been missed that I would like to add for beginners.

One of the biggest mistakes that newer programmers make is indentation. (Which I was sad to see was not mentioned in the tutorial,)

When you are reading over a heavily nested function, you have to 1. spend a long time moving left to right which either takes forever with arrow keys, or you have to use your mouse (Which a lot of programmers do not like to do.) and 2. You have to remember every single thing that took place before, like if you put 3 if statements inside of each other then you have to remember every single one of them, But there is another way that will fix this issue.

A good example in this script would be the OnPartTouched function

local function OnPartTouched(hit: BasePart)
	local player: Player = game.Players:GetPlayerFromCharacter(hit.Parent)
	if player then
		if tick() - Debounce[player] >= Cooldown then
			player.Gold.Value += GoldValue
			Debounce[player] = tick() --/ Update cooldown
		end
	end
end

There is two nested if statements in this. If player then and if tick() - Debounce[player] >= Cooldown then. You can see that that is a lot to remember for basically nothing. To fix that you can use if (parameter) then return end (or replace return with continue depending on the situation, but for this you should use return). You do have to Flip the value to do this though, so instead of if player then do if not player then

Here is how you can put that into the OnPartTouched function:

local function OnPartTouched(hit: BasePart)
	local player: Player = game.Players:GetPlayerFromCharacter(hit.Parent)
	
	if not player then return end --/ Not a player
	if tick() - Debounce[player] >= Cooldown then return end --/ Cooldown active

	player.Gold.Value += GoldValue
	Debounce[player] = tick() --/ Update cooldown
end

Easy as that! Your script now has less indentation and is easier to read.

also I would really recommend @ReturnedTrue’s comment because it gets into way more of the “complicated parts” instead of just indentation problems.

[Edit] Some of my wording was bad at the start, hopefully my English should be more readable now.

1 Like
#20

It really depends, i mentioned this kind of statement is used mostly to force exit loop or to perform single task like adjusting values, multi line if statements are great for scopes that we want to see perform something, not exit

if not state then return end --/ force exit

if tick() - Debounce[player] >= cooldown then --/ we perform more complex task after this
    -- code
end