My combat code looks ugly and idk why

Help and Feedback Code Review
#1

Is there anyone to make this easier to look at? or just better organized

local LTimer = nil
local RTimer = nil

function Click(actionName, inputState, inputObject)
	if (not Character:FindFirstChild("Debounce"))  then
		if actionName == "LeftClick" then
			if inputState == Enum.UserInputState.Begin then
				LTimer = tick()
			end
			if inputState == Enum.UserInputState.End then
				if tick()-LTimer > 0.4 then
					Remotes.Combat:FireServer("L")
				else
					Remotes.Combat:FireServer("LHeavy")
				end
				LTimer = nil
			end
		elseif actionName == "RightClick" then
			if inputState == Enum.UserInputState.Begin then
				RTimer = tick()
			end
			if inputState == Enum.UserInputState.End then
				if tick()-RTimer > 0.4 then
					Remotes.Combat:FireServer("R")
				else
					Remotes.Combat:FireServer("RHeavy")
				end
				RTimer = nil
			end
		end
	end
end

ContextActionService:BindAction("LeftClick",Click, true, Enum.UserInputType.MouseButton1)
ContextActionService:BindAction("RightClick",Click, true, Enum.UserInputType.MouseButton2)
#2

It looks pretty good overall, but I have a few small suggestions that I think you might be interested in trying.

You could try introducing some empty lines to add some “pacing” to the code by visually separating different logical groupings. This can make it easier to tell what belongs together and what should be read as 1 unit. It also makes room for adding comments without making the code look like one big word wall.

Example:

			-- optional comment: handle button press logic
			if inputState == Enum.UserInputState.Begin then
				LTimer = tick()
			end

			-- optional comment: handle button release logic
			if inputState == Enum.UserInputState.End then
				-- timer response stuff is separated from the timer change
				if tick()-LTimer > 0.4 then
					Remotes.Combat:FireServer("L")
				else
					Remotes.Combat:FireServer("LHeavy")
				end

				LTimer = nil
			end

One reason why the code might look daunting to read is that there are several nested if blocks. This increases the amount of complexity and possible number of branches in the code. It also pushes the code farther and farther to the right.

This could be combatted by taking note of duplicated logic between branches, and removing that code from the if blocks. You would use variables to control how the logic behaves instead. This sadly isn’t always possible, but it is made easier by dynamic languages like Lua, vs something like C++ where visually identical code functions completely differently.

Example:

   			if tick()-RTimer > 0.4 then
   				Remotes.Combat:FireServer("R")
   			else
   				Remotes.Combat:FireServer("RHeavy")
   			end

changed into

-- Use a ternary operator: "condition and valueIfTrue or valueIfFalse"
-- Ternary operators pick the first or the second value depending on if the input condition is true or not
Remotes.Combat:FireServer((tick()-RTimer > 0.4) and "R" or "RHeavy")
-- or
local serverMessage = (tick()-RTimer > 0.4) and "R" or "RHeavy"
Remotes.Combat:FireServer(serverMessage)
2 Likes
#3

Ive never seen the variable thing that is amazing thank you. Is there any reference I can read on the topic?

#4 
local Timer = {
    L = nil,
    R = nil
}

function FireServer(ClickType)
    if inputState == Enum.UserInputState.Begin then
        Timer[ClickType] = os.time()
    end

    if inputState == Enum.UserInputState.End then
        Remotes.Combat:FireServer((os.time() - Timer[ClickType] > 0.4) and ClickType or ClickType.."Heavy")

        Timer[ClickType] = nil
    end
end

function Click(actionName, inputState, inputObject)
    if not Character:FindFirstChild("Debounce") then
        if actionName == "LeftClick" then
            FireServer("L")
        elseif actionName == "RightClick" then
            FireServer("R")
        end
    end
end

ContextActionService:BindAction("LeftClick", Click, true, Enum.UserInputType.MouseButton1)
ContextActionService:BindAction("RightClick", Click, true, Enum.UserInputType.MouseButton2)

I removed the repeated code and replaced tick() with os.time()
I also added a ternary statement like @MettaurSp suggested

just a side note I named that function FireServer because I didn’t know what to name it, you can change the name

if you have any questions about the code just ask

EDIT:
missed a parentheses

1 Like
#5

I’m sure there are talks about code styling out there, but I don’t have any on hand, sorry.

A lot of this type of refactoring is just small things that you pick up over time as you program. As long as you are critical of your own code and looking for places where it may be lacking or could be improved where necessary you should make steady progress over time.

I’d recommend occasionally looking at the language specs or other peoples’ code because you might find some features or tricks that you didn’t know about before. A little experimentation every once in a while can go a long way.

Also I’d like to point out that adding new helper functions for repeated logic is also a very nice way to clean up the code, like D0RYU demonstrated.

1 Like
#6 
function Click(actionName, inputState, inputObject)
    if not Character:FindFirstChild("Debounce") then
        FireServer(string.sub(actionName, 1, 1))
    end
end

this is up to you if you want to add this or not, but it is possible to do this without the if statements by using string.sub

basically it just gets the first character of actionName

1 Like
#7

So the issue im running into is that i want the Heavy to fire after the Mouse1 has been held for 0.4 seconds, but right now it only reads on key release

#9

maybe change this line into this

Remotes.Combat:FireServer((os.time() - Timer[ClickType] > 0.4) and ClickType.."Heavy" or ClickType)

I switched ClickType.."Heavy" and ClickType

2 Likes
#10

Ah i didnt relies they were switched, thank you

1 Like