Rewriting code and reorganizing a game: journey through a disaster (Part 1)

Resources Community Tutorials
1

This is kind of a blog for people looking for examples about code rewriting and reorganizing of a project.

Who am I?

I am Wolo, the main scripter of the Roblox RTS game [( ✦ ) [RUSSIA] Conquest: Napoleonic Wars] ([RUSSIA] Conquest: Napoleonic Wars [Pre-Alpha] - Roblox) and ( ✦ ) Ages Of Iron 2: Punic Wars [Early Alpha]. I have been interested in RTS from 13. Over 2+ years of experience in Roblox scripting and 3+ years as freelancer. I am also the owner of 3DBlox Games, a group recently made. 3DBlox Games - Roblox

I also have a short video about my experience as a Roblox RTS developer: My experience as a Roblox RTS games developer - YouTube

---------------------------------------------------------------------------------------------------

Introduction

Back when I was developing Conquest: Napoleonic Wars I made a lot of mistakes when writing its code and deciding when X thing goes to X. Hopefully, this post about me fixing all the mistakes can help you. Remember, its part 1. There is a lot of code and I will fix prt by part while I make posts to help scripters having problem with ugly code.

---------------------------------------------------------------------------------------------------

General tips about how to write clean code when developing your games

Before starting with how I fixed part by part of my game I would like to give some general tips for writing clean code:

  • Use headers to separate parts of the script
    Using ------------------------------------NAME OF THE SECTION------------------------------------------------- to separate parts of the scripts is really nice to make it clear where each thing belongs.
-----------------------SERVICES---------------------------
local Players = game:GetService("Players")
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local SoundService = game:GetService("SoundService")


----------------------MODULES---------------------------
local LocalGameplayData = require(ReplicatedStorage.Client.ModuleScripts.LocalGameplayData)
local Enumerations = require(ReplicatedStorage.Shared.ModuleScripts.Misc.Enumerations)

local Players = game:GetService("Players")
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local SoundService = game:GetService("SoundService")

local LocalGameplayData = require(ReplicatedStorage.Client.ModuleScripts.LocalGameplayData)
local Enumerations = require(ReplicatedStorage.Shared.ModuleScripts.Misc.Enumerations)
  • Avoid nesting!
    Nesting is basically including code within another, like this
if condition then
    nested code
end

One nested code like that is alright, the problem is when this happens

if condition then
      if condition then
               if condition then
                     if condition then

                      end
               end
      end
end

When you do “too much nesting”, the code becomes a lot harder to understand. To fix this, I like to use one trick that solves most situations regarding nesting for me.

Lets see this code

GUIContainer.TroopsBar.ChildAdded:Connect(function(child: Frame)
	if (child:IsA("Frame")) then
		child.InputBegan:Connect(function(input: InputObject)
			if (input.UserInputType ~= Enum.UserInputType.MouseButton1) then
				--do stuff
			end
		end)
	end

The nesting has already become a problem. What if, instead of checking if child is not a frame and then nesting the rest of the code within the if, we check if child is a frame first and if not we return?

GUIContainer.TroopsBar.ChildAdded:Connect(function(child: Frame)
	if (not child:IsA("Frame")) then return end

	child.InputBegan:Connect(function(input: InputObject)
		if (input.UserInputType ~= Enum.UserInputType.MouseButton) then return end
		--Do stuff
	end)
end)

The different is not super wow in this example, but in other scenarios using this anti nesting idea can help A LOT.

  • Write comments to explain why you are doing something, not how you are doing it

  • Every object name in the explorer must be completely representative of what it is made for. If the name does not indicate it serves X purpose, then do not use it for that purpose.

  • If a module script is designed for X thing, then do not add unrelated functionality to it. Make another module. Same with scripts, do not add unrelated functionality to a script that was not supposed to have it.

  • Every variable name in your code must represent what the variable is used for. No single letter variables.

  • Similar to variables, every function name must tell you what its used for.

What are all these value objects…? No!

image
image463×249 12 KB

Yeah, not the best idea. In the game, you can press different buttons while in gameplay to change some specific things. What we are doing here is clearly not optimal.. So, what if we use a module script instead?

local Data = {
	FormationSelected = "none",
	SelectedFlagID = "",
	SelectedSkinName = "",
	InfantryAutoRotate = false,  
	EnableMobileUI = false,
	MarchAtSameSpeed = false,
	MarchInFormation = false,
	PingEnabled = false,
	PlayerIsPlaying = false,
	ShowUnitPaths = false,
	ShowUnitsRange = false,
}

This is a lot better. Instead of having a lot of value objects in the explorer, I have a module script, which is miles better than having a lot of value objects in the explorer. This also improves client loading time a bit.

Maybe separate the functions from the main UI script?

I have two UIs in Conquest. One for mobile and one for PC. The code of both is very similar, but I had copy pasted the entire script with the functions of the buttons. It is already questionable that I had the functions that the buttons activated in the UI script, but having another one that is very similar with the same mistake becomes a nightmare.

To solve this, I separated the functions to another module.

local ButtonsWithFunctions = {
	MarchAtSameSpeed = {
		Button = MarchAtSameSpeedButton,
		Function = GameplayUIButtons.MarchAtSameSpeed
	},
	MarchInFormation = {
		Button = MarchInFormation,
		Function = GameplayUIButtons.MarchInFormation
	},
	PingButton = {
		Button = PingButton,
		Function = GameplayUIButtons.PingEnabled
	},
	FormationsOpeningButton = {
		Button = FormationsOpeningButton,
		ArgReplacement = FormationButtons, --this is what we will give to the funciton if the property is available
		Function = GameplayUIButtons.ToggleFormationsButtonVisibility
	},
	FormSquareButton = {
		Button = FormSquare,
		Function = GameplayUIButtons.FormSquare
	},
	ShowRangeButton = {
		Button = ShowRangeButton,
		Function = GameplayUIButtons.ToggleUnitsRange
	},
	RotateUnitsButton = {
		Button = RotateUnitsButton,
		Function = GameplayUIButtons.RotateUnits
	},
	HoldFireButton = {
		Button = HoldFireButton,
		Function = GameplayUIButtons.HoldFire
	},
	HaltButton = {
		Button = HaltButton,
		Function = GameplayUIButtons.HaltSelectedUnits
	},
	ToggleSelectAll = {
		Button = ToggleSelectAllButton,
		Function = GameplayUIButtons.SelectAllUnitsToggle
	},

	ToggleInfantryCones = {
		Button = ToggleInfantryConesButton,
		Function = GameplayUIButtons.ToggleInfantryConeFOV
	},

}

This way, I just had to link the functions to the activation of the buttons.

for _, buttonData in ButtonsWithFunctions do
	local Button: ImageButton = buttonData.Button
	local Function = buttonData.Function
	local ArgReplacement = buttonData["ArgReplacement"]

	Button.Activated:Connect(function()
		if (ArgReplacement ~= nil) then
			Function(ArgReplacement)
		else
			Function(Button)
		end
	end)
end

This is considerably better, right?

I suppose there is a better way of doing this. If you have an idea, just comment it!

But I need to detect when the value is changed!

This can be solved using a bindable event and an enum. First, I set a module that will have the enums the game will use. Think of enums as “identifiers” that we will use to know what to do, search, get, etc.

Enumerations.ControlEvent = {
	EnabledMobileUI = "EnableMobileUI"
}

I also set a bindable event named “ControlEvent”, that fires like this

ControlEvent:Fire(Enumerations.ControlEvent.Name, value)

Now, I just go back to the module previously created and add two functions; one for getting the data value and one for setting it. This after obviously moving the properties from the module to another one.

local Data = {
	FormationSelected = "none",
	SelectedFlagID = "",
	SelectedSkinName = "",
	InfantryAutoRotate = false,  
	EnableMobileUI = false,
	MarchAtSameSpeed = false,
	MarchInFormation = false,
	PingEnabled = false,
	PlayerIsPlaying = false,
	ShowUnitPaths = false,
	ShowUnitsRange = false,
}

function module:SetData(Name: string, value: any)
	if Data[Name] == nil then return end
	if typeof(Data[Name]) ~= typeof(value) then return warn(`Type of value for data {Name} is incorrect`) end
	
	Data[Name] = value
	
	ControlEvent:Fire(Name, value)
end

--Use the enum module with enum.ControlEvent.Name for the name argument when using the 
--function
function module:GetData (Name: string): any?
	if Data[Name] == nil then return warn(`Data named {Name} not found`) end 
	
	return Data[Name]
end

When writing a string, you can use `` instead of “”. The advantage of using ` compared to " is that you can do

Data named {Name} not found

Instead of

“Data named " .. Name .. " not found”

Basically, using ` allows you to include variables in the string with {}, allowing for cleaner code.

Now, an example of how I replace an outdated code that detected value change on object with ControlEvent and the enum:

------WITH CONTROL EVENT
ControlEvent.Event:Connect(function(Name: string, Value: any)
	if Name ~= Enumerations.ControlEvent.EnabledMobileUI then return end
	
	if (not Value) then
		PlayerUnitsCardList = PlayerUIContainer:WaitForChild("TroopsBar")
	else
		PlayerUnitsCardList = PlayerMobileUIContainer:WaitForChild("TroopsBar")
	end
end)

----DETECTING VALUE CHANGE ON OBJECT
EnableMobileUI:GetPropertyChangedSignal("Value"):Connect(function()
	local Value: boolean = EnableMobileUI.Value
	
	if (not Value) then
		PlayerUnitsCardList = PlayerUIContainer:WaitForChild("TroopsBar")
	else
		PlayerUnitsCardList = PlayerMobileUIContainer:WaitForChild("TroopsBar")
	end
end)

See why we use enums? It allows me to make sure I am checking the correct data field change.

Fixing ugly code

Whats this…?

local InfantryAutoRotate

local GUI = script.Parent
local Buttons = GUI.Container:WaitForChild("Buttons")
local Formations = GUI.Container:WaitForChild("Formations")
--local InfantryAutoRotate: ImageButton = GUI.Container.ExtraButtons:WaitForChild("InfantryAutoRotate")

local LocalPlayer = game.Players.LocalPlayer

for i, button: ImageButton in script.Parent.Container:GetDescendants() do
	if (button:IsA("ImageButton") == false) then continue end
	
	button.MouseButton1Click:Connect(function()
		script.Click:Play()
	end)
end

script.Parent.Container.TroopsBar.ChildAdded:Connect(function(child: Frame)
	if (child:IsA("Frame") == nil) then return end
	child.InputBegan:Connect(function(input: InputObject)
		if (input.UserInputType ~= Enum.UserInputType.MouseButton1 and input.UserInputType ~= Enum.UserInputType.Touch) then
			return
		end
	end)
end)


--Temporary stuff
game.Players.LocalPlayer.CharacterRemoving:Connect(function()
	script.Parent.Enabled = false
end)

local PlayerScripts = game.Players.LocalPlayer.PlayerScripts
local EnableMobileUI: BoolValue = PlayerScripts.EnableMobileUI

local PlayerPlayingEvent = game:GetService("ReplicatedStorage").Client.BindableEvents.PlayerIsPlaying -- THIS IS TEMPORARAY GET RID WHEN DONE
PlayerPlayingEvent.Event:Connect(function()
	if (EnableMobileUI.Value == true) then return end
	GUI.Enabled = true
end)

Yeah, this code is absolutely ugly. It has the mistake of declaring global variables below, the way it acceses things make it messy (ex: game.Players.LocalPlayer.CharacterRemoving) and it has no separators between parts of the script. Lets fix it!

First, lets move all variables to the top of the script, next we will require services and import modules, then change things like game.Players.LocalPlayer.CharacterRemoving to LocalPlayer.CharacterRemoving, finally, separate variables, services, imports and events on the top of the script with some headers.

-----------------------SERVICES---------------------------
local Players = game:GetService("Players")
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local SoundService = game:GetService("SoundService")


----------------------MODULES---------------------------
local LocalGameplayData = require(ReplicatedStorage.Client.ModuleScripts.LocalGameplayData)
local Enumerations = require(ReplicatedStorage.Shared.ModuleScripts.Misc.Enumerations)

----------------------EVENTS-----------------------------
local PlayerPlayingEvent = ReplicatedStorage.Client.BindableEvents.PlayerIsPlaying -- THIS IS TEMPORARAY GET RID WHEN DONE

----------------------VARIABLES--------------------------
local LocalPlayer = Players.LocalPlayer
local ClickSound = SoundService.SoundEffects.ClickSound

local GUI = script.Parent
local GUIContainer = GUI.Container

local Buttons = GUIContainer:WaitForChild("Buttons")
local Formations = GUIContainer:WaitForChild("Formations")

This is looking a lot better, isnt it?

Additionally, separate the rest of the script with headers aswell.

-----------------HANDLING EVENTS-----------------------
GUIContainer.TroopsBar.ChildAdded:Connect(function(child: Frame)
	if (not child:IsA("Frame")) then return end
	child.InputBegan:Connect(function(input: InputObject)
		if (input.UserInputType ~= Enum.UserInputType.MouseButton1 and input.UserInputType ~= Enum.UserInputType.Touch) then
			return
		end
	end)
end)

LocalPlayer.CharacterRemoving:Connect(function()
	GUI.Enabled = false
end)

--This only fires when player deploys so dont worry about memory leaks
PlayerPlayingEvent.Event:Connect(function()
	if (LocalGameplayData:GetData(Enumerations.ControlEvent.EnabledMobileUI) == false) then return end
	
	GUI.Enabled = true
end)

----------------------ADDING BUTTONS SOUND----------------------

for _, button: ImageButton in script.Parent.Container:GetDescendants() do
	if (button:IsA("ImageButton") == false) then continue end
	
	button.MouseButton1Click:Connect(function()
		ClickSound:Play()
	end)
end

End of part 1 and links to my other posts

I would like to thank u for reaching this part. If you wish to support me, you can do so by joining this game and donating: Wolo - Donation Place - Roblox or buying clothes in my group

Here are links to my other guides that you may find useful in the development process of your game:

Freelancer guide: mistakes to avoid when looking for commissions - Resources / Community Tutorials - Developer Forum | Roblox

General guide on making Roblox RTS games: What you should know before starting - Resources / Community Resources - Developer Forum | Roblox

The Bandwith Art: learning how to optimize and improve your game network performance - Resources / Community Resources - Developer Forum | Roblox

5 Likes
2

just a few nitpicks I have regarding this, the first, is you’ve been slightly inconsistent in how you check something, for example in one spot you do

if (child:IsA("Frame") == nil) then

but later do

if (button:IsA("imageButton") == false) then

which is just being inconsistent, overall doesn’t pose a huge problem, but it’s minorly annoying to me at least
additionally, you’d don’t even NEED the == false or == nil, you can simply do

if instance:IsA("ClassName") then

and if you need it to only happen when it isn’t that class, you can just use the not keyword

if not instance:IsA("ClassName") then

this applies to almost every if statement that happens, if you’re simply verifying if the value is truthy(any value other than nil or false), you can just do if value then, no need to do if value ~= nil then, and for verifying if it’s falsy(nil or false), you just need to do if not value then

now that my nitpick of your if statements are done, my other nitpick has to do with your for loop
you never use the index/key variable you defined, as such, it’s considered proper to define unused variables as _, this is to show that it’s not used for anything(technically it’s still a valid variable, its more for readability, not actually throwing it out)

3

You are right, I did not really think about these when writing the guide, mainly because they are some habits of mine :rofl:

But yeah, I will edit the post in a moment to replace it with the ones u suggest, thanks!

4

Best tutorial i ever seen! I didnt thing about organizing code before if we expect OOP, thanks you! I will check other tutorials too!

1 Like
5

you say it’s the best tutorial and you don’t like it, that’s weird

6

for real, let me fix this. Actully thats mine bad