The problems of naive coding

Resources Community Resources
#1

Naive Coding, oh boy where do I even begin…

Taking a naive approach your code can be really damaging, as you’re just assuming everything is safe once it’s written.

Below I’ve provided some reasons why being naive is not a good idea

1. Security

This is the processing of handling remotes and other stuff. If you leave your remotes unsecured, exploiters could easily just fire them and give themselves extra boosts or affect other players’ experiences.

For example, lets assume we have a function that makes a store purchase
In this first example, we’ll do all the validation on the client.

local ReplicatedStorage = game:GetService("ReplicatedStorage")
local LocalPlayer = game:GetService("Players").LocalPlayer
local Cash = LocalPlayer:WaitForChild("leaderstats").Cash
local ProductMetadata = require(ReplicatedStorage.Products)

local function MakePurchase(itemId)
   local item = assert(ProductMetadata[itemId], "Unrecognised Item")
   if Cash.Value >= item.Cost then
      Cash.Value -= item.Cost
      ReplicatedStorage.DoPurchase:FireServer(itemId)
   end
end

MakePurchase("Sword")

The problem with this is that the DoPurchase function will simply grant the item and not remove currency or check that the user even has enough

The solution here? Do your validation on the server, not the client.
So yet again, here we our with our NEW client script

local ReplicatedStorage = game:GetService("ReplicatedStorage")
local LocalPlayer = game:GetService("Players").LocalPlayer
local Cash = LocalPlayer:WaitForChild("leaderstats").Cash

local function MakePurchase(itemId)
  ReplicatedStorage.DoPurchase:FireServer(itemId)
end

MakePurchase("Sword")

And on the server side of things we can do this

local ReplicatedStorage = game:GetService("ReplicatedStorage")
local ProductMetadata = require(ReplicatedStorage.Products

ReplicatedStorage.DoPurchase.OnServerEvent:Connect(function(p, itemId)
   local item = assert(ProductMetadata[itemId], "Unrecognised Item")
   local cash = p.leaderstats.Cash
   if Cash.Value >= item.Cost then
      Cash.Value -= item.Cost
      --grant the item
   end
end)

Now, whenever DoPurchase is fire from a client, it’ll do everything as well as deduct the cash from the server

2. Assuming Truthy Returns (FindFirstChild etc.)

Truthy returns is where a function will return something that is not nil or false. Certain functions such as FindFirstChild, etc. will not always return truthfully

Here’s the example why this can be bad

local Part = workspace:FindFirstChild("Part")
Part.Anchored = false

The problem? Part might not exist in workspace, meaning this code will error as you’re attempting to index a nil reference

This fix is quite simple, all you have to do is change the function to WaitForChild()

local Part = workspace:WaitForChild("Part")
Part.Anchored = false

3. Not wrapping HTTP requests in pcalls

HTTP requests can fail, this extends to the functions that need to use HTTP to work propertly (DataStore:SetAsync, TextFilterResult, Http:GetAsync)

Our example will be datastores, as SetAsync does return an error if it doesn’t work properly, however your code could end up creating new data ontop of pre-existing data

local DataStoreService = game:GetService("DataStoreService")
local DataStore = DataStoreService:GetDataStore("Example")
local Players = game:GetService("Players")

local function createNewData(userId)
  --idk make a new datastore
end

Players.PlayerAdded:Connect(function(p)
  local data = DataStore:GetAsync(p.UserId)
  if not data then
    createNewData(p.UserId)
  end
end)

This code can error and result in users not receiving their data, and because it is unhandled, there is a chance that the user could lose their data and get it overwritten

The fix is simple, wrap GetAsync in a pcall and then tag the player

local DataStoreService = game:GetService("DataStoreService")
local CollectionService = game:GetService("CollectionService")
local DataStore = DataStoreService:GetDataStore("Example")
local Players = game:GetService("Players")

local function createNewData(userId)
  --idk make a new datastore
end

Players.PlayerAdded:Connect(function(p)
  local success, data = pcall(DataStore.GetAsync, DataStore, p.UserId)
  if not success then
    CollectionService:AddTag(p, "DataFetchError")
    return
  end
  if not data then
    createNewData(p.UserId)
  end
end)

By just adding a pcall and a tag called ‘DataFetchError’, it allows us to handle DataStore errors, preventing data from getting overwritten (assuming you also make sure to handle the tag)

local function SetData(ds, p, newData)
  if CollectionService:HasTag(p, "DataFetchError") then
    return --prevents overwrite of data
  end
  
  ds:SetAsync(p.UserId, newData) --technically this should be PCALL wrapped as well, however I am not writing an entire datastore system
end

3.1 Assuming TextFilterResult’s functions will always work

There’s no need to provide code for this one as the Wiki does a good job of summarising why you should not assume this, and it extends from the previous point.

If you incorrectly assume that TextFilterResult always works, you could end up displaying unfiltered text, which can result in the game being moderated as it does not filter text correctly

23 Likes
Defining and Better words
#2

Wow, this is really informative, thanks for sharing this!:grin:

#3

Just to add on to OP’s post, remote events should also be checked to see if they are being spammed. Hackers can spam remote events and break the server. However, if you check if they are going overboard, you can find the player in the tuple of arguments sent by the remote event automatically, and then kick the player and add them to a ban list. However, you would have to make sure you would only ban them when there is actually real remote event spamming involved.

And also, isn’t waitforchild unsafe because it can yield, preventing your script from running?

#4

That isn’t really “naive”.

That is literally the whole point of it, so it waits for the child

2 Likes
#5

Yeah it is supposed to wait for the object to be loaded but in doing that it can make your script not run and screw up your entire game

#6

Not sure how that is relevant. If it yields indefinitely then it is an issue you caused, like not using the correct name, not necessarily naivety.

#7

Is relevant. Having a game that actually runs is totally relevant. You are right that the mistake it caused by not spelling what to wait for correctly or not referencing it right, and you can fix that, but there are still other ways this could go horribly wrong. On slow devices, loading can take a while, and that object being waited for could be needed very soon, so it might be better to put it in a pcall, wait and see if it loads, and if not, just use an alternative method for that thing you want to do that doesn’t require the object. Let’s say I have a part I need to be loaded to do this animation, but someone is playing on a slow device and it doesn’t get loaded in time. I don’t want to throw an error by trying to animate a part that doesn’t exist, so if I don’t get it in time I just don’t animate it and try to find some way to cover it up or still make it look good alternatively.

In short: Slow devices don’t always work with waitforchild, especially in big games, so just safe proof it by adding a pcall or just seeing if the variable referring to the object is still nil and then using alternative.

#8

Is not.

I also don’t understand how having a slow device is related to naivety in anyway.

The entire purpose of :WaitForChild is to yield the thread until an instance with the specified name is found. If it is yielding indefinitely for you then that is an issue with your code and not because you are naive or whatever.

Please stay on topic.