I’m still quite the noob in coding, but I think I’ve gotten much better then where I was a few months ago although I have an issue with having my scripts becoming really messy and confusing imo and this is I feel like due to my lack of knowledge as I usually code the way I feel makes sense so I usually end up with I guess weird methods and solutions?
for example here’s a script I recently rewrote
event.OnServerEvent:Connect(function(player, Action, Input, Fighter, Mouse, HMRPos, HMRCFrame)
local character = player.Character
if Action == "Combat" then
Combo = Combo..Input --add their input to the combo string which will be used to see if they input any combos
if moveData[Fighter].Moves[Input].type == "magic" then
Hitbox[moveData[Fighter].Moves[Input].type]()
elseif moveData[Fighter].Moves[Input].type == "projectile" then
Hitbox[moveData[Fighter].Moves[Input].type]()
elseif moveData[Fighter].Moves[Input].type == "physical" then
Hitbox[moveData[Fighter].Moves[Input].type](moveData[Fighter].Moves[Input].dmg, moveData[Fighter].Moves[Input].kb, nil, character, nil)
end
print(Combo)
local oldCombo = Combo
wait(0.5)
if Combo == oldCombo then
Combo = ""
print("Combo Reset")
else
if moveData[Fighter].Combos[Combo] then
local Track = Instance.new("Animation")
Track.AnimationId = "rbxassetid://"..moveData[Fighter].Combos[Combo].anim
local Anim = character.Humanoid:LoadAnimation(Track)
Anim:Play()
if moveData[Fighter].Combos[Combo].type == "magic" then
if moveData[Fighter].Combos[Combo].target == "self" then
--temp visual effect
local P = Instance.new("Part", workspace)
P.CanCollide = false
P.Transparency = 0.5
P.Size = moveData[Fighter].Combos[Combo].size
P.Anchored = true
P.CFrame = CFrame.new(HMRPos)
game.Debris:AddItem(P,0.5)
elseif moveData[Fighter].Combos[Combo].target == "place" then
--temp visual effect
local P = Instance.new("Part", workspace)
P.CanCollide = false
P.Transparency = 0.5
P.Size = moveData[Fighter].Combos[Combo].size
P.Anchored = true
P.CFrame = CFrame.new(Mouse)
game.Debris:AddItem(P,0.5)
end
elseif moveData[Fighter].Combos[Combo].type == "projectile" then
Hitbox[moveData[Fighter].Combos[Combo].type](player, HMRCFrame, Mouse, 20, character)
elseif moveData[Fighter].Combos[Combo].type == "physical" then
end
print("Combo")
Combo = ""
print("Combo Reset")
elseif string.len(Combo) >= 3 then
print("Invalid Combo")
Combo = ""
end
end
elseif Action == "Block" then
end
end)
I understand all of it and know how it works and it’s a lot better then my old script which was 200+ lines while this is only like 80 also I don’t really have a habit of making comments so oof.
Anyways any advice for making code organize would be helpful
edit: IMO I feel like this is probably considered messy, but I might be wrong and it’s probably not that bad?
It looks like you’d benefit most from getting to know when you should split things off into multiple functions and store things in variables.
You access moveData[Fighter].Moves[Input].type a lot at the start for example. You can store this in a variable. Also you have a couple branches with end up with the same code executing, so you can merge them. It might look something like:
local move = moveData[Fighter].Moves[Input]
if move.type == "magic" or move.type == "projectile" then
Hitbox[move.type]()
elseif move.type == "physical" then
Hitbox[move.type](move.dmg, move.kb, nil, character, nil)
end
It also looks like you might have a lot of different code depending on the Action variable itself, so I suggest splitting each branch into its own function that handles the specific logic.
Lastly, make sure that in the event that any value after player in the arguments list is spoofed, it won’t be an issue for your game. Thinking like an exploiter, it looks like it might be possible to mess with your code by spamming because there are no debounces. Additionally, since you give the client control over the action and such, an exploiter might be able to just set theirs to whichever does the most damage or whichever stat is desired.
The first argument to OnServerEvent (the Player) is the only argument that is guaranteed to be correct. Every argument after that is an argument that you manually pass, which means an exploiter can pass their own too.
So someone messing with your game might try something like passing a table where the Action string would be, which depending on your event logic might break or create some vulnerability. You just need to assume that any of these arguments can be spoofed, so you should never send any important data over it. Instead, handle that data in the server to begin with so the client has no say in it.
As for debouncing, I mean have some kind of time or length check to stop events from running their code when they are spammed and such. If you have a “Craft” event for example, and players can only craft once every minute, you would put a check on the server which makes sure the last time the player crafted was over a minute ago.
Hmm ok so here’s a local script which is just userinput and animations there’s probably a much better way I can do this, but with this you’re saying an exploiter can change what I send to the server? There’s probably obvioulsy a much better way I can do this so sorry if it’s bad lmao
if anims then
for i,animation in pairs(anims:GetChildren()) do
if animation:IsA("Animation") and animation.AnimationId ~= "" then
anim[animation.Name] = character.Humanoid:LoadAnimation(animation)
end
end
end
local controls = {
["E"] = function(Input)
anim.E:Play()
event:FireServer("Combat", Input, script.Name, mouse.hit.p, character.HumanoidRootPart.Position, character.HumanoidRootPart.CFrame)
end,
["Q"] = function(Input)
anim.Block:Play()
event:FireServer("Block", Input)
end,
}
UIS.InputBegan:Connect(function(Input, Game)
if Game then return end
if controls[UIS:GetStringForKeyCode(Input.KeyCode)] then
local key = UIS:GetStringForKeyCode(Input.KeyCode)
controls[UIS:GetStringForKeyCode(Input.KeyCode)](key)
end
end)
That’s alright. Though your code on the client can be improved a bit too. Apart from the storing of long accesses in variables you can also reduce some redundant code.
You don’t need to send character.HumanoidRootPart.Position and character.HumanoidRootPart.CFrame; if you just send the CFrame you can access its position.
But like I said, an user seems to be able to just spam E or Q (or emulate all sorts of parameters being sent such as the Action). I’m not sure if this is intended.
UIS.InputBegan:Connect(function(Input, Game)
local key = UIS:GetStringForKeyCode(Input.KeyCode)
if Game then return end
if controls[UIS:GetStringForKeyCode(Input.KeyCode)] and not debounces[key] then
debounces[key] = true
print("key pressed")
controls[UIS:GetStringForKeyCode(Input.KeyCode)](key)
wait(10)
debounces[key] = false
end
end)
That works, although you can just store the time instead so you aren’t having weird dangling waits. You then just compare the last time used to the current one.
Due to latency, it makes sense to have this on the client, but you also need to check on the server separately for security.
Also you localized key but are still recomputing it despite having the value still in the variable.