How can I optimize my "spaghetti" code to be easier to read?

LuaRook · March 7, 2020, 1:18am

Hello,

I am currently unsatisfied with my code. I have created a LocalScript using Functions and UserInputType, etc, and created a bunch of spaghetti code which is very hard for my eyes to handle. On the other hand, it does get the job done, but is very hard to understand at times .

Improvements, I want to upgrade the Organization and purpose of using functions. The script shines runs off of just ONE Module Script and can get VERY HARD TO UNDERSTAND .
Maids and Classes, I would like to make everything depend off of classes using Module Scripts, and easier to configure.
It is not the best Filtering Enabled script, it needs to be improved greatly and is barely FE.

Code

-- Services
local UserInputService = game:GetService("UserInputService")

-- Main Variables
local tool = script.Parent
local aiming = false
local meleeing = false
local tagged = nil
local cooldown = false
local reloading = false
local debounce = false
local Player = game.Players.LocalPlayer
local ExpectingInput = false

local MouseEvent = tool:WaitForChild("MouseEvent")
local FirePointObject = tool.Emitter:WaitForChild("GunFirePoint")

local followMouse = false

local recoil = 0

local firing = false

local Handle = tool.Handle

-- Configuration
local animPath = game.Lighting.Animations.Rifle

local rps = 0.0666666667000
local reloadTime = tool.Handle.reload.TimeLength

local headshotDamage = 25
local defaultDamage = 17

local reloadRequirements = "and ammo.Value < ammo.MaxValue and not reloading and heldammo.Value > 0"

-- Functions
function reload()
	reloading = true
	tool.Handle.reload:Play()
	rld:Play(0.25)
	tool.Mag.Transparency = 1
	wait(reloadTime)
	tool.Mag.Transparency = 0
	reloading = false
	heldammo.Value = heldammo.Value - 1
	ammo.Value = ammo.MaxValue
end

function enable()
	aim:Play(0.25)
	tool.CLIENT_SCRIPT.Disabled = false
	tool.CLIENT_TORSOSCRIPT.Return:Fire(true)
	hold:Stop()
	print('Animation success.')
	tool.GripForward = Vector3.new(-0.1, 0, -0.9)
	tool.GripRight = Vector3.new(0.9, 0, -0.1)
	aiming = true	
end

function disable()
	aim:Stop()
	tool.CLIENT_SCRIPT.Disabled = true
	tool.CLIENT_TORSOSCRIPT.Return:Fire(false)
	hold:Play(0.25)
	print('Animation success.')
	tool.GripForward = Vector3.new(0, 0, -1)
	tool.GripRight = Vector3.new(1, 0, 0)
	aiming = false
end

function shoot(hitPart)
	while firing and ammo.Value > 0 and aiming do
		recoil = recoil + .5
		ammo.Value = ammo.Value - 1
		fire:Play()
		aim:Play(0.25)
		hold:Stop()
	
		end
	end

function unEquip()
	tool.CLIENT_SCRIPT.Disabled = true
	aim:Stop()
	hold:Stop()
	melee:Stop()
	fire:Stop()
	rld:Stop()
	ExpectingInput = false
	aiming = false
end

-- Main Script
tool.Equipped:connect(function(mouse)
	local character = tool.Parent
	local hum = character.Humanoid
	local Player = game:GetService("Players").LocalPlayer
	ExpectingInput = true
	
	local rlMouse = Player:GetMouse()
	
	ammo = script.Parent.Ammo
	heldammo = script.Parent.Stock
	
	mouse.Icon = "http://www.roblox.com/asset/?id=106491038"
	
	hold = hum:LoadAnimation(animPath.idle)
	aim = hum:LoadAnimation(animPath.aim)
	fire = hum:LoadAnimation(animPath.fire)
	melee = hum:LoadAnimation(animPath.melee)
	rld = hum:LoadAnimation(animPath.reload) 
	
	hold:Play(0.25)
	debounce = true

	
	UserInputService.InputBegan:Connect(function (input, gameHandledEvent)
		if gameHandledEvent or not ExpectingInput then
			return
		end
		
		if input.UserInputType == Enum.KeyCode == Enum.KeyCode.R and ammo.Value < ammo.MaxValue and not reloading then
			reload()
			
		if input.UserInputType == Enum.KeyCode == Enum.KeyCode.E and not meleeing and not reloading then
			if aiming == false then
				-- Enabled
				enable()
			elseif aiming == true then
				-- Disabled
				disable()
			end
		end

		if input.UserInputType == Enum.UserInputType.MouseButton1 then
			if aiming and not meleeing and not cooldown and ammo.Value > 0 and not reloading then
				local FireDirection = (mouse.Hit.p - FirePointObject.WorldPosition).Unit
				aim:Play(0.25)
				hold:Stop()
				tool.CLIENT_SCRIPT.Disabled = false
				firing = true
				cooldown = true
				shoot()
				MouseEvent:FireServer(FireDirection)
				wait(.01)
				cooldown = false
				fire:Stop()
			elseif not meleeing and not aiming and not reloading and not firing then
			end
		end
	end
end)
end)

tool.Unequipped:connect(function()
	unEquip()
end)

Thanks,
Rook.

Ropexal · March 7, 2020, 1:44am

I think your code is fine, but you could move some functions like Reload() to a module-script so don’t have to repeat it.
Module scripts are not classes. Any changes made to a module-script from a variable will sync with all other variables unlike a class.
There is no such thing as barely fe, its either filtering enabled or not filtering enabled there is no in-between.

MrLonely1221 · March 7, 2020, 3:23am

This doesn’t really look like spaghetti code to me.

HilyrHere · March 14, 2020, 7:25pm

If at times you don’t understand your code you could add comments, I’ve heard somewhere a good way to think of them is explanations, explanations for why you did it like that

r_r · March 15, 2020, 6:24am

I have a lot to comment on, but note that I haven’t read your code with any regard to functionality or potential redundancy or incompleteness. I’m only concerned with your formatting.

Your variable naming preference isn’t consistent, you seem to have a combination of camelCase and PascalCase. I suspect not all of this code was written by you (because the practice preferences are all over the place), although correct me if I’m wrong.

Variables named like this are named using the naming convention camelCase:

But you have variables named like this, which use the naming convention PascalCase:

Naming is an important aspect when considering code readability, so its best to stick to one or the other. Roblox uses PascalCase for all of its non-deprecated functions, like WaitForChild, FindFirstChild, etc.

Your formatting is improper and confusing, decreasing code readability, and the way you group your variables seems arbitrary.

These are all grouped together:

But these are spaced out and separated:

Additionally, each function, if statement, and loop should have the code inside it indented to indicate that code will execute in its respective function, if statement, or loop.

For example, this would exemplify good practice in formatting and indentation:

local function Count(Number)
	-- This indentation indicates that
	-- the following indented code will be
	-- executed when the function is called.
	for i = 1, Number, 1 do
		-- This indentation indicates that
		-- the following indented code will be
		-- executed in this for loop.
		print(i)
		if i == 3 then
			-- This indentation indicates that the
			-- print statement will only execute when
			-- "i" is equal to 3.
			print("Reached 3!")
		end
	end
end

Count(10)

Whereas your code has sections like this, where it isn’t clear which section of code will be ran when, and which end belongs to its corresponding statement.

You can see in my example function above that each function, loop, and statement has a corresponding end statement aligned with it, and this makes your code more clear and increases readability.

What leads you to believe this?

Sailor_Bea · August 9, 2020, 4:24am

Sailor_Bea · August 9, 2020, 4:24am