Constructive Feedback on my plane handler

Help and Feedback Code Review
#1 
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local UserInputService = game:GetService("UserInputService")
local RunService = game:GetService("RunService")
local Players = game:GetService("Players")

local player = Players.LocalPlayer
local mouse = player:GetMouse()

local remotesFolder = ReplicatedStorage.Remotes.Plane
local serverReplicationFolder = remotesFolder.ServerReplication

local planesFolder = workspace.Planes

-- Plane Variables

local planeEngineOn = nil	
	
local MAX_PLANE_SPEED = 175
local CURRENT_PLANE_SPEED = 0
local STALL_PLANE_SPEED = 0	

-- Events

remotesFolder.Plane_Control.OnClientEvent:Connect(function(plane)	
	
-- Folders
	
local bodyVelocity = plane.PrimaryPart.BodyVelocity	
local gyro = plane.PrimaryPart.BodyGyro
	
local afterBurner = plane.Engine.Afterburner
local gunHolder = plane.GunHolder
local gun = plane.Gun	
	
local seatValue = plane.Handler.Driver		
	
if not plane.PrimaryPart then
	warn(string.format("No primary part found in %s %s", plane.Name, ", expected 'PrimaryPart' "))
	return
end
	
local function updateEngineColor()
	remotesFolder.ServerReplication.ReplicateEngineColor:FireServer(afterBurner, CURRENT_PLANE_SPEED)
end	
	
local function updatePlaneEngineSound()
	-- Unfinished
end

local function updatePlaneVelocity()
	bodyVelocity.MaxForce = Vector3.new(19250, 19250, 19250) * ( CURRENT_PLANE_SPEED / 1.2)
	bodyVelocity.Velocity = mouse.Hit.LookVector * CURRENT_PLANE_SPEED
end

local function updatePlaneGyro()
	gyro.CFrame = CFrame.new(plane.PrimaryPart.Position, mouse.Hit.LookVector * gyro.P)
end
	
local function updatePlaneSpeed(bool)
	if bool then
		if CURRENT_PLANE_SPEED < MAX_PLANE_SPEED then
			CURRENT_PLANE_SPEED +=  1	
		end
	else 
		if CURRENT_PLANE_SPEED > STALL_PLANE_SPEED then
			CURRENT_PLANE_SPEED -= 1
		end
	end
end	
	
local function resetPlanePropertyValues()
		
	--[[ Reset plane property values
		
		Set's the plane speed, it's max force of body velocity and engine back
		to default. If statements to prevent value changing unnecessarily
		
	    // MaxForce = Vector3.new()
		// MaxTorque = Vector3.new()
		// Plane_Speed = Default or 0
		
	]]	
		
	if bodyVelocity.MaxForce ~= Vector3.new() then
		bodyVelocity.MaxForce = Vector3.new()
	end		
		
	if CURRENT_PLANE_SPEED > STALL_PLANE_SPEED then
		CURRENT_PLANE_SPEED = STALL_PLANE_SPEED
	end
		
	updatePlaneVelocity()	
	updateEngineColor()	
end		
	
--[[ Input Handler
	
Uses a function to handle plane speed
	
]]	
	
UserInputService.InputBegan:Connect(function(input, gameProcessed)
		
	if not gameProcessed and input.KeyCode == Enum.KeyCode.E then
		planeEngineOn = not planeEngineOn
	end
		
	if planeEngineOn then	
		
		while not gameProcessed and UserInputService:IsKeyDown(Enum.KeyCode.W) and wait() do
			updatePlaneSpeed(true)	
		end
	
		while not gameProcessed and UserInputService:IsKeyDown(Enum.KeyCode.S) and wait() do
			updatePlaneSpeed(nil)	
		end		
	end
end)	
	
RunService.Stepped:Connect(function()
		
		--[[ 
			
			-- Fires on physics simulation,	useful when handling objects
			unanchored and their properties associated with it.
			
			If statements used to prevent values changing unnecessarily, helps in keeping
			efficient memory, though only improving 0.000000000000001% memory.
			
			-----------------------------------------------------------------------------
            / Functions	
		
		    default() -- Calls the function to reset all the values associated with the plane 
		
		--]]
		
		if not planesFolder:FindFirstChildWhichIsA("Model") then
			wait()
			script:Destroy()
		end
		
		if not seatValue.Value then -- No Player found in seat, turn off the engine
			if not planeEngineOn then
				planeEngineOn = nil
			end
			wait()
			script:Destroy()
		end
		
		if planeEngineOn then	
			updatePlaneGyro()
			updatePlaneVelocity()
			updateEngineColor()
		else
			resetPlanePropertyValues()
			return
		end
	end)
end)

Updated: Almost the whole script.

Here’s what I want reviewed:

  • Readability of code
  • Efficiency of code
#2
Wall of text 
if planeEngine then planeObject.PrimaryPart.BodyVelocity.Velocity = mouse.Hit.LookVector * planeSpeed end

No reason for having this be a one-liner. Split it up so we don’t have to scroll horizontally to see it.

This is bad variable naming, because it doesn’t tell the reader what the variable refers to. When you first see it at the top of the script, it seems like it’s supposed to refer to some Instance that represents the engine of the plane, i.e. an object. What it actually seems to represent is whether or not the plane (engine) is turned on or off. It’s extra confusing to change it between true and nil. Because it represents an on/off state, it’s much more natural to always have it set to a bool.

Change the variable name to planeEngineOn or something similar, and switch it between true and `false.

You’re not using the planeMoving variable, so it should be deleted.

The updatePlaneSpeed and rotatePlane functions both update something about the plane and are called in the same context (updating the plane), so I would change the name of rotatePlane to reflect this, e.g. to updatePlaneRotation or updatePlaneOrientation.

They both use the same checks to see if actually doing the update is valid/should happen. When calling a function called updateSomething, it’s pretty natural to expect it to actually update something. Right now, the update functions can be called without actually updating anything. For these reasons I would move the checks outside of the functions, and instead put them in the RunService.Stepped callback. Since you’re already doing half the check in there, it’s not even a big change.

The update speed function takes a speed parameter, but the updateOrientation function doesn’t take an orientation parameter. I think they should both take no parameters, since it’s always clear which plane they’ll be updating, namely the one that’s defined in the outermost scope of the script.

Since you’re dealing with user input in a LocalScript, I’d update controls on RenderStepped rather than Stepped, because RenderStepped has the potential to be faster and that should result in more responsive- feeling controls.

I’m pretty sure your format string is wrong. It should be "No primary part found in: %s"

(missing %s)

Setting the BodyVelocity.Velocity to (0, 0, 0) will cause the plane to be unable to move, which isn’t great because you may want it to either fall down from the sky if it’s stalled mid-air, or move around when hit by e.g. a tank driving over it. You should set the MaxForce to (0, 0, 0) when the speed is 0.

Here's how I'd write your script: 
local UserInputService = game:GetService("UserInputService")
local Players = game:GetService("Players")
local RunService = game:GetService("RunService")

local MAX_PLANE_SPEED = 20
local MAX_FORCE = 40000
local MAX_TORQUE = 10000

local player = Players.LocalPlayer
local mouse = player:GetMouse()

local plane = game.Workspace:WaitForChild("Plane")

local planeEngineOn = false
local planeSpeed = 0

local function updatePlaneSpeed()
	local v = plane.PrimaryPart.BodyVelocity
	v.Velocity = mouse.Hit.LookVector * planeSpeed

	if planeSpeed == 0 then
		v.MaxForce = Vector3.new()
	else
		v.MaxForce = Vector3.new(1, 1, 1) * MAX_FORCE
	end
end

local function updatePlaneOrientation()
	plane.PrimaryPart.BodyGyro.CFrame = CFrame.new(
		plane.PrimaryPart.Position, 
		mouse.Hit.Position
	)

	if planeSpeed == 0 then
		v.MaxTorque = Vector3.new()
	else
		v.MaxTorque = Vector3.new(1, 1, 1) * MAX_TORQUE
	end
end

UserInputService.InputBegan:Connect(function(input)
	if input.KeyCode == Enum.KeyCode.E then
		planeEngineOn = not planeEngineOn
	end

	if planeEngineOn then
		if input.KeyCode == Enum.KeyCode.W then
			planeSpeed += 1
		end
		
		if input.KeyCode == Enum.KeyCode.S then
			planeSpeed -= 1
		end

		planeSpeed = math.clamp(planeSpeed, 0, MAX_PLANE_SPEED)
	end

	--no need to wait for a RenderStepped to happen (probably makes no difference)
	updatePlaneSpeed() 
	updatePlaneOrientation()
end)

RunService.RenderStepped:Connect(function()
	if planeObject.PrimaryPart then
		updatePlaneOrientation(plane)
		updatePlaneSpeed(plane, planeSpeed)	
	else
		warn(string.format("No PrimaryPart found in: %s", planeObject:GetFullName()))
	end	
end)
1 Like
#3

Yup, forgot that %s. Thanks!

( 30 characters )

#4

Can I also just set the velocity to 0 instead of max force?

#5

Nope. That will cause the BodyVelocity to STRUGGLE REALLY HARD to make it move at 0 studs/s, i.e. stand still no matter what. The only way to make it not do anything is to set the MaxForce to 0

1 Like