Is my fishing rod script good or not?

Help and Feedback Code Review
#1

Hello, I just finished the biggest script I’ve made, 213 lines of code. I’m wondering what are your thoughts on it and how can I improve my code. 213 lines is big for me but is it above 200 because I made some things overcomplicated? Here’s the script:

  • Main script in the fishing rod
local tool = script.Parent
local UserInputService = game:GetService("UserInputService")
local Players = game:GetService("Players")
local RepSto = game:GetService("ReplicatedStorage")
local TwSer = game:GetService("TweenService")
local fishGui = Players.LocalPlayer.PlayerGui.epicFISHING
local noFish = Players.LocalPlayer.PlayerGui.NoFish
local DaGui = Players.LocalPlayer.PlayerGui.BULLSHARK

local IsUsingRod = false
local IDK = false

local MAX_MOUSE_DISTANCE = 110
local MAX_ROD_DISTANCE = 100
local force

local TwInfo = TweenInfo.new(1.5)
local Tween = TwSer:Create(DaGui.Frame.ImageLabel, TwInfo, {Position = UDim2.new(.945,0,0,0)})

local fish
local hitPosition

local function getWorldMousePosition()
	local mouseLocation = UserInputService:GetMouseLocation()
	local screenToWorldRay = workspace.CurrentCamera:ViewportPointToRay(mouseLocation.X, mouseLocation.Y)
	local directionVector = screenToWorldRay.Direction * MAX_MOUSE_DISTANCE
	local weaponRaycastParams = RaycastParams.new()
	weaponRaycastParams.FilterDescendantsInstances = {Players.LocalPlayer.Character}
	local raycastResult = workspace:Raycast(screenToWorldRay.Origin, directionVector, weaponRaycastParams)
	if raycastResult then
		return raycastResult.Position
	end
end

local function GetRandomFish()
	if force <= 25 then
		return RepSto:FindFirstChild("Basic fish")
	elseif force <= 30 then
		return RepSto:FindFirstChild("Rare fish")
	elseif force <= 34 then
		return RepSto:FindFirstChild("Shiny fish")
	elseif force <= 38 then
		return RepSto:FindFirstChild("Baby mako")
	elseif force <= 40 then
		return RepSto.Bananalligator
	else
	    return
	end
end

local function UseRod()
	local mouseLocation = getWorldMousePosition()
	if mouseLocation then
		local targetDirection = (mouseLocation - tool.Handle.Position).Unit
		local directionVector = targetDirection * MAX_ROD_DISTANCE
		local weaponRaycastParams = RaycastParams.new()
		weaponRaycastParams.FilterDescendantsInstances = {Players.LocalPlayer.Character}
		local weaponRaycastResult = workspace:Raycast(tool.Handle.Position, directionVector, weaponRaycastParams)
		if weaponRaycastResult then
			hitPosition = weaponRaycastResult.Position
			local Material = weaponRaycastResult.Material
			if Material == Enum.Material.Water and hitPosition and Players.LocalPlayer:GetAttribute("Baits") >= 1 and IsUsingRod == false then
				IsUsingRod = true
				force = math.random(10, 41)
				Players.LocalPlayer:SetAttribute("Baits", Players.LocalPlayer:GetAttribute("Baits") - 1)
				local Animator = Players.LocalPlayer.Character.Humanoid.Animator
				local Swing = Instance.new("Animation")
				Swing.AnimationId = "rbxassetid://12646581867"
				local SwingTrack = Animator:LoadAnimation(Swing)
				SwingTrack.Priority = Enum.AnimationPriority.Action
				SwingTrack.Looped = false
				SwingTrack:Play()
				SwingTrack:AdjustSpeed(.35)
				Players.LocalPlayer.Character.Humanoid.WalkSpeed = 0
				game.ReplicatedStorage.FISHING:FireServer(hitPosition)
				wait(5)
				if force >= 20 then
					fishGui.Force.Text = ("Fish's force: "..force)
					fishGui.Enabled = true
				else
					noFish.Enabled = true
				end
				game.ReplicatedStorage.FISHING.OnClientEvent:Wait()
				noFish.Enabled = false
				fishGui.Enabled = false
				Players.LocalPlayer.Character.Humanoid.WalkSpeed = 16
				IsUsingRod = false
			end
		end
	end
end

tool.Activated:Connect(UseRod)

local function Fight()
	local PosOfDaGui = DaGui:WaitForChild("Frame").ImageLabel.Position
	if PosOfDaGui.X.Scale >= .41 and PosOfDaGui.X.Scale <= .51 then
		RepSto.BullSharkFished:FireServer(DaGui)
	else
		RepSto.BullSharkFished:FireServer(Players.LocalPlayer.Character)
	end
	return
end

fishGui.Force.Activated:Connect(function()
	if IDK == false then
		IDK = true
		fish = GetRandomFish()
	end
	force = force - 1
	fishGui.Force.Text = ("Fish's force: "..force)
	if force == 0 then
		fishGui.Enabled = false
		if fish then
			RepSto.BoughtSomething:FireServer(fish)
		else
			local Anim = Instance.new("Animation")
			Anim.AnimationId = "rbxassetid://12892091586"
			local AnimTrack = Players.LocalPlayer.Character.Humanoid.Animator:LoadAnimation(Anim)
			AnimTrack.Priority = Enum.AnimationPriority.Action2
			AnimTrack.Looped = false
			if tool then
			script.Parent = Players.LocalPlayer.PlayerScripts
			tool:Destroy()
			end
			game.Workspace.CurrentCamera.CameraType = Enum.CameraType.Scriptable
			AnimTrack:Play()
			AnimTrack:AdjustSpeed(.45)
			AnimTrack.Stopped:Wait()
			game.Workspace.CurrentCamera.CameraType = Enum.CameraType.Custom
			Players.LocalPlayer.PlayerGui.BlackScreen.Enabled = true
			game.SoundService.WaterSplash:Play()
			local place = Vector3.new(54.342, -21.323, -96.222)
			RepSto.BullSharkFished:FireServer(nil,place)
			wait(3)
			Players.LocalPlayer.PlayerGui.BlackScreen.Enabled = false
			local HasChosenDialog = false
			while HasChosenDialog == false do
				game.Workspace.Gorilla.Head.Help.DialogChoiceSelected:Connect(function(player, dialog)
					if player == Players.LocalPlayer and dialog.Name == "ItComes" then
						HasChosenDialog = true
					end
				end)
				wait(.1)
			end
			wait(3)
			while true do
				local Shark = RepSto.BullShark:Clone()
				Shark.Parent = game.Workspace
				local STween = TwSer:Create(Shark, TweenInfo.new(1.5),{Position = Players.LocalPlayer.Character.HumanoidRootPart.Position})
				STween:Play()
				DaGui.Enabled = true
				Tween:Play()
				Tween.Completed:Wait()
			    if DaGui.Enabled == true then	
			        DaGui.Enabled = false
					Fight()
				end
				Shark:Destroy()
				DaGui.Frame.ImageLabel.Position = UDim2.new(0,0,0,0)
				HasChosenDialog = true
				RepSto.BullSharkFished.OnClientEvent:Connect(function()
					script:Destroy()
				end)
				wait(math.random(3,6))
			end
		end
		IDK = false
	end
end)

local function CheckInput(Input)
	local Animate = false
	if DaGui.Enabled == true then
		if Input:IsA("InputObject") then
			if Input.KeyCode == Enum.KeyCode.E then
				Animate = true
			elseif Input.KeyCode == Enum.KeyCode.ButtonX then
				Animate = true
			end
		else
			Animate = true
		end
	end
	if Animate == true then
		DaGui.Enabled = false
		local Animator = Players.LocalPlayer.Character.Humanoid.Animator
		local animation = Instance.new("Animation")
		animation.AnimationId = "rbxassetid://12779583215"
		local animationtrack = Animator:LoadAnimation(animation)
		animationtrack:Play()
		animationtrack.Stopped:Wait()
		Fight()
	end
	Animate = false
end

UserInputService.TouchTap:Connect(CheckInput)
UserInputService.InputBegan:Connect(CheckInput)

game:GetService("RunService").Heartbeat:Connect(function()
	if workspace.CurrentCamera.CameraType == Enum.CameraType.Scriptable then
		workspace.CurrentCamera.CFrame = Players.LocalPlayer.Character.Head.CFrame * CFrame.new(0,0,-.5)
	end
end)

Players.LocalPlayer.Character.Humanoid.Died:Connect(function()
	local Shark = game.Workspace:FindFirstChild("BullShark")
	if Shark then
		Shark:Destroy()
	end
	script:Destroy()
end)

*The other script, was originally 2 scripts but I fused them together

local Event = game.ReplicatedStorage.BullSharkFished

local Punchs = 0

Event.OnServerEvent:Connect(function(player, Variable, place)
	if not Variable then
		player.Character.HumanoidRootPart.Position = place
	else
		if Variable:IsA("Model") then
			if Variable:FindFirstChild("LeftFoot") then
				Variable.LeftFoot:Destroy()
				Variable.LeftUpperLeg:Destroy()
				Variable.LeftLowerLeg:Destroy()
			elseif Variable:FindFirstChild("RightFoot") then
				Variable.RightFoot:Destroy()
				Variable.RightUpperLeg:Destroy()
				Variable.RightLowerLeg:Destroy()
			elseif Variable:FindFirstChild("LeftHand") then
				Variable.LeftHand:Destroy()
				Variable.LeftUpperArm:Destroy()
				Variable.LeftLowerArm:Destroy()
			elseif Variable:FindFirstChild("RightHand") then
				Variable.RightHand:Destroy()
				Variable.RightUpperArm:Destroy()	
				Variable.RightLowerArm:Destroy()	
			else
				Variable.Head:Destroy()
			end
	    else
		    Punchs += 1
		    if Punchs == 3 then
			    Punchs = 0
				Event:FireClient(player)
				local Shark = game.ReplicatedStorage.BullSharkTool:Clone()
				Shark.Name = "Bull shark"
				Shark.Parent = player.Backpack
			end
		end
	end
end)

local Players = game:GetService("Players")
local UserInputService = game:GetService("UserInputService")
local TweenSer = game:GetService("TweenService")
local Event2 = game.ReplicatedStorage.FISHING

Event2.OnServerEvent:Connect(function(player, GoalPos)
	local Rod = player.Backpack:FindFirstChild("Fishing rod")
	if not Rod then
		Rod = player.Character:FindFirstChild("Fishing rod")
	end
	wait(1)
	Rod.FishHook.Ball.Weld.Enabled = false
	local TwIn = TweenInfo.new(1)
	local goal = {CFrame = CFrame.new(GoalPos)}
	local Tween = TweenSer:Create(Rod.FishHook.Ball, TwIn, goal)
	Tween:Play()
	Tween.Completed:Wait()
	wait(8)
	Rod.FishHook:PivotTo(Rod.Handle.CFrame * CFrame.new(0,-1,0))
	Rod.FishHook.Ball.Weld.Enabled = true
	Event2:FireClient(player)
end)

Thanks for reading.

1 Like
#2

looks pretty good but it would be better if you used task.wait() instead of wait it’s newer and more reliable i never use wait because it’s outdated and frowned upon among experienced coders its also
considered legacy so i prefer not to use it but to each his own

examples in common types of loops
if you read this comment and find it helpful please favourite it

while task.wait(.5) do
–your code
end

repeat task.wait(.1)
–your code
until nil

#3

Where’s the WaitForChild? Also, why are you not storing a reference to the player?

You’re loading this multiple times, which causes a memory leak.

:Wait is not very reliable, in my opinion. You should put in the wait yourself.

I think this is a typo.

This variable name could be more specific, like CharacterOrNil.

You could possibly organize this into a table of things to destroy.

This variable is added to any time any player kills them, so that the problem is that any player could kill get 2 punches and then the other 2nd player could do one punch and get the kill.

As said above, use task.wait instead of wait for your use case. Though I wonder if anyone here knows how wait could possibly be better than task.wait…

1 Like
#4

Thanks for writing a well written reply to my post I fixed most of the problems you pointed in my script and fixed another while fixing them. About your idea of a table storing the things to destroy, I’ve tried but I quickly realised that would mean making for loops in the script which would make the script longer and less efficient. Also what would you propose as a replacement for :Wait()?

#5

I would use :Once.

Actually, it will make the script shorter. Although it might not be as performant, it would be much more readable, which is worth the cost. However, it is not necessary.

#6

I replaced every Wait() with Once(). I prefer performance over readability so I’m not going to use tables, I never actually used one before. I’ll keep this post open until the update it’s related to comes out. here’s my scripts so far:

*the one in the fishing rod

local tool = script.Parent
local UserInputService = game:GetService("UserInputService")
local Players = game:GetService("Players")
local RepSto = game:GetService("ReplicatedStorage")
local TwSer = game:GetService("TweenService")
local fishGui = Players.LocalPlayer.PlayerGui.epicFISHING
local noFish = Players.LocalPlayer.PlayerGui.NoFish
local DaGui = Players.LocalPlayer.PlayerGui.BULLSHARK

local IsUsingRod = false
local IDK = false

local MAX_MOUSE_DISTANCE = 110
local MAX_ROD_DISTANCE = 100
local force

local TwInfo = TweenInfo.new(1.5)
local Tween = TwSer:Create(DaGui.Frame.ImageLabel, TwInfo, {Position = UDim2.new(.945,0,0,0)})

local fish
local hitPosition
local SwingTrack

local function getWorldMousePosition()
	local mouseLocation = UserInputService:GetMouseLocation()
	local screenToWorldRay = workspace.CurrentCamera:ViewportPointToRay(mouseLocation.X, mouseLocation.Y)
	local directionVector = screenToWorldRay.Direction * MAX_MOUSE_DISTANCE
	local weaponRaycastParams = RaycastParams.new()
	weaponRaycastParams.FilterDescendantsInstances = {Players.LocalPlayer.Character}
	local raycastResult = workspace:Raycast(screenToWorldRay.Origin, directionVector, weaponRaycastParams)
	if raycastResult then
		return raycastResult.Position
	end
end

local function GetRandomFish()
	if force <= 25 then
		return RepSto:FindFirstChild("Basic fish")
	elseif force <= 30 then
		return RepSto:FindFirstChild("Rare fish")
	elseif force <= 34 then
		return RepSto:FindFirstChild("Shiny fish")
	elseif force <= 38 then
		return RepSto:FindFirstChild("Baby mako")
	elseif force <= 40 then
		return RepSto.Bananalligator
	else
	    return
	end
end

local function UseRod()
	local mouseLocation = getWorldMousePosition()
	if mouseLocation then
		local targetDirection = (mouseLocation - tool.Handle.Position).Unit
		local directionVector = targetDirection * MAX_ROD_DISTANCE
		local weaponRaycastParams = RaycastParams.new()
		weaponRaycastParams.FilterDescendantsInstances = {Players.LocalPlayer.Character}
		local weaponRaycastResult = workspace:Raycast(tool.Handle.Position, directionVector, weaponRaycastParams)
		if weaponRaycastResult then
			hitPosition = weaponRaycastResult.Position
			local Material = weaponRaycastResult.Material
			if Material == Enum.Material.Water and hitPosition and Players.LocalPlayer:GetAttribute("Baits") >= 1 and IsUsingRod == false then
				IsUsingRod = true
				force = math.random(10, 41)
				Players.LocalPlayer:SetAttribute("Baits", Players.LocalPlayer:GetAttribute("Baits") - 1)
				if not SwingTrack then
					local Animator = Players.LocalPlayer.Character.Humanoid.Animator
					local Swing = Instance.new("Animation")
					Swing.AnimationId = "rbxassetid://12646581867"
					SwingTrack = Animator:LoadAnimation(Swing)
					SwingTrack.Priority = Enum.AnimationPriority.Action
					SwingTrack.Looped = false
				end
				SwingTrack:Play()
				SwingTrack:AdjustSpeed(.35)
				Players.LocalPlayer.Character.Humanoid.WalkSpeed = 0
				game.ReplicatedStorage.FISHING:FireServer(hitPosition)
				task.wait(5)
				if force >= 20 then
					fishGui.Force.Text = ("Fish's force: "..force)
					fishGui.Enabled = true
				else
					noFish.Enabled = true
				end
				game.ReplicatedStorage.FISHING.OnClientEvent:Once()
				noFish.Enabled = false
				fishGui.Enabled = false
				Players.LocalPlayer.Character.Humanoid.WalkSpeed = 16
				IsUsingRod = false
			end
		end
	end
end

tool.Activated:Connect(UseRod)

local function Fight()
	local PosOfDaGui = DaGui:WaitForChild("Frame").ImageLabel.Position
	if PosOfDaGui.X.Scale >= .41 and PosOfDaGui.X.Scale <= .51 then
		RepSto.BullSharkFished:FireServer(DaGui)
	else
		RepSto.BullSharkFished:FireServer(Players.LocalPlayer.Character)
	end
	return
end

fishGui.Force.Activated:Connect(function()
	if IDK == false then
		IDK = true
		fish = GetRandomFish()
	end
	force = force - 1
	fishGui.Force.Text = ("Fish's force: "..force)
	if force == 0 then
		fishGui.Enabled = false
		if fish then
			RepSto.BoughtSomething:FireServer(fish)
		else
			local Anim = Instance.new("Animation")
			Anim.AnimationId = "rbxassetid://12892091586"
			local AnimTrack = Players.LocalPlayer.Character.Humanoid.Animator:LoadAnimation(Anim)
			AnimTrack.Priority = Enum.AnimationPriority.Action2
			AnimTrack.Looped = false
			if tool then
			script.Parent = Players.LocalPlayer.PlayerScripts
			tool:Destroy()
			end
			game.Workspace.CurrentCamera.CameraType = Enum.CameraType.Scriptable
			AnimTrack:Play()
			AnimTrack:AdjustSpeed(.45)
			AnimTrack.Stopped:Wait()
			game.Workspace.CurrentCamera.CameraType = Enum.CameraType.Custom
			Players.LocalPlayer.PlayerGui.BlackScreen.Enabled = true
			game.SoundService.WaterSplash:Play()
			local place = Vector3.new(54.342, -21.323, -96.222)
			RepSto.BullSharkFished:FireServer(nil,place)
			task.wait(3)
			Players.LocalPlayer.PlayerGui.BlackScreen.Enabled = false
			local HasChosenDialog = false
			while HasChosenDialog == false do
				game.Workspace.Gorilla.Head.Help.DialogChoiceSelected:Connect(function(player, dialog)
					if player == Players.LocalPlayer and dialog.Name == "ItComes" then
						HasChosenDialog = true
					end
				end)
				wait(.1)
			end
			task.wait(3)
			while true do
				local Shark = RepSto.BullShark:Clone()
				Shark.Parent = game.Workspace
				local STween = TwSer:Create(Shark, TweenInfo.new(1.5),{Position = Players.LocalPlayer.Character.HumanoidRootPart.Position})
				STween:Play()
				DaGui.Enabled = true
				Tween:Play()
				Tween.Completed:Once()
			    if DaGui.Enabled == true then	
			        DaGui.Enabled = false
					Fight()
				end
				Shark:Destroy()
				DaGui.Frame.ImageLabel.Position = UDim2.new(0,0,0,0)
				HasChosenDialog = true
				RepSto.BullSharkFished.OnClientEvent:Connect(function()
					Shark:Destroy()
					script:Destroy()
				end)
				task.wait(math.random(3,6))
			end
		end
		IDK = false
	end
end)

local function CheckInput(Input)
	local Animate = false
	if DaGui.Enabled == true then
		if Input:IsA("InputObject") then
			if Input.KeyCode == Enum.KeyCode.E then
				Animate = true
			elseif Input.KeyCode == Enum.KeyCode.ButtonX then
				Animate = true
			end
		else
			Animate = true
		end
	end
	if Animate == true then
		DaGui.Enabled = false
		local Animator = Players.LocalPlayer.Character.Humanoid.Animator
		local animation = Instance.new("Animation")
		animation.AnimationId = "rbxassetid://12779583215"
		local animationtrack = Animator:LoadAnimation(animation)
		animationtrack:Play()
		Fight()
	end
	Animate = false
end

UserInputService.TouchTap:Connect(CheckInput)
UserInputService.InputBegan:Connect(CheckInput)

game:GetService("RunService").Heartbeat:Connect(function()
	if workspace.CurrentCamera.CameraType == Enum.CameraType.Scriptable then
		workspace.CurrentCamera.CFrame = Players.LocalPlayer.Character.Head.CFrame * CFrame.new(0,0,-.5)
	end
end)

Players.LocalPlayer.Character.Humanoid.Died:Connect(function()
	local Shark = game.Workspace:FindFirstChild("BullShark")
	if Shark then
		Shark:Destroy()
	end
	script:Destroy()
end)

*The one in ServerScriptService

local Event = game.ReplicatedStorage.BullSharkFished
local BadgeService = game:GetService("BadgeService")
local badgeId = 2143027673

Event.OnServerEvent:Connect(function(player, Variable, place)
	if not Variable then
		player.Character:MoveTo(place)
	else
		if Variable:IsA("Model") then
			if Variable:FindFirstChild("LeftFoot") then
				Variable.LeftFoot:Destroy()
				Variable.LeftUpperLeg:Destroy()
				Variable.LeftLowerLeg:Destroy()
			elseif Variable:FindFirstChild("RightFoot") then
				Variable.RightFoot:Destroy()
				Variable.RightUpperLeg:Destroy()
				Variable.RightLowerLeg:Destroy()
			elseif Variable:FindFirstChild("LeftHand") then
				Variable.LeftHand:Destroy()
				Variable.LeftUpperArm:Destroy()
				Variable.LeftLowerArm:Destroy()
			elseif Variable:FindFirstChild("RightHand") then
				Variable.RightHand:Destroy()
				Variable.RightUpperArm:Destroy()	
				Variable.RightLowerArm:Destroy()	
			else
				Variable.Head:Destroy()
			end
		else
s			local Punches = player:GetAttribute("Punches")
			Punches += 1
			player:SetAttribute("Punches", Punches)
		    if Punches == 3 then
			    player:SetAttribute("Punches", 0)
				Event:FireClient(player)
				local Shark = game.ReplicatedStorage.BullSharkTool:Clone()
				Shark.Name = "Bull shark"
				Shark.Parent = player.Backpack
				local success, badgeInfo = pcall(BadgeService.GetBadgeInfoAsync, BadgeService, badgeId)
				if success then
					if badgeInfo.IsEnabled then
						local awarded, errorMessage = pcall(BadgeService.AwardBadge, BadgeService, player.UserId, badgeId)
						if not awarded then
							warn("Error while awarding Badge:", errorMessage)
						end
					end
				else
					warn("Error while fetching Badge info!")
				end
			end
		end
	end
end)

local Players = game:GetService("Players")
local UserInputService = game:GetService("UserInputService")
local TweenSer = game:GetService("TweenService")
local Event2 = game.ReplicatedStorage.FISHING

Event2.OnServerEvent:Connect(function(player, GoalPos)
	local Rod = player.Backpack:FindFirstChild("Fishing rod")
	if not Rod then
		Rod = player.Character:FindFirstChild("Fishing rod")
	end
	task.wait(1)
	Rod.FishHook.Ball.Weld.Enabled = false
	local TwIn = TweenInfo.new(1)
	local goal = {CFrame = CFrame.new(GoalPos)}
	local Tween = TweenSer:Create(Rod.FishHook.Ball, TwIn, goal)
	Tween:Play()
	Tween.Completed:Wait()
	task.wait(8)
	Rod.FishHook:PivotTo(Rod.Handle.CFrame * CFrame.new(0,-1,0))
	Rod.FishHook.Ball.Weld.Enabled = true
	Event2:FireClient(player)
end)

If anyone still sees something that could be shortened I’m still open.

1 Like
#7

I think you missed one. This could also be Once.

1 Like
#8

I just tested my fishing rod again and it throws errors on the lines having :Once() I’m going back to :Wait() sorry…

The update got released, now this forum is no longer necessary.

1 Like