Also, if you’re looking for general, concise improvements; there’s a lot that can be done here.
You are not following any general code-writing guidelines, which is fine in practice: only if you never plan on sourcing collaboration. Also, I would heavily recommend using Promises in this sense instead of LuaTuple types.
Here is a completely altered code! Dissect it all you want, this is just the way I would’ve made it if I had the opportunity to.
Server-side
-- atmConvert.server.lua
--!strict
---@diagnostic disable: undefined-global
-- Never prefix your services with "__", this is a convention for private variables and functions inside of a class
-- Also, keep your services at the top of your file so you're not constantly redeclaring them every time you need them
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local CollectionService = game:GetService("CollectionService")
-- Use promises instead of LuaTuples, they are much easier to use and read
-- I am using VSCode and Wally, so that is what ReplicatedStorage.Packages refers to
local Promise = require(ReplicatedStorage.Packages.Promise)
-- Keep these at the top too, so you don't have to unnecessarily declare them every time you need them
local clientEvents: Folder = ReplicatedStorage:WaitForChild("ClientEvents")
local convert: RemoteFunction = clientEvents:WaitForChild("Convert")
-- create a type for the ATM model so that you can easily navigate the children with intellisense
type ATMModel = Model & {
SA: Instance & {
ErrorSound: Sound,
ProcessSound: Sound,
WithdrawSound: Sound
}
}
type Character = Model & {
Humanoid: Humanoid,
Values: Folder & {
Ragdolled: BoolValue
}
}
type ExtendedPlayer = Player & {
Character: Character?,
Stats: Folder & {
-- killstreak does not have a capital S
Killstreak: IntValue
}
}
-- store the convert.OnServerInvoke function in a local function so that you can easily debug it
local function convertInvoked(player: ExtendedPlayer, amount: number | any, atm: ATMModel)
-- Use promises so that you don't have to use an ugly LuaTuple type [boolean, string?]
return Promise.new(function(resolve, reject, onCancel)
-- do character checking first so that you don't have to unnecessarily do everything else, just to drop it later
local character = player.Character
-- also don't use FindFirstChildOfClass, it is slower and Humanoid should never be renamed
if not character or not character:FindFirstChild("Humanoid") then
return reject("Your character is not valid!")
end
-- never name a folder with the word "Folder" in it, it is redundant
-- also, for BoolValues or any type of boolean, you do not need to do "Value == true", just "Value" is enough
-- this is because is already a boolean, it's like doing "if true == true then"
if not character.Humanoid.Health > 0 or character.Values.Ragdolled.Value then
return reject("Your character is dead!")
end
-- instead of doing an "if not" statement, just do an "if" statement so it takes less space
-- also, do not shorten service names, the only real exception to this for me is "UserInputService" to "UserInput"
if CollectionService:HasTag(player, "junkCooldown") then
return reject("You are on cooldown!")
end
CollectionService:AddTag(player, "junkCooldown")
task.delay(2, function()
CollectionService:RemoveTag(player, "junkCooldown")
end)
-- please give your variables proper names, not "P" or "KS", abbreviations are very horrible to debug
local unknownChild = atm.SA
-- never name a variable "err" unless it is an error object
-- also, give your Instances concise names, do not randomly switch from camelCase to PascalCase
-- use attributes here instead of Value objects as they clog up the hierarchy
if unknownChild:GetAttribute("busy") then
return reject("ATM is busy!")
end
-- do not abbreviate "killstreak" as "KS", it is hard to read
local killstreak: number? = tonumber(amount)
if not killstreak then
return reject("Parameter 'amount' is not a number!")
end
-- do not reject if not a whole number, just round it down
killstreak = math.floor(killstreak)
if killstreak == 0 then
return reject("Parameter 'amount' expected a non-zero number!")
end
-- i don't know if this module belongs to you, but :Distance should be renamed to :GetDistance or :CompareDistance
if player.Stats.Killstreak.Value < killstreak then
return reject("You do not have enough killstreak!")
end
-- do not run both comparisons for killstreak and distance to the atm, you will get an error which you can't tell which one it is
if not RisytalEngine:Distance(character, 8, atm.PrimaryPart) then
return reject("You are too far away from the ATM!")
end
unknownChild:SetAttribute("busy", true)
unknownChild.ProcessSound:Play()
-- use a connection for this instead of rawdogging a task.wait(sound length)
-- however, you should make this a variable so you can disconnect it later to avoid a memory leak
local connection
connection = unknownChild.Process.Ended:Connect(function()
-- disconnect it at the top so you don't have to disconnect it in multiple places
-- declaring something in spot so you don't have to redefine it multiple times is a good practice
-- it also makes your code way more readable and concise
connection:Disconnect()
-- MAKE SURE TO SET BUSY TO FALSE BEFORE YOU REJECT OR RESOLVE!
-- THIS ERROR IS PRESENT IN THE ORIGINAL SCRIPT
unknownChild:SetAttribute("busy", false)
if not RisytalEngine:Distance(character, 8, atm.PrimaryPart) then
unknownChild.ErrorSound:Play()
return reject("You went too far away from the ATM!")
elseif character.Humanoid.Health <= 0 or character.Values.Ragdolled.Value then
unknownChild.ErrorSound:Play()
return reject("Your character died!")
end
unknownChild.WithdrawSound:Play()
player.Stats.Killstreak.Value -= killstreak
-- i do not know what GetTIXKS does, but it should be renamed to something more descriptive
-- also should be in camelCase intead, PascalCase is for classes
player.leaderstats.TIX.Value += killstreak * getTIXfromKillstreak(player)
-- you should resolve with true as there's no extra data to resolve with
return resolve(true)
end)
-- you should disconnect the connection if the promise is cancelled,
-- there is multiple reasons why a promise may be cancelled, such as the player leaving the game
onCancel(function()
connection:Disconnect()
end)
end)
end
convert.OnServerInvoke = convertInvoked
Client-side example
-- templateAtmConvertCall.client.lua
--!strict
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local clientEvents: Folder = ReplicatedStorage:WaitForChild("ClientEvents")
local convert: RemoteFunction = clientEvents:WaitForChild("Convert")
-- just a template, i doubt this is what your code actually looks like
local testAmount = 100
local testModel = game.Workspace:WaitForChild("ATM")
-- You should wrap this in a pcall too, just in case something completely unexepected was to occur
local success, response = pcall(convert.InvokeServer, convert, testAmount, testModel)
if not success then
warn("Failed to invoke convert: " .. response)
-- use debug.traceback() to get the full stack trace of the error
-- you use this as pcall will swallow the error and only return the error message
warn(debug.traceback())
return
end
response
:andThen(function(result)
print("Success: " .. result) -- will print true, this is where you can handle it however
end)
:catch(function(err)
warn("Failed to convert: " .. err)
-- also use debug.traceback() here to be able to easily debug what all went down
warn(debug.traceback())
end)
