How can I make this code improved and look cleaner?

Help and Feedback Code Review
#1

I’m making a moveset module, and I want to make it not only improved in quality, but also look cleaner and more readable.

local module = {}

module.LJCoolDown = false
module.CCooldown = false
module.DCooldown = false
module.GPCooldown = false

-- moves function
function module.moves(input, character)
	local hum = character:FindFirstChildOfClass("Humanoid")
	if input == "LJ" then
		if not module.LJCoolDown then
			module.LJCoolDown = true
			
			local root = character:FindFirstChild("HumanoidRootPart")
			
			hum.AutoRotate = false
			
			root:ApplyImpulse((1250 * root.CFrame.LookVector) + Vector3.new(0, workspace.Gravity * .5, 0))
			
			task.wait(.8)
			
			hum.AutoRotate = true
			module.LJCoolDown = false
		else warn("Long Jump Is On Cooldown.") end
	elseif input == "Crouch" then 
		if not module.CCooldown then
			module.CCooldown = true

			task.wait(1)

			module.CCooldown = false
		else warn("Crouch Is On Cooldown.") end
	elseif input == "Dive" then
		if not module.DCooldown then
			module.DCooldown = true print("Dive Activated")

			task.wait(1)

			module.DCooldown = false
		else warn("Dive Is On Cooldown.") end
	elseif input == "GP" then
		if not module.GPCooldown then
			module.GPCooldown = true print("Ground Pound Activated")

			task.wait(1)
		else
			warn("Ground Pound Is On Cooldown.")
		end
	end
end

-- state manager


return module
1 Like
#2

Honestly, if you are making a module for other users, it won’t matter because they will require the module. But are you trying to make it cleaner/more organized for yourself, or other people? BTW if you want to improve quality, I would recommend making comments explaining the different parts of code.

#3

Myself

#4

I got bored and rewrote some pseudo-code of what could be changed in your code. I tried to describe as much as possible in the comments, but I’m not sure how much they’ll help.

Essentially, I placed the Cooldown/Moves inside a map so I can then do a lookup for them to reduce the huge if-block. I also added some nil-checking for the Humanoid and RootPart since those can potentially not exist.

-- // Maps

-- We put the Cooldowns into a Map
-- So we can easily check if they are active
-- With a simple lookup
local CooldownMap = {
	LongJump = false,
	Crouch = false,
	Dive = false,
	GroundPound = false
}

-- Same for our Moves
-- We lookup the Function for the corresponding Move
-- And pass the Humanoid and RootPart through
local MoveMap = {
	
	LongJump = function(Humanoid: Humanoid, RootPart: BasePart): ()
		CooldownMap.LongJump = true
		
		Humanoid.AutoRotate = false
		RootPart:ApplyImpulse(
			(RootPart.CFrame.LookVector * 1250)
			+ Vector3.yAxis * (workspace.Gravity * 0.5)
		)
		
		-- This will yield the running thread everytime you call it
		-- You may want to use task.delay instead
		task.wait(.8)
		
		Humanoid.AutoRotate = true
		CooldownMap.LongJump = false
	end,
	
	Crouch = function(Humanoid: Humanoid, RootPart: BasePart): ()
		...
	end,
	
}


-- // Module Interface

-- Our type for Input so we get AutoComplete
type Input = "LongJump" | "Crouch" | "Dive" | "GroundPound"

local Module = {
	
	-- 'UseMove' describes the function's job more concisely than 'Moves'
	UseMove = function(Input: Input, Character: Model): ()
		-- Get the Humanoid & RootPart
		local Humanoid: Humanoid? = Character:FindFirstChildOfClass("Humanoid")
		local RootPart: BasePart? = if Humanoid then Humanoid.RootPart else nil
		
		-- Remember, these could potentially not exist
		-- So you should always check if they do
		if not Humanoid or not RootPart then
			warn("No Humanoid or RootPart") -- Maybe add some error handling?
			return
		end
		
		-- If the Cooldown isn't Active
		if not CooldownMap[Input] then
			-- Call Relative Function in Map
			MoveMap[Input](Humanoid, RootPart)
		end
		
	end,
	
}


return Module

Sorry in advance for the excessive TypeChecking. But if you want saleable code, having type annotations can tremendously help so I encourage you to learn about it.

1 Like
#5

What is a map and pseudo-code?

And also, can you explain to me what you exactly changed? Because this looks like it was made in a slightly altered version of lua code, and I’m confused on the stuff like the colons and end points having a , mark.

#6

Sorry, I may have gone a bit overkill with everything.

The extra colons and such are type annotations which help the editor understand what each variable is and allow it to make autocompletion improvements from it.

Pseudo-Code is code that isn’t written to work but to showcase a way of how things could be written.

Essentially a map is a dictionary that we put a fixed number of stuff in for us to grab and edit later. It has named keys and values for each piece of data we add.

So instead of having multiple if-statements such as

if Input == "LongJump" then
	LongJump_Cooldown = true
elseif Input == "Crouch" then
	...

We can just set the key for what we want inside a map

local CooldownMap = {
	LongJump = false,
	Crouch = false,
	Dive = false,
	GroundPound = false
}

CooldownMap[Input] = true

The same can go for your moves, which can be placed inside a map as functions. You can then call these functions after grabbing them from the map.

local MoveMap = {
	LongJump = function(Humanoid, RootPart)
		
	end,
	-- etc.
}

local Move = MoveMap[Input]
Move(Humanoid, RootPart)

Or alternatively:

MoveMap[Input](Humanoid, RootPart)
1 Like
#7

What do the | you’ve placed in the code resemble?

#8

This is a Type Union declaration.

type t = "a" | "b"

The "a" and "b" are string singletons and the | represents t being either "a" or "b"

So, with this you can get autocomplete for functions

local function f(data: "a" | "b")
	print(data) -- data can only be "a" or "b"
end

I did this mainly because Input can only be a few specific strings, and by annotating that, I won’t need to remember every possible string for Input and won’t make as many bugs.

1 Like