I need a code review and improvement for my gun system

Hello everyone I am making a gun system and I need a code review and if anyone can improve it a little bit just tell me!

Here is the code:

Client script:

local eq = false
local plr = game.Players.LocalPlayer
local mouse = plr:GetMouse()
local HitSound = game.SoundService:WaitForChild('Hit')
script.Parent.Equipped:Connect(function()
	eq = true
end)
script.Parent.Unequipped:Connect(function()
	
		eq = false

end)
local cooldown = false
local Damage = 30
local FireRate = 0.12
mouse.Button1Down:Connect(function()
	if eq == true then
		local Target1 = mouse.Target
		if cooldown == false then
			script.Parent.PlaySound:FireServer()
			if Target1 == nil then

				
				print('No Model')
				cooldown = true
				script.Parent.ShootEvent:FireServer()

				wait(FireRate)
				cooldown = false

			elseif Target1.Parent.Parent.ClassName == 'Model' then
				local Event = Target1.Parent.Parent:WaitForChild('Damage')
				Event:FireServer(Damage)
				cooldown = true
				script.Parent.ShootEvent:FireServer()
				print(Target1:GetFullName())

				if Target1.Parent.Parent:WaitForChild('Humanoid') then
					HitSound:Play()
				end


				wait(FireRate)
				cooldown = false

			elseif Target1.Parent.ClassName == 'Model' then
				local Event = Target1.Parent:WaitForChild('Damage')
				Event:FireServer(Damage)
				cooldown = true
				script.Parent.ShootEvent:FireServer()
				print(Target1:GetFullName())
				if Target1.Parent:WaitForChild('Humanoid') then
					HitSound:Play()
				end

				wait(FireRate)
				cooldown = false

				


			else
				print('No Model')
				cooldown = true
				script.Parent.ShootEvent:FireServer()

				wait(FireRate)
				cooldown = false

			end
		end

	end
	
end)

Server script:

local ShootEvent = script.Parent.ShootEvent
local PlaySound = script.Parent.PlaySound

local ShootSound = script.Parent.Flame:WaitForChild('FireSound')


local onReload = script.Parent:WaitForChild("OnReload")
local flame = script.Parent:WaitForChild('Flame')

local tool = script.Parent
local reloadAnim = tool:WaitForChild("Reload")

local ammo = script:WaitForChild("Ammo")
local maxammo = script.MaxAmmo.Value
local idleAnimTrack
local shootAnim = tool:WaitForChild("Shoot")
local idleAnim = tool:WaitForChild("Idle")

local firstEquipped = false

script.Parent.Equipped:Connect(function()
	local ammoGui = script.AmmoGui:Clone()
	ammoGui.AmmoStatus.Text = ammo.Value .. "/30"
	ammoGui.GunName.Text = tool.Name

	ammoGui.Parent = game.Players:GetPlayerFromCharacter(tool.Parent).PlayerGui
	if not idleAnimTrack then idleAnimTrack = tool.Parent.Humanoid:LoadAnimation(idleAnim) end

	if not idleAnimTrack.IsPlaying then idleAnimTrack:Play() end
end)
script.Parent.Unequipped:Connect(function()
	
	if idleAnimTrack and idleAnimTrack.IsPlaying then idleAnimTrack:Stop() end

end)

local fired = false
local firedTime = 0
local fireRate = .1
local isReloading = false
local reloadAnim = tool:WaitForChild("Reload")

local function reload()

	if isReloading or ammo.Value == 30 then return end
	isReloading = true



	local reloadAnimTrack = tool.Parent.Humanoid:LoadAnimation(reloadAnim)
	--idleAnimTrack:Stop()
	reloadAnimTrack:Play()
	wait(0.75)
	--reloadSound:Stop()
	--reloadSound.TimePosition = 0
	--idleAnimTrack:Play()

	ammo.Value = maxammo

	isReloading = false


end
local function FXon()
	for i, flashPart in pairs(flame:GetChildren()) do

		if flashPart.Name == 'FlashFX' then flashPart.Enabled = true end
		if flashPart.Name == 'FlashFXFlash' then flashPart.Enabled = true end
		if flashPart.Name == 'OverHeat' then flashPart.Enabled = true end
		if flashPart.Name == 'Smoke' then flashPart.Enabled = true end
		if flashPart.Name == 'Smoke2' then flashPart.Enabled = true end
	end
end

local function FXoff()
	for i, flashPart in pairs(flame:GetChildren()) do

		if flashPart.Name == 'FlashFX' then flashPart.Enabled = false end
		if flashPart.Name == 'FlashFXFlash' then flashPart.Enabled = false end
		if flashPart.Name == 'OverHeat' then flashPart.Enabled = false end
		if flashPart.Name == 'Smoke' then flashPart.Enabled = false end
		if flashPart.Name == 'Smoke2' then flashPart.Enabled = false end
	end
end
ShootEvent.OnServerEvent:Connect(function(player)
	if isReloading then return end
	if ammo.Value < 1 then

		reload()

		return
	end

	if game.Players:GetPlayerFromCharacter(script.Parent.Parent) ~= player then
		return
	end
	if player.Character:FindFirstChildOfClass("Humanoid").Health == 0 then
		return
	end
	

		fired = true
		--firedTime = tick()



		tool.Parent.Humanoid:LoadAnimation(shootAnim):Play()

		ammo.Value = ammo.Value - 1

	fired = false


	


end)
ShootEvent.OnServerEvent:Connect(function()
	FXon()
	wait(0.006)
	FXoff()
end)

PlaySound.OnServerEvent:Connect(function()
	ShootSound:Play()
end)

Damage receiver (in starter character script):

local Event = script.Parent:WaitForChild('Damage')

Event.OnServerEvent:Connect(function(plr, Damage)

local Humanoid = script.Parent:WaitForChild('Humanoid')

Humanoid:TakeDamage(Damage)

print(Damage)

end)

thank you in advance!

1 Like

also i forgot to say that the thing i made is a gun and it’s a tool not a viewmodel lol

Not my underqualified rear end doing code reviews LOL.

Overall, nice script. I quite like it.

I’ve noticed only one major problem at first glance. You add a lot of unnecessary lines of code by repeating yourself in your script.

For example, in your client script, you basically repeat the same thing twice, but targeted towards a different instance. You can use a function in this case and pass the instance in as a parameter and do the same thing with fewer lines of code.

What you have now:

elseif Target1.Parent.Parent.ClassName == 'Model' then
				local Event = Target1.Parent.Parent:WaitForChild('Damage')
				Event:FireServer(Damage)
				cooldown = true
				script.Parent.ShootEvent:FireServer()
				print(Target1:GetFullName())

				if Target1.Parent.Parent:WaitForChild('Humanoid') then
					HitSound:Play()
				end


				wait(FireRate)
				cooldown = false

			elseif Target1.Parent.ClassName == 'Model' then
				local Event = Target1.Parent:WaitForChild('Damage')
				Event:FireServer(Damage)
				cooldown = true
				script.Parent.ShootEvent:FireServer()
				print(Target1:GetFullName())
				if Target1.Parent:WaitForChild('Humanoid') then
					HitSound:Play()
				end

				wait(FireRate)
				cooldown = false

What I would recommend:

local function Fire(TargetPart, TargetCharacter)
    local Event = TargetCharacter:WaitForChild('Damage')
				Event:FireServer(Damage)
				cooldown = true
				script.Parent.ShootEvent:FireServer()
				print(TargetPart:GetFullName())
				if TargetPart.Parent:WaitForChild('Humanoid') then
					HitSound:Play()
				end

				wait(FireRate)
				cooldown = false
end

and then where the elseifs are:

elseif Target1.Parent:IsA("Model") then -- Instance:IsA("SomeClassName") is equivalent to Instance.ClassName == "SomeClassName", by the way.
    Fire(Target1, Target1.Parent)
elseif Target1.Parent.Parent:IsA("Model") then
    Fire(Target1, Target1.Parent.Parent)




Let’s look at another example. In your server script, you have two functions that do similar things, and in each of these functions, there is a series of lines that do the exact same thing, except for different parts.

Here’s what you have now:

local function FXon()
	for i, flashPart in pairs(flame:GetChildren()) do

		if flashPart.Name == 'FlashFX' then flashPart.Enabled = true end
		if flashPart.Name == 'FlashFXFlash' then flashPart.Enabled = true end
		if flashPart.Name == 'OverHeat' then flashPart.Enabled = true end
		if flashPart.Name == 'Smoke' then flashPart.Enabled = true end
		if flashPart.Name == 'Smoke2' then flashPart.Enabled = true end
	end
end

local function FXoff()
	for i, flashPart in pairs(flame:GetChildren()) do

		if flashPart.Name == 'FlashFX' then flashPart.Enabled = false end
		if flashPart.Name == 'FlashFXFlash' then flashPart.Enabled = false end
		if flashPart.Name == 'OverHeat' then flashPart.Enabled = false end
		if flashPart.Name == 'Smoke' then flashPart.Enabled = false end
		if flashPart.Name == 'Smoke2' then flashPart.Enabled = false end
	end
end

Let’s first combine these functions into one.

local function SetFX(Enabled)
    for i, flashPart in pairs(flame:GetChildren()) do
        if flashPart.Name == 'FlashFX' then flashPart.Enabled = Enabled end
		if flashPart.Name == 'FlashFXFlash' then flashPart.Enabled = Enabled end
		if flashPart.Name == 'OverHeat' then flashPart.Enabled = Enabled end
		if flashPart.Name == 'Smoke' then flashPart.Enabled = Enabled  end
		if flashPart.Name == 'Smoke2' then flashPart.Enabled = Enabled end
    end
end

Already, we’ve dropped around 10 lines of code and we are still able to achieve the same result. Now, assuming you don’t have anything else inside of flame other than the five instances whose properties you are changing, you can further drop lines of code by changing it to this:

The function to use if you don’t have anything else in flame:

local function SetFX(Enabled)
    for i, flashPart in pairs(flame:GetChildren()) do
        flashPart.Enabled = Enabled
    end
end

Now, if you do have other items inside of flame that you don’t want to change with this function, we have other options. For instance, we can put all of the instances that you do want to change into a folder, and loop through that folder instead of flame.

If you don’t want to do the folder thing, then try putting the names of the parts you want to change into a table, and use the table.find method to see if FlashPart is a part that you want to change.

The function to use if you do have other things in flame but you don’t want to put the FX items into a folder:

local FXNames = {"FlashFX", "FlashFXFlash", "OverHeat", "Smoke", "Smoke2"}
local function SetFX(Enabled)
    for i, FlashPart in pairs(flame:GetChildren()) do
        if table.find(FXNames, FlashPart.Name) then FlashPart.Enabled = Enabled
    end
end

Beware; if you have a ton of things inside of flame, then this function might be more computationally expensive then it needs to be. The table.find method will be looping through huge amounts of parts before the function ends. If this is the case, then let’s chop a few milliseconds off of our time to complete the loop. Consider the following:

The function to use if you have TONS of other things in flame:

local FXNames = {"FlashFX", "FlashFXFlash", "OverHeat", "Smoke", "Smoke2"}
local function SetFX(Enabled)
    local Throwaway = {} -- This will serve as a fake "part" that we can use if one of our parts doesn't exist. Spares us the use of an if statement.
    for i, FXName in pairs(FXNames) do
        (flame:FindFirstChild(FXName) or Throwaway).Enabled = Enabled
    end
end

The line in that for loop may look a little funky. Let's look at what's going on with that.

It’s just saying that we take either:

  1. The FX part we want, or
  2. If that isn’t inside of the part, then we take the “Throwaway” table that we made before,

and then with that value, we set the “Enabled” property to the “Enabled” parameter.


Once we've done all of that, we can change the
ShootEvent.OnServerEvent:Connect(function()
	FXon()
	wait(0.006)
	FXoff()
end)

to

ShootEvent.OnServerEvent:Connect(function()
	SetFX(true)
	wait(0.006)
	SetFX(false)
end)




That’s all I’ve got for you. Good job on the script! :slight_smile:

Well thank you right now i am on my phone i am gonna chek out tommorow! Thank you!’

You’re very welcome! :slight_smile: