Hi is it bad practice to set a value to something it already is?
For example I might have a light and there might be 2 buttons, OFF and ON and this is the code that manages them. Would this be bad practice or should I check if its already that value shown in the 2nd code?
local Light = "OFF"
function OFF()
Light = "OFF"
end
function On()
Light = "On"
end
Or should I do this?
local Light = "OFF"
function OFF()
if Light == "OFF" then return end
Light = "OFF"
end
function On()
if Light == "On" then return end
Light = "On"
end
At this level, it’s too insignificant to matter. However, if you run into a similar situation where additional code is executed to enter the desired state, then you would want to avoid that redundancy. An extreme example could be a switch that involves enabling all in-game light fixtures, where each light fixture could have multiple lighting components. If you were to try to toggle on these light fixtures repeatedly, you’d redundantly work through each component of every light fixture.
A better design for your particular example, and most related examples, would be to remove the option to enter the same state entirely by linking both operations to one function or button, where each subsequent call or interaction inverts the current state. An on/off state should be represented with booleans too as it is a binary state, and this gives us the ability to simplify your code:
local lightsOn = false
local function toggleLights()
lightsOn = not lightsOn
if lightsOn then
-- Turn on lights
else
-- Turn off lights
end
end
I do it all the time if that makes my code shorter, It’s insignificant. If anybody tries to tell you otherwise they’re either trolling or trying to act smart.
The only case I would say don’t do so is if you have say a bunch of functions that fire when the value of a property changes. And having those functions fire without reason could be bad, but realistically the first thing the function should do is check if the old value is the same as the new value and if so just return immediately
I mean I always check if it has to change if it’s something that would run multiple times.
For example if it keeps pressing an elevator button it should check if it’s not already pressed.
Anyway I think it’s good practice to check it, but in your case it would be better to make a toggle function where it toggles based on current state
if you making settings you could make folder inside player named “Settings” or something and inside that folder put bool values and when player clicks button remote gets fired and the server updates the setting value
example :
–// server script
local PlayerSettings = {
["Particles"] = false --// if true than setting is on
}
for i,v in pairs(PlayerSettings) do
--// create a bool value inside players settings folder and on client handle all the settings displaying and stuff
end
if you are using these functions to only change the Light variable then its fine to not quit the function because that isn’t a costly operation
however if you have other logic in these functions then it would be better to exit the loop
example:
local Light = "OFF"
--[[
The functions are expensive so its good for performance to exit the loop when there is no need to run it
]]
function OFF()
if Light == "OFF" then return end
Light = "OFF"
for _,light in workspace:GetDescendants() do
if light:IsA("SpotLight") or light:IsA("SurfaceLight") or light:IsA("PointLight") then
light.Enabled = false
end
end
end
function On()
if Light == "On" then return end
Light = "On"
for _,light in workspace:GetDescendants() do
if light:IsA("SpotLight") or light:IsA("SurfaceLight") or light:IsA("PointLight") then
light.Enabled = true
end
end
end
So this is my usecase, ToggleLights() would be fired when the .Stabling value changes to true to turn all lights off, when the EndLights mode gets changed, when the coupled state is changed and when the stabling value changes to false to turn lights on based on the settings.
Each time the function is fired, it turns all the lights off and then turns them on based on the settings.
local Red: Color3 = Color3.fromRGB(255, 0, 0)
local White: Color3 = Color3.fromRGB(255, 255, 255)
function MultipleUnitService:ToggleLight(CardinalDirection, LightName: string, State: string)
local Lights = self.Miscellaneous.Lights[CardinalDirection]
local Light = Lights[LightName]
local isEnabled = (State == "Enabled")
-- Set common properties
Light.Material = isEnabled and Neon or SmoothPlastic
Light.Attachment.SpotLight.Enabled = isEnabled
-- Handle specific light properties
if LightName == "Taillights" then
Light.Color = isEnabled and Red or White
end
end
function MultipleUnitService:ToggleLights(CardinalDirection)
local Cab = self.Cabs[CardinalDirection]
self:ToggleLight(CardinalDirection, "Headlights", "Disabled")
self:ToggleLight(CardinalDirection, "DitchlightLeft", "Disabled")
self:ToggleLight(CardinalDirection, "DitchlightRight", "Disabled")
self:ToggleLight(CardinalDirection, "Taillights", "Disabled")
if self.Train.Stabling == true then return end
if Cab.MasterKey == true then
if Cab.EndLights == "H&D" then
self:ToggleLight(CardinalDirection, "Headlights", "Enabled")
self:ToggleLight(CardinalDirection, "DitchlightLeft", "Enabled")
self:ToggleLight(CardinalDirection, "DitchlightRight", "Enabled")
end
elseif Cab.MasterKey == false and Cab.Coupled == false then
self:ToggleLight(CardinalDirection, "Taillights", "Enabled")
end
if Cab.EndLights == "Headlights" then
self:ToggleLight(CardinalDirection, "Headlights", "Enabled")
elseif Cab.EndLights == "Ditchlights" then
self:ToggleLight(CardinalDirection, "DitchlightLeft", "Enabled")
self:ToggleLight(CardinalDirection, "DitchlightRight", "Enabled")
end
end
function MultipleUnitService:UpdateEndLights(CardinalDirection, Mode)
self.Cabs[CardinalDirection].EndLights = Mode
self:ToggleLights(CardinalDirection)
end
In this case, I would say your code is fine the way it is since it isn’t doing anything too expensive. However, someone else may have some possible optimizations you could implement.
I suppose these 2 new lines probably improve it even if its just by a smidge?
function MultipleUnitService:ToggleLight(CardinalDirection, LightName: string, State: string)
local Lights = self.Miscellaneous.Lights[CardinalDirection]
local Light = Lights[LightName]
local isEnabled = (State == "Enabled")
--New lines of code
local Material = (isEnabled and Neon) or SmoothPlastic
if Light.Material == Material then return end
-- Set common properties
Light.Material = Material
Light.Attachment.SpotLight.Enabled = isEnabled
-- Handle specific light properties
if LightName == "Taillights" then
Light.Color = isEnabled and Red or White
end
end
OR
function MultipleUnitService:ToggleLight(CardinalDirection, LightName: string, State: string)
local Lights = self.Miscellaneous.Lights[CardinalDirection]
local Light = Lights[LightName]
local isEnabled = (State == "Enabled")
--New lines of code
if Light.Attachment.SpotLight.Enabled == isEnabled then return end
-- Set common properties
Light.Material = (isEnabled and Neon) or SmoothPlastic
Light.Attachment.SpotLight.Enabled = isEnabled
-- Handle specific light properties
if LightName == "Taillights" then
Light.Color = isEnabled and Red or White
end
end