Code Review - Tower Placement

Hi! Thanks for stopping by and to people that will answer on this topic.
I am creating a tower placement and looking for feedback to optimize and improve the code. I’m still new to OOP.

Should I split the modules between client and server?
Is there anything that I can change to improve the code?
Is my code easy to read and understand?

Any tips on writing better comments and naming variables.

Any tips in general.

Thanks.

--// Variables
local TowerSotrage = workspace:WaitForChild("Towers")

local Physic = game:GetService("PhysicsService")
local collision = "Entities"
--// Services
local HTTP = game:GetService("HttpService")
local runservice = game:GetService("RunService")
local RS = game:GetService("ReplicatedStorage")
--// End

local Remotes = RS:WaitForChild("Remotes")
local Purchase = Remotes:WaitForChild("Tower")
local Config = RS:WaitForChild("Config")

--// Modules
local modules = RS:WaitForChild("Modules")
local Listener = require(script:WaitForChild("Listener"))
local Raycast = require(modules:WaitForChild("RayCast"))
--// End
--// FX Defaults
local offsetDefault = Vector3.new(0,1.6,0)
local canbePlace = Color3.new(0.137255, 1, 0.0431373)
local cantbePlace = Color3.new(1, 0, 0)

--// End

if runservice:IsClient() then
	Player = game:GetService("Players")
	LocalPlayer = Player.LocalPlayer
	Mouse = LocalPlayer:GetMouse()
	UIS = game:GetService("UserInputService")
	config = Config:WaitForChild(LocalPlayer.Name)
	Busy = config:WaitForChild("Busy")
end


local whitelist = {
	workspace.Ground
}


--[[
	
	Model = Tower
	Offset = Tower offset or it'll look like it's glitched under the ground
	
	Supports non-humanoid but not priority for now.

]]

--// Local functions ( Differentiate between humanoid and non-humanoid )
--// Check for tower placement
local function Verify(ground)
	for i,v in pairs(whitelist) do
		if v == ground then
			return true
		end
	end
	return false
end
local function IsAModel(model)
	if model:IsA("Model") then
		local primarypart = model.PrimaryPart
		if primarypart then
			return primarypart
		else
			error("PrimaryPart is missing")
		end
	end
	return false
end
local function getPart(model)
	local parts = {}
	if IsAModel(model) then
		for i,v in pairs(model:GetDescendants()) do
			if v:IsA("BasePart") then
				table.insert(parts, v)
			end
		end
	else
		table.insert(parts, model)
	end
	return parts
end
local function ForceField(model)
	local Parts = getPart(model)
	for i,v in pairs(Parts) do
		v.Material = Enum.Material.ForceField
	end
end
local function AdjustColor(model, Color)
	local Parts = getPart(model)
	for i,v in pairs(Parts) do
		v.Color = Color
	end
end
local function AdjustPosition(model, pos)
	local Type = typeof(pos)
	if Type == "Vector3" then
		pos = CFrame.new(pos)
	elseif Type ~= "CFrame" then
		error("Position must be a Vector or Cframe")
	end
	local Amodel = IsAModel(model)
	if Amodel then
		model:SetPrimaryPartCFrame(pos)
	else
		model.CFrame = pos
	end
end
local function GetPosition(model)
	local Amodel = IsAModel(model)
	if Amodel then
		return Amodel.Position
	else
		return model.Position
	end
end
--// End of Local Functions


--// OOP
local tower = {}
tower.__index = tower
tower.Server = {}


function tower.new(owner, model, pos, data, offset)	
	local newTower = {}
	setmetatable(newTower,tower)
	newTower.Tower = model:clone()
	newTower.Model = model
	newTower.Owner = owner or nil
	newTower.Offset = offset or offsetDefault
	newTower.IgnoreList = {}
	newTower.Data = data or {Damage = 10, Range = 20} -- Default tower data
	newTower.Position = pos or nil
	return newTower
end


--// Raycast mouse position
function tower:RayCast(Origin, End)

	--// Arguments are used by the server
	local origin = Origin or Mouse.UnitRay.Origin
	local End = End or Mouse.UnitRay.Direction * 50

	return Raycast.new(origin, End, self.IgnoreList)
end

--// Raycast ignorelists
function tower:AddIgnoreList(Additions)
	local IgnoreList = {workspace.CurrentCamera, self.Tower}

	--// Additions
	if Additions then
		if typeof(Additions) == "table" then
			for i,v in pairs(Additions) do
				table.insert(IgnoreList,Additions)
			end
		else
			error("Additions must be in table")
		end
	end
	--// Defaults \\--

	--// Ignore entities (humanoids)
	for i,v in pairs(workspace:GetDescendants()) do
		local hum = v:FindFirstChild("Humanoid")
		if hum then
			table.insert(IgnoreList, hum.Parent)
		end
	end

	--// Checks for enemies that doesn'have humanoid
	for i,v in pairs(TowerSotrage:GetChildren()) do
		--// Humanoids are already in ignorelist
		if not v:FindFirstChild("Humanoid") then
			table.insert(IgnoreList, v)
		end
	end

	--// Ignore Towers
	for i,v in pairs(TowerSotrage:GetChildren()) do
		table.insert(IgnoreList,v)
	end
	self.IgnoreList = IgnoreList
end


function tower:FilterCollisions()
	local Parts = getPart(self.Tower)
	for i,v in pairs(Parts) do
		--// Set collisions to "Entities" (Collide's to default only)
		Physic:SetPartCollisionGroup(v, collision)

		--// Can only run on server
		if not runservice:IsClient() then
			v:SetNetworkOwner(nil)
		end

	end
end

--// Self explanatory
function tower:Authorized(Player)
	if self.Owner == Player then
		return true
	end
end
function tower:Upgrade(Player)
	if self:Authorized(Player) then
		local Data = self.Data
		
		-- Temporary
		Data.Damage += 10
		Data.Range += 10
		print(Data,self)
	end
end
function tower:Delete(Player)
	if self:Authorized(Player) then
		self.Tower:Destroy()
		self = nil
	end
end
function tower:BuildConnections()
	local Upgrade = Instance.new("RemoteEvent", self.Tower)
	Upgrade.Name = "Upgrade"
	local Delete = Instance.new("RemoteEvent", self.Tower)
	Delete.Name = "Delete"
	
	Listener.RemoteEvent(self, Upgrade, "Upgrade")
	Listener.RemoteEvent(self, Delete, "Delete")
end
function tower:Destroy()
	if self.Tower then
		self.Tower:Destroy()
	end
	self = nil
end
--// END


--// To call if wanted to place a tower.
function tower:Place()
	local Tower = self.Tower
	Tower.Parent = TowerSotrage
	Tower.Name = HTTP:GenerateGUID(false) -- false to disable curly brackets
	--// Defaults \\--
	self:FilterCollisions()
	self:AddIgnoreList()


	if runservice:IsClient() then
		if Busy.Value then 
			self:Destroy()
			return warn("Player is still busy")
		end
		self.CanBePlace = false
		
		Busy.Value = true
		ForceField(Tower) -- FX

		local con
		con = Mouse.Button1Down:Connect(function()
			if self.CanBePlace then
				self.Position = GetPosition(Tower)
				runservice:UnbindFromRenderStep("Place")
				Purchase:FireServer(self.Model, self.Position)
				con:Disconnect()
				self:Destroy()
				
				Busy.Value = false 
			else
				warn("Cannot be place in here")
			end

		end)
		

		runservice:BindToRenderStep(
			"Place", 
			Enum.RenderPriority.Character.Value,
			function()
				local hit,pos = self:RayCast()
				if hit then
					local verify = Verify(hit)
					if verify then
						AdjustColor(Tower, canbePlace)
						self.CanBePlace = true
					else
						self.CanBePlace = false
						AdjustColor(Tower, cantbePlace)
					end
					local pos = pos + self.Offset
					AdjustPosition(Tower, pos)
				end
			end)

	else	-- Server
		
		local Position = self.Position
		local Amodel = IsAModel(Tower) 
		local Direction = -CFrame.new(Position).UpVector*50
		local hit,pos = self:RayCast(Position, Direction)
		local verify = Verify(hit)
		if verify then
			AdjustPosition(Tower, pos+self.Offset)
			self:BuildConnections()
			table.insert(tower.Server, self)
		else
			self:Destroy()
			warn("Cannot be place in here")
		end


	end
	
	

end



return tower

Here are a few things you can do:
Spell “TowerStorage” correctly,
Move your Towers instance to the ServerStorage
Abbreviate “PhysicsService” with “PS”
Abbreviate “RunService” with “RS”
Replace Color3.new with Color3.fromRGB
Add an “s” to “Player” to prevent confusion
Don’t have your configuration for every play in a folder in ReplicatedStorage
warn() instead of erroring out for the for issues

1 Like