How do I improve and optimize this section of my code?

Help and Feedback Code Review
, ,
1

This section of my code is a heavy attack function called from ContextActionService, and I’m curious if there’s a different approach I could take (since it feels fairly obvious that I’m missing something from lack of knowledge).
My sword is based on a table with strings; states, and the main part I’m not happy with is the sequential elseifs and task.waits.

Client:

function swordHeavy(actionName, inputState)
	if inputState == Enum.UserInputState.Begin then
		if heavyReady == false then return else
			if currentState ~= states[1] then return else
				currentState = states[7]
				sword.Events.SpeedEvent:FireServer(currentState, 1)
				heavyAnimTrack:Play()
				task.spawn(swordTimer, "heavy")
				task.wait(0.75)
				sword.Events.SpeedEvent:FireServer(currentState, 2)
				task.wait(0.5)
				sword.Events.SpeedEvent:FireServer(currentState, 3)
				task.wait(0.70)
				sword.Events.SpeedEvent:FireServer(currentState, 4)
				currentState = states[1]
			end
		end
	end
end

Server:

local function speedEvent(plr, action: string, mode: number)
	if action == states[3] then
		if mode == 1 then
			moduleScript.updatePlayerHumanoid(char, "WalkSpeed", 10)
		elseif mode == 2 then
			moduleScript.updatePlayerHumanoid(char, "WalkSpeed", 16)
		end
	elseif action == states[7] then
		if mode == 1 then
			moduleScript.updatePlayerHumanoid(char, "WalkSpeed", 5)
		elseif mode == 2 then
			moduleScript.updatePlayerHumanoid(char, "WalkSpeed", 70)
		elseif mode == 3 then
			moduleScript.updatePlayerHumanoid(char, "WalkSpeed", 10)
		elseif mode == 4 then
			moduleScript.updatePlayerHumanoid(char, "WalkSpeed", 16)
		end
	end
end
2

dont spam the else, just use end, your overly nesting for no reason (on the client) ^

it looks like you can for loop this. ^
for example:

local timetowait = {.75, .5, .7, 0)

for i=1, 4 do
      sword.Events.SpeedEvent:FireServer(currentState, i)

      if i == 1 then
	    heavyAnimTrack:Play()
	    task.spawn(swordTimer, "heavy")
      end

      task.wait(timetowait[i]) --you can find better ways to do this.
end

you could make a table to define all the walkspeeds for each mode. ^

ex:

local wsForState3 = {10, 16}
local wsForState7 = {5, 70, 10, 16}

local function speedEvent(plr, action: string, mode: number)
	if action == states[3] then
		moduleScript.updatePlayerHumanoid(char, "WalkSpeed", wsForState3[mode])
	elseif action == states[7] then
		moduleScript.updatePlayerHumanoid(char, "WalkSpeed", wsForState7[mode])
	end
end

also, spamming RemoteEvents like that is really bad, use Atrributes, Values, literally any other defining factor.

you can just use a .Changed or a :GetPropertyChangedSignal if you were to use a value, which yes will take up a little bit of memory but spamming RemoteEvents can lead to sever/client lag

2 Likes
3

this could use a table, I’m thinking something like this

local walkspeeds = {
action = {
mode = walkspeed,
mode = walkspeed
},
action = {
mode = walkspeed,
mode = walkspeed 
}
}

it will be easier to add more modes/actions and needs very little code to use

setwalkspeed(char, walkspeeds[action][mode])
1 Like
4

Use tables instead of if statements

local Switch = {
    ["Mode1"] = function()
        -- code
    end)
    ["Mode2"] = function()
        -- code
    end)
}

-- code

local Function = Switch[mode]
if Function then
    -- code
end
1 Like
5 
local speedHolder = {
	[1] = 5,
	[2] = 70,
	[3] = 10,
	[4] = 16,
}

local function speedEvent(plr, action: string, mode: number)
	if action == states[3] then
		return moduleScript.updatePlayerHumanoid(char, "WalkSpeed", speedHolder[mode + 2])
	elseif action == states[7] then
		return moduleScript.updatePlayerHumanoid(char, "WalkSpeed", speedHolder[mode])
	end
end

This code uses a table and manipulates your mode value to retrieve data. Your if statements are condensed into one return function. I have not tested the code so you might need to update it slightly

1 Like