Can these punch and punching dummy systems code be improved or optimized?

Help and Feedback Code Review
#1

So I have created a punch system which uses magnitude checks to deal damage and I have coded punch dummys to slowly regenerate or replace themselves if their HumanoidStateType is dead because they can’t slowly regenerate in that case, I would like someone to review my systems code and tell me if there’s anything that can be improved or optimized in the code.

It’d be appreciated if anyone tells me if there’s anything that can be improved or optimized in the code of these two systems!

Also what do y’all think about using magnitude checks to deal damage instead of using .Touched, Region3 and Raycasting to check if the player punched something?

1 Like
#2

There’s a few improvements that I think you can add to your scripts, here are all of them sorted by the script they relate to:

Punch system local script 
local plrChar = LocalPlayer.Character or LocalPlayer.CharacterAdded:wait()

:wait() is deprecated, you should use :Wait() instead (this only applies to the function on events, lowercase “wait(n)” is fine)

Punch system script

I think you’re using WaitForChild wayyyyy too much. WaitForChild should only be used if you aren’t certain that the object you’re trying to index is going to exist, from what I see you should only really be using WaitForChild in the LocalScript.

Why would you need to wait for the character to be added? You aren’t going to be punching while you’re dead :p

local plrChar = plr.Character or plr.CharacterAdded:wait()

Another issue I see is that you aren’t doing any sanity checks, an exploiter could spam the remote and instantly kill people. Implement a debounce/other sanity-check system to avoid this.

Punch dummy system script 
local BackupDummy = ReplicatedStorage:WaitForChild("BackupDummy")

If the client doesn’t use the BackupDummy, then you should be placing it in ServerStorage instead to reduce the amount of objects that need to be sent to the client (better for people with bad internet connections, and for your game’s security)

if DummyHumanoid.Health < 100 and BeingPunched.Value == false then
	DummyHumanoid.Health = DummyHumanoid.Health + 0.1			
end

Few changes I would make to this:

-- use the MaxHealth property instead of a fixed number, this could allow for dummies of differing health
if DummyHumanoid.Health < DummyHumanoid.MaxHealth and BeingPunched.Value == false then
	DummyHumanoid.Health += 0.1 -- compound assignment! (much cleaner imo, not required though)
end

if BeingPunched.Value == true then
	wait(5)
	BeingPunched.Value = false
end

Now this is a huge problem. If even a single dummy gets punched, the regeneration of ALL DUMMIES are halted for 5 entire seconds. You can solve this using delay()

if BeingPunched.Value == true then
	delay(5, function()
		BeingPunched.Value = false
	end)
end

Info on what delay() is (taken from the Roblox wiki, click on any of the hyperlinks to go there)

void delay ( number delayTime, function callback )
Schedules a function to be executed after delayTime seconds have passed, without yielding the current thread. This function allows multiple Lua threads to be executed in parallel from the same stack. The delay will have a minimum duration of 29 milliseconds, but this minimum may be higher depending on the target framerate and various throttling conditions. If the delayTime parameter is not specified, the minimum duration will be used.

Delay is commonly used to do stuff on a different thread after a set amount of time, if you want to learn more about them I recommend this really useful thread: Coroutines - When and how to use them

Here you edited out the script while I was writing this reply, so uhh that’s the end of this section I guess
Nevermind I’m just blind

If you have any questions, feel free to ask.

2 Likes
#3

Thank you for the constructive feedback, what is your opinion about the magnitude checks for the punch to deal damage instead of using .Touched, Region3 and Raycasting?

Are magnitude checks any good or are they bad and shouldn’t be used because they can cause problems?

1 Like
#4

I think magnitude checks are good as they require the least work to calculate, it should be good enough for your usage. Although, the only problem I can see with it is that you could punch someone through a wall if it was thin enough.

2 Likes