Gun System Code review

JAcoboiskaka1121 · November 18, 2024, 6:13pm

Heya!

So, I was trying to get a scripter role in the hiddenDevs discord so I can get more commissions. The problem is, that the requirements are quite long, Here they are:

I usually split my code into different modules, but writing one script with 200 lines of code in one script?? So I decided to make a gun system, I uploaded it, provided a demo game(which they haven’t even played, the visit count is 0).

So I locked in, and wrote 200 lines of code.

Then the results got in, and I was rejected.

I thought I got everything, I even put comments on each line, But I guess not.

I guess using raycasting is a deprecated API now…

Anyway, heres the code:

Code

--Script wrote by JAcoboiskaka1121

local Tool = script.Parent--Get the tool

local Player = game.Players.LocalPlayer--Get the player
local Mouse = Player:GetMouse()--Get the players mouse

local GUI = Player.PlayerGui:WaitForChild("MainGUI")--Get the GUI
local Textlabel = GUI:FindFirstChild("AmmoCounter")--Get the Ammo Counter
local ContextActionServ = game:GetService("ContextActionService")-- Get Context Action Service(for reloading)
local Debris = game:GetService("Debris")--Get Debris Service(for destroying parts)
local TweenService = game:GetService("TweenService")--Get Tween Service(for smooth animations)
local RunService = game:GetService("RunService") -- get RunService(for continuous checking)

local CONFIG = {--configuration table
	PROJECTILE = {--projectile settings
		SIZE = Vector3.new(0.5,0.5,0.5),
		COLOR = Color3.fromRGB(255, 255, 0),
		SPEED = 90,
		TRAIL_LIFETIME = 1
	},
	STATES = {--states
		CAN_SHOOT = true,
		RELOADING = false,
		AMMO = script.Parent.Ammo
	},
	DAMAGE = {-- damage settings
		BASE_DAMAGE = 50,
	},
	EFFECTS = {-- effects
		HIT_EFFECT_LIFETIME = 1
	}
}

local STATS = {-- statistics
	KILL_COUNT = 0,
	SHOTS_FIRED = 0,
	RELOADS = 0,
	TOTAL_DAMAGE_DEALT = 0
}

local GUI_ELEMENTS = {-- gui elements
	AMMO_COUNTER = GUI.AmmoCounter,
	KILL_COUNTER = GUI.KillCounter,
	SHOTS_FIRED = GUI.ShotsFired,
	RELOADS = GUI.Reloads,
	TOTAL_DAMAGE = GUI.TotalDamage
}

CONFIG.STATES.AMMO.Changed:Connect(function(value)--when ammo amount is changed
	Textlabel.Text = value-- set the ammo amount in the text box
	if CONFIG.STATES.AMMO.Value < 5 then-- if the ammo amount is less than 5
	Textlabel.TextColor3 = Color3.fromRGB(255,0,0)-- set the color to red
	else-- if more than 5 then
	Textlabel.TextColor3 = Color3.fromRGB(255,255,255)-- set the color to white
	end
end)

local connection2 = RunService.RenderStepped:Connect(function()-- continually check and change the different statistics
	GUI_ELEMENTS.KILL_COUNTER.Text = "Kills: "..STATS.KILL_COUNT
	GUI_ELEMENTS.SHOTS_FIRED.Text = "Shots Fired: "..STATS.SHOTS_FIRED
	GUI_ELEMENTS.RELOADS.Text = "Reloads: "..STATS.RELOADS
	GUI_ELEMENTS.TOTAL_DAMAGE.Text = "Total Damage: "..STATS.TOTAL_DAMAGE_DEALT
end)

function Reload(actionname, inputstate, inputobject)-- make function to reload
	if CONFIG.STATES.RELOADING then return end-- if is reloading then stop
	if inputstate == Enum.UserInputState.Begin then-- when the input is began
		CONFIG.STATES.CAN_SHOOT = false-- set cant shoot(cant reload and shoot at the same time)
		CONFIG.STATES.RELOADING = true-- set reloading state
		script.Parent["Pistol Reload"]:Play()--play reloading sound
		STATS.RELOADS += 1-- change statistics
		task.wait(2)-- wait 2 seconds
		CONFIG.STATES.AMMO.Value = 12-- revert back 
		CONFIG.STATES.CAN_SHOOT = true-- set can shoot state back to true
		CONFIG.STATES.RELOADING = false-- set reloading state to false
	end
end

ContextActionServ:BindAction("Reload", Reload, false, Enum.KeyCode.R)-- When R is pressed, call reload function

local ActiveTracers = {}--make table for active tracers

local function CreateTracer(startPos, endPos)-- Make function for creating tracers
	local distance = (endPos - startPos).Magnitude--get distance
	local midPoint = (startPos + endPos) / 2--get mid point
	local direction = (endPos - startPos).Unit--get direction

	local tracer = Instance.new("Part")--make a new tracer
	tracer.Name = "Tracer"--name it tracer
	tracer.Size = Vector3.new(0.25,0.25, distance)-- set tracer size
	tracer.CFrame = CFrame.new(midPoint, endPos)--set tracer position
	tracer.Anchored = true--set tracer to anchored
	tracer.CanCollide = false-- set tracer collide to false
	tracer.Material = Enum.Material.Neon-- set material to neon(for a slight bloom)
	tracer.Color = CONFIG.PROJECTILE.COLOR-- set color
	tracer.Parent = workspace-- set parent to workspace

	table.insert(ActiveTracers, tracer)-- insert tracer to table

	local tween = TweenService:Create(tracer, TweenInfo.new(CONFIG.PROJECTILE.TRAIL_LIFETIME), {-- animate tracer fade
		Transparency = 1
	})
	tween:Play()-- play the animation

	Debris:AddItem(tracer, CONFIG.PROJECTILE.TRAIL_LIFETIME)--Destroy tracer after time
	
	task.defer(function()
		task.wait(CONFIG.PROJECTILE.TRAIL_LIFETIME)
		local index = table.find(ActiveTracers, tracer)
		if index then
			table.remove(ActiveTracers, index)
		end
	end)

	return tracer-- return tracer for more control
end

local function HandleProjectile(origin, direction, raycastParams)-- make function to handle projectile
	local currentPos = origin-- get the origin
	local active = true-- set the active variable

	local connection
	connection = RunService.Heartbeat:Connect(function(deltaTime)-- run every heartbeat(every frame after physics simulation)
		if not active then return end--if not active then stop the function

		local movement = direction * CONFIG.PROJECTILE.SPEED * deltaTime -- multiply the direction with the speed and delta time for a consistent result
		local newPos = currentPos + movement -- calculate new position by getting old position and calculating movement

		local ray = workspace:Raycast(currentPos, movement, raycastParams)--shoot the ray cast
		
		if ray then-- if the ray hit something
			
			CreateTracer(origin, ray.Position)-- make a tracer

			local hitPart = ray.Instance-- get the instance that the ray hit
			local humanoid = hitPart.Parent:FindFirstChildOfClass("Humanoid")-- get the humanoid from the instance

			if humanoid then-- if the humanoid is found
				humanoid:TakeDamage(CONFIG.DAMAGE.BASE_DAMAGE)-- take damage to the humanoid

				if humanoid.Health == 0 then-- if the humanoid is dead
					STATS.KILL_COUNT += 1-- add it to the kill count
				end

				STATS.TOTAL_DAMAGE_DEALT += CONFIG.DAMAGE.BASE_DAMAGE-- add damage to the total damage dealt
				active = false-- set active to false 
				connection:Disconnect()-- disconnect the HeartBeat function
				return
			end
			--if not a humanoid
			active = false--set active to false
			connection:Disconnect()--disconnect the function
			return-- stop the function
		else
			--if ray didnt hit anything
			local tracerEndPos = origin + direction*100-- calculate the end position of the tracer
			CreateTracer(currentPos, tracerEndPos)-- create the tracer
		end

		currentPos = newPos-- set the current position to the new position
	end)
end

Tool.Activated:Connect(function()-- when the gun is shot
	if CONFIG.STATES.AMMO.Value <= 0 then-- if there is no ammo
		script.Parent["Ammo Magazine 3 (SFX)"]:Play()-- play empty magazine sound
		CONFIG.STATES.CAN_SHOOT = false-- turn off shooting
		return-- stop the function
	end
	
	if not CONFIG.STATES.CAN_SHOOT then return end-- if cant shoot, stop the function
	
	local Origin = Tool.MeshPart.Origin.WorldPosition--get the Origin position
	local Direction = (Mouse.Hit.Position - Origin).Unit--get the direction shot
	
	CONFIG.STATES.AMMO.Value -= 1--subtract ammo
	CONFIG.STATES.CAN_SHOOT = true--turn on shooting
	STATS.SHOTS_FIRED += 1-- add 1 to shots fired
	
	local Params = RaycastParams.new()-- make new ray cast parameters
	Params.FilterType = Enum.RaycastFilterType.Exclude-- set filter type to exclude(can detect all except the filter list)
	
	local filterList = {Tool, Tool.Handle, Tool.MeshPart, Player.Character}-- exclude the tool, and the local players character
	
	for _, tracer in ipairs(ActiveTracers) do-- add all active tracers to the filter list
		table.insert(filterList, tracer)
	end
	Params.FilterDescendantsInstances = filterList-- ignore the instances in the filter list
	Params.IgnoreWater = true-- will ignore water 

	script.Parent["Gun Shot"]:Play()-- play the gun shot sound
	
	HandleProjectile(Origin, Direction*100,Params)-- fire the gun
end)

Tool.Equipped:Connect(function()-- when tool equipped
	for i,v in pairs(GUI_ELEMENTS) do-- loop through gui elements
		v.Visible = true--make them visible
	end
end)

Tool.Unequipped:Connect(function()--when tool unequipped
	for i,v in pairs(GUI_ELEMENTS) do--loop through gui elements
		v.Visible = false--hide them
	end
end)

Heres the game I submitted:

Tell me what I could have improved on to not get rejected.

Any help is appreciated!

12345koip · November 18, 2024, 7:18pm

Yup… that’s really hard to read.

Remember that too much commenting is just as bad as too little. It makes your code too overcrowded, it’s hard to tell what’s a comment and what isn’t. Also try to distance comment from code with spaces.

Some comments aren’t in-depth enough.

What’s SIZE? Is it the size of the bullet? The size of the hitbox? How are we to know?
How is SPEED used? What does it control? What about TRAIL_LIFETIME?

Skellyton:

if inputstate == Enum.UserInputState.Begin then-- when the input is began
		CONFIG.STATES.CAN_SHOOT = false-- set cant shoot(cant reload and shoot at the same time)
		CONFIG.STATES.RELOADING = true-- set reloading state
		script.Parent["Pistol Reload"]:Play()--play reloading sound
		STATS.RELOADS += 1-- change statistics
		task.wait(2)-- wait 2 seconds
		CONFIG.STATES.AMMO.Value = 12-- revert back 
		CONFIG.STATES.CAN_SHOOT = true-- set can shoot state back to true
		CONFIG.STATES.RELOADING = false-- set reloading state to false
	end

The same issue goes here. It would be much better in a format like this:

--function to handle a reload request from the player
local function Reload(actionName: string, inputState: Enum.UserInputState, inputObject: InputObject): ()
    --if reloading is in progress, don't start another reload
    if CONFIG.STATES.RELOADING then return end

    if inputstate == Enum.UserInputState.Begin then --if it was the start of the click, like mouse button down#

        --disable shooting and indicate reloading is in progress
        CONFIG.STATES.CAN_SHOOT = false
        CONFIG.STATES.RELOADING = true

        --play the sound and increment the reload counter
        script.Parent["Pistol Reload"]:Play()
        STATS.RELOADS += 1
        task.wait(2) --wait 2 seconds so reloading isn't instant

        --reset the ammo, allow shooting, and indicate reloading is no longer in progress
        CONFIG.STATES.AMMO.Value = 12
        CONFIG.STATES.CAN_SHOOT = true
        CONFIG.STATES.Reloading = false
    end
end

Your code is also a bit all over the place. I'm seeing references, to settings, to functions, to function calls, to more references, to more function calls. Try to keep your code in a neat, readable format.

Like this:

Services and requiring modules
Constants and variables, grouped (if function calls needed for these, do it)
Subprograms
Main code and function calls (excluding connections)
RBXScriptConnections

Like this:

--services
local players = game:GetService("Players")

--parts in the workspace
local part1 = workspace:FindFirstChild("Part1")
local part2 = workspace:FindFirstChild("Part2")

--the random number generator and min, max values for it
local generator = Random.new()
local min = 3
local max = 5


--when the part is touched
local function onTouch(hit: BasePart): ()
    --attempt to get the player in order to check for a character
    local model = hit:FindFirstAncestorOfClass("Model")
    local player = players:GetPlayerFromCharacter(model)

    if player then --if it was a player that walked on the part (player is not nil)
        --generate a random number and output it
        local num = generator:NextInteger(min, max)
        print(num)
    end
end

--connections
part1.Touched:Connect(onTouch) --run onTouch function when the part is touched

As for the deprecated API part, the Mouse object is not recommended for new work and is only updated due to it’s already widespread use. Use UserInputService instead.

JAcoboiskaka1121 · November 18, 2024, 7:24pm

Ill probably rewrite everything with OOP, maybe that will change something.

12345koip · November 18, 2024, 7:25pm

OOP would certainly help to simplify the code and make it more readable. Just remember these key formatting things. It also ticks the box of metatables, which I saw on your screenshot of application criteria.

system · December 2, 2024, 7:26pm

This topic was automatically closed 14 days after the last reply. New replies are no longer allowed.