Understanding script organization

I want my code to be more organized, and well written.

The issue is that something just feels weird about how im doing things and unorganized right now im trying to make a gun system and everything is in one module, i was looking at roblox open sourced weapons and they had multiple scripts like BaseWeapon, BulletWeapon and i was wondering if I should take that approach.

Here is the code if you wanted to see

local RunService = game:GetService("RunService")
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local ContextActionService = game:GetService("ContextActionService")
local Players = game:GetService("Players")

local PartCache = require(ReplicatedStorage.Packages.PartCache)

local player = Players.LocalPlayer
local character = player.Character or player.CharacterAdded:Wait()
local humanoid = character:WaitForChild("Humanoid")
local animator = humanoid.Animator

local bulletHole = ReplicatedStorage.Assets.Effects.HitMarks.BulletHole

local camera = workspace.CurrentCamera
local mouse = player:GetMouse()

local remote = ReplicatedStorage.Remotes.Weapon

local raycastParams = RaycastParams.new()
raycastParams.FilterType = Enum.RaycastFilterType.Exclude
raycastParams.FilterDescendantsInstances = {character, workspace.Debris}

local Weapon = {} do
	Weapon.__index = Weapon

	local function fireRay(self)
		local smokePart = self.tool.Model.SmokePart
		local rayOrigin = smokePart.Position
		local rayDirection = (mouse.Hit.Position - rayOrigin).Unit

		local raycastResult = workspace:Raycast(rayOrigin, rayDirection * self.range, raycastParams)
		
		if raycastResult then
			local flash = smokePart.FlashLight
			local flashFx = smokePart.FlashFX
			
			if flash and flashFx then
				task.spawn(function()
					flash.Enabled = true
					flashFx.Enabled = true
					
					task.wait(.05)
					
					flash.Enabled = false
					flashFx.Enabled = false
				end)
			end
			
			local hitmarker = self.cache:GetPart()
			hitmarker.CFrame = CFrame.new(raycastResult.Position, raycastResult.Position + raycastResult.Normal)
			hitmarker.BulletHole.Color3 = raycastResult.Instance.Color
			
			task.delay(3, function()
				self.cache:ReturnPart(hitmarker)
			end)
		end
	end

	local function weaponFired(self)
		remote:FireServer("Shot", mouse.Hit.Position, self.tool)
		fireRay(self)

		local recoilTrack = animator:LoadAnimation(self.tool.Animations.Recoil)
		recoilTrack:Play()
	end

	local function onActivated(self)
		if not self.tool:GetAttribute("IsLoaded") or self.tool:GetAttribute("CurrentMagAmmo") == 0 then print("load the weapon") return end

		self.tool:SetAttribute("Firing", true)

		if self.fireType ~= "Auto" and  self.tool:GetAttribute("Attachment") ~= "Switch" and os.clock() - self.lastShotFired > .2 then
			weaponFired(self)
			self.lastShotFired = os.clock()
		end
	end

	local function onDeactivated(self)
		if self.tool:GetAttribute("Attachment") == "Binary Trigger" and self.tool:GetAttribute("Firing") == true  then
			weaponFired(self)
		end

		self.tool:SetAttribute("Firing", false) 
	end

	local function onEquipped(self)
		--CameraSystem:Enable()
		self.hold = animator:LoadAnimation(self.tool.Animations.Hold)
		self.hold:Play()
		
		--[[local armWeld = Instance.new("Weld")
		armWeld.Part0 = character.UpperTorso
		armWeld.Part1 = character.RightUpperArm
		armWeld.Parent = character
		
		RunService.Heartbeat:Connect(function()
			local cframe = CFrame.new(character.UpperTorso.Position, mouse.Hit.Position) * CFrame.Angles(math.pi/2, 0, 0)
			armWeld.C0 = armOffset * character.UpperTorso.CFrame:toObjectSpace(cframe)
		end)]]

		mouse.Icon = "http://www.roblox.com/asset/?id=14621120315"
	end

	local function onUnequipped(self)
		--CameraSystem:Disable()
		self.hold:Stop()

		if self.tool:GetAttribute("Firing") == true then
			self.tool:SetAttribute("Firing", false)
		end
	end

	local function loadWeapon(tool)
		remote:FireServer("LoadWeapon", tool)
		print("weapon loaded")
	end

	local function onHeartbeat(self, deltaTime)
		if self.tool:GetAttribute("Firing") == true and self.tool:GetAttribute("Attachment") == "Switch" or self.fireType == "Auto" then
			if os.clock()- self.lastShotFired >= .05 then 
				weaponFired(self)
				self.lastShotFired = os.clock()
			end
		end
	end

	local function onRenderStepped(deltaTime)
		local recoil = spring:update(deltaTime) do
			camera.CFrame = camera.CFrame * CFrame.Angles(recoil.x, 0, 0)
		end
	end

	function Weapon.new(instance)
		local self = setmetatable({
			tool = instance,
			fireType = instance:GetAttribute("FireType"),
			range = instance:GetAttribute("Range"),
			lastShotFired = os.clock(),
			
			connections = {}
		}, Weapon)
		
		return self
	end

	function Weapon:setup()
		self.cache = PartCache.new(ReplicatedStorage.Assets.Effects.HitMarks.BulletHole, 20)
		self.cache:SetCacheParent(workspace.Debris)
		
		if not self.tool:GetAttribute("Firing") then
			self.tool:SetAttribute("Firing", false)
		end

		if not table.find(raycastParams.FilterDescendantsInstances, self.tool) then
			raycastParams:AddToFilter(self.tool)
		end

		self.connections.activated = self.tool.Activated:Connect(function() onActivated(self) end)
		self.connections.deactivated = self.tool.Deactivated:Connect(function() onDeactivated(self) end)
		self.connections.equipped = self.tool.Equipped:Connect(function() onEquipped(self) end)
		self.connections.unequipped = self.tool.Unequipped:Connect(function() onUnequipped(self) end)
		self.connections.heartbeat = RunService.Heartbeat:Connect(function(deltaTime) onHeartbeat(self, deltaTime) end)

		RunService:BindToRenderStep("Recoil", Enum.RenderPriority.Camera.Value - 1, onRenderStepped)
		ContextActionService:BindAction("Load", function() loadWeapon(self.tool) end, false, Enum.KeyCode.F)
	end

	function Weapon:remove()
		for _, connection in self.connections do
			connection:Disconnect()
		end
		
		self.connections = nil
		self.tool = nil
		self.lastShotFired = nil
		
		setmetatable(self, nil); self = nil
	end
end

return Weapon

should this be in the code review section?

For me the best way is to use comments. For exemple if I start a new script I will write all the thing I want in it in comments. Then write the tables.

--Game Services

--Tables

--Player

--script Goal or Actions

Etc… Then just classify you things with these title and you should be able to write ur code without being worried about organizations. However your script already looks clean.

Ok, should i have multiple scripts though for the weapon system? that handle different parts or just have everything in one module

Honestly I don’t use modules a lot so I’m not the best in it but modules can store lots of function. If you don’t want to get lost you can either use comments to separate your actions, or create other modules with different names. But both way work and since modules don’t work on their own I don’t think it’ll affect the gameā€˜s performance very much.

Ye, I know I dont gotta worry about performance its just organization

I mean it would be cleaner when using multiple modules for specific functions.

1 Like

for what i do when making something i would have two module scripts, one for the server one for the client

put my services at the top
followed by constants
followed by global variables
followed by local functions
followed by module functions
finally ending with init functions

i do this for systems like guns. but for other things i would have a module script thats responsible for doing one job (still using the same order of services, constants, etch

(i use the Knit framework)
for example: ā€œMoneyServiceā€ handles the money in the game, ā€œCharacterServiceā€ handles the characters, you get it

i try not to be vague like ā€œGameServiceā€ since that could do anything

same stuff on the client

in my opinion making a module script for only one function is stupid. you should have classes that hold a bunch of functions. if you have useful functions that are used everywhere put all of those in one module and call it something like Methods

for tables and data storage, i do put each of them in their own module. like WeaponSettings goes in its own module

ok, ye i figured out what im gonna do i preciate the info from all yall

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