Emergency Vehicle System

Help and Feedback Code Review
#1

Code Review for Emergency Vehicle Lighting and Siren

I’ve created an ELS (Emergency Lighting System) for my vehicles. I’m not sure why, but something about this code seems buggy and/or like it could be optimized. I’m looking to make my code clean.

I’ve tried fake ternaries, but I always seem to run into problems with them. I don’t think I’m writing them properly, so I just remove them from my code.

If anyone has suggestions or feedback, it would be greatly appreciated. Thank you!

(Context: The GUI referenced in the code below is displayed only to the person in the vehicle seat.)

local pattern = "Off"
local siren = "Off"
local airhorn = false
local traffic = "Off"
local takedown = false
local alley = false
local car = workspace["2018 Ford Police Interceptor Utility"].Body
local sirens = car.ELS.Siren

--[[TEXT SCALING]]
SmartScale:NewGroup{
	TextObjects = {
		ContentFrame.Lights.TitleLabel,
		ContentFrame.Lights.ContentLabel,		
		ContentFrame.Siren.TitleLabel,
		ContentFrame.Siren.ContentLabel,	
		ContentFrame.Traffic.TitleLabel,
		ContentFrame.Traffic.ContentLabel,	
		ContentFrame.Floods.TitleLabel,
		ContentFrame.Floods.ContentLabel
	},
	
	OnlyUpdateWhenVisible = true,
	UpdateWhenMadeVisible = true
}
SmartScale:NewGroup{
	TextObjects = {
		TitleFrame.TitleLabel	
	},
	
	OnlyUpdateWhenVisible = true,
	UpdateWhenMadeVisible = true,
}

--[[FUNCTIONS]]
function checkIfSirenPlaying(sirenArg)
	if sirenArg == "Wail" then
		if sirens.Wail.IsPlaying == true then
			guiModify("Siren",string.upper("OFF"))
			siren = "Off"
		else
			guiModify("Siren","WAIL")
			siren = "Wail"
		end
	elseif sirenArg == "Yelp" then
		if sirens.Yelp.IsPlaying == true then
			guiModify("Siren",string.upper("OFF"))
			siren = "Off"
		else
			guiModify("Siren","YELP")
			siren = "Yelp"
		end
	elseif sirenArg == "Phaser" then
		if sirens.Phaser.IsPlaying == true then
			guiModify("Siren",string.upper("OFF"))
			siren = "Off"
		else
			guiModify("Siren","PHASER")
			siren = "Phaser"
		end
	elseif sirenArg == "Off" then
		guiModify("Siren","OFF")
		siren = "Off"
	end
end

function userInputBegan(i,gp)
	if not gp then
		if i.KeyCode == Enum.KeyCode.J then
			if pattern == "Pattern 1" then
				pattern = "Pattern 2"
			elseif pattern == "Pattern 2" then
				pattern = "Off"
			elseif pattern == "Off" then
				pattern = "Pattern 1"
			end
			car.ELS.Events.LightsEvent:FireServer(pattern)
			guiModify("Lights",string.upper(pattern))
		elseif i.KeyCode == Enum.KeyCode.R then
			car.ELS.Events.SirenEvent:FireServer("Wail")
			checkIfSirenPlaying("Wail")
		elseif i.KeyCode == Enum.KeyCode.F then
			car.ELS.Events.SirenEvent:FireServer("AirhornOn")
			guiModify("Siren","AIRHORN")
		elseif i.KeyCode == Enum.KeyCode.V then
			car.ELS.Events.SirenEvent:FireServer("Phaser")
			checkIfSirenPlaying("Phaser")
		elseif i.KeyCode == Enum.KeyCode.G then
			car.ELS.Events.SirenEvent:FireServer("Yelp")
			checkIfSirenPlaying("Yelp")
		elseif i.KeyCode == Enum.KeyCode.B then
			if traffic == "Left" then
				traffic = "Right"
				car.ELS.Events.TrafficEvent:FireServer("Right")
			elseif traffic == "Right" then
				traffic = "Split"
				car.ELS.Events.TrafficEvent:FireServer("Split")
			elseif traffic == "Split" then
				traffic = "Off"
				car.ELS.Events.TrafficEvent:FireServer("Off")
			elseif traffic == "Off" then
				traffic = "Left"
				car.ELS.Events.TrafficEvent:FireServer("Left")
			end
			guiModify("Traffic",string.upper(traffic))
		elseif i.KeyCode == Enum.KeyCode.N then
			car.ELS.Events.FloodEvent:FireServer("Takedown")
			takedown = not takedown
			if takedown and alley then
				guiModify("Floods","ALL")
			elseif takedown then
				guiModify("Floods",string.upper("Takedown"))
			elseif not takedown and alley then
				guiModify("Floods",string.upper("Alley"))
			else
				guiModify("Floods",string.upper("Off"))
			end
		elseif i.KeyCode == Enum.KeyCode.M then
			car.ELS.Events.FloodEvent:FireServer("Alleys")
			alley = not alley
			if takedown and alley then
				guiModify("Floods","ALL")
			elseif takedown then
				guiModify("Floods",string.upper("Takedown"))
			elseif not takedown and alley then
				guiModify("Floods",string.upper("Alley"))
			else
				guiModify("Floods",string.upper("Off"))
			end	
		end
	end
end

function userInputEnded(i,gp)
	if i.KeyCode == Enum.KeyCode.F then
		car.ELS.Events.SirenEvent:FireServer("AirhornOff")
		guiModify("Siren",string.upper(siren))
	end
end

function guiModify(frame,value)
	ContentFrame[frame].ContentLabel.Text = value
	if value == "OFF" then
		ContentFrame[frame].ContentLabel.TextColor3 = Color3.fromRGB(198, 40, 40)
	else
		ContentFrame[frame].ContentLabel.TextColor3 = Color3.fromRGB(0, 200, 83)
	end
end

--[[KEYBIND CONNECTIONS]]
UIS.InputBegan:Connect(userInputBegan)
UIS.InputEnded:Connect(userInputEnded)
``
8 Likes
#2

Lua does not have switch-case statements, which means your code is going to have if statements. Lots and lots of if statements. This isn’t a bad thing though - long code isn’t synonymous with bad code, or dirty code.

However, if you’d like to clean up a bit, here are some suggestions:

Boolean Logic

For example, in your code you have

Here, if takedown == true you will never reach the check for not takedown. You are effectively asking if takedown is true, and if the answer if no you’re proceeding to ask if it is NOT true. This is redundant.

You can also remove the line takedown = not takedown, and then reverse your conditional checks.
For example, rather than

takedown = not takedown
if takedown and alley then
    ...

you could have

if alley and not takedown then
    ...

However this would then also benefit from re-arranging your conditional statements, but I’m not going to go that deep into this.

Normalisation

In all of your guiModify() calls, you have either parsed an all-caps string or set it to all-caps with string.upper(). Here there are two possibilities.
Possibility number one, you are never calling guiModify() with unknown parameters. If this is the case, then you should simply be parsing in an all-caps string, rather than wasting computation time with string.upper(). This is the difference:

guiModify("Floods", string.upper("Takedown"))
-- vs
guiModify("Floods", "TAKEDOWN")

One of these looks cleaner to me.

The second possibility is that you are calling with unknown strings, and you can’t guarantee that the input will be upper-case. In this case, you should parse these strings in unmodified and then upper-case them within the function.

function guiModify(frame, value)
    value = string.upper(value)
    ...
end

Variables

I’m using guiModify() as an example again. Simply, you have multiple calls to ContentFrame[frame] - if you ever call something more than once, you should assign it to a variable.

function guiModify(frame, value)
    frame = ContentFrame[frame]
    ...
end

Magic strings

Last but definitely not least - magic strings. These are bad! Never use them! Tut tut!
Strings are computationally expensive and using them in conditional statements leaves you open to mystery errors caused by some typo 300 lines ago.

if pattern == "Pattern 1" then
    pattern = "Pattern 2"
elseif pattern == "Pattern 2" then
    ...

and so on. In most languages, Enums would be the solution here - unfortunately, Lua really doesn’t have a good system for custom Enums. The easiest way to get around this then would be tables:

local patterns = {}
patterns.Off = 0
patterns.Pattern1 = 1
patterns.Pattern2 = 2

if pattern == patterns.Pattern1 then
    pattern = patterns.Pattern2
elseif pattern == patterns.Pattern2 then
    pattern = patterns.Off
...

The nice things about this approach are that the Lua linter is aware of what is inside the patterns table, so auto-complete can be used, and the compiler will throw an error in the right place at the right time if you have a typo, such as pattern = patterns.Of.

On another note, I’ve typed the word “pattern” so much now that I am beginning to doubt it is even real.

I hope this has been a helpful code review, and good luck in your future scripting!

6 Likes
#3

This is amazing, thank you so much! Is it fine if I DM you with a few more questions about your post?

#4

Of course, feel free to DM me any time. I’m not always the fastest to reply but I’ll always get back to you!

1 Like