Are there bad practices in my script?

Help and Feedback Scripting Support
#1

I just started to use datastore2. I wrote a datastore script using datastore2 that works and all.

HOWEVER, I’m not sure if there is any bad practices in my script that will lead to players losing data.

Note: When something gives coins or gems to a player, it gets saved automatically.

local Datastore2Module = require(script.DataStore2)
local Players = game:GetService("Players")
local ReplicatedStorage = game:GetService("ReplicatedStorage")

local Mainkey = "Data"

local DefUserData = {
	Currency = {
		["Coins"] = 0;
		["Gems"] = 0;
	};
	Strings = {};
}

Datastore2Module.Combine(Mainkey, "Currency", "Strings")

Players.PlayerAdded:Connect(function(plr)
	local UserData = Datastore2Module(Mainkey, plr):GetTable(DefUserData)
	
	local CurrencyData = Datastore2Module("Currency", plr):GetTable(DefUserData.Currency)
	local StringsData = Datastore2Module("Strings", plr):GetTable(DefUserData.Strings)
	
	local leaderstats = script.leaderstats:Clone()
	
	for currency, value in pairs(CurrencyData) do
		leaderstats[currency].Value = value
	end
	for i, String in pairs(StringsData) do
		local NewString = Instance.new("Model", leaderstats.Strings)
		NewString.Name = String
	end
	
	leaderstats.Parent = plr
	
	local function UpdateStats()
		UserData.Currency.Coins = leaderstats.Coins.Value
		UserData.Currency.Gems = leaderstats.Gems.Value
		
		local StringsTable = {}
		
		for i, String in pairs(leaderstats.Strings:GetChildren()) do
			table.insert(StringsTable, String.Name)
		end
		
		UserData.Strings = StringsTable
		
		--// Set the data
		Datastore2Module("Currency", plr):Set(UserData.Currency)
		Datastore2Module("Strings", plr):Set(UserData.Strings)
		
		print(Datastore2Module("Currency", plr):GetTable(DefUserData.Currency), Datastore2Module("Strings", plr):GetTable(DefUserData.Strings))
	end
	
	for i, instance in pairs(leaderstats:GetChildren()) do
		if instance:IsA("ValueBase") then
			instance.Changed:Connect(UpdateStats)
		elseif instance.Name == "Strings" then
			instance.ChildAdded:Connect(UpdateStats)
		end
	end
end)
#2

Well I’d like to point out that when using a for loop you can replace the index variable with _ like so:

for _, v in pairs() do

You would only need to do this if you do not need the index variable.

1 Like
#3

I don’t think that affects the ability of this script to save data though. As I said: “HOWEVER, I’m not sure if there is any bad practices in my script that will lead to players losing data.”

#4

Who knows the return from this? you can never be certain because it is never checked.

All these are concatenated function calls, each one assuming the return value, never do this.

No check whether the clone worked or if indeed the object to be cloned actually exists.

Memory can always reach a wall, always check allocations were succesful.

Embedding functions within functions is not necessary, everything here can be supplied to locally defined functions.

1 Like
#5

Thank you for the criticism, I will consider them. But what do you mean when you said to “check allocations”

#6 
local NewString = Instance.new("Model", leaderstats.Strings)
if NewString ~= nil then
	NewString.Name = String;
end
1 Like
#7

I don’t get it. If roblox creates a new instance, why would I need to check if it is nil when the last line literally says “Instance.new(“Model”, leaderstats.Strings)”

#8

This isn’t a thing, what? If the instance can’t be created, an error is raised. Instance.new cannot just randomly return nil. I’m not exactly sure what happens if you run out of memory but it’s definitely not “things start silently returning nil”.

They believe that Instance.new can return nil if there’s no more memory left. But that’s not a thing that can happen - Instance.new can never return nil. You don’t get nil if “allocation wasn’t successful”.

1 Like
#9

Roblox merely attempts to allocate the instance, if it is a local allocation then it is governed by the memory of the host (client) to actually create the object in memory. If code is badly coded, or the operating system running the code has reached it’s memory limit then these allocations can easily fail.

A request is purely that, a request. Roblox servers have no knowledge of the local host, if the host has not the memory to create local object instances then they will not exist. Even though Roblox servers can continue to request them.

1 Like
#10

This makes zero sense. These leaderstats are stored on the server. It doesn’t matter if it successfully replicates to the client or not. If the client doesn’t have enough memory left to receive a few instances, they have bigger problems and they will probably crash soon anyway. If the server doesn’t have enough memory left to create a single new instance, again it has bigger problems and will probably throw an error or shut down.

Instance.new instantly and synchronously creates a new instance. The “request” for a client to create a matching instance is handled separately by the replication layer and can not result in Instance.new returning nil on the server. That’s complete nonsense.

1 Like
#11

The issue is not the server, the issue lies with local allocation. If the memory of the host has reached it’s limit, then it matters not how many requests are made via the server, the objects have no memory to exist where they are being requested to exist.

1 Like
#12

That’s defined behaviour, it will always exist on the server, possibly not on the client.

1 Like
#13

I’ve been scripting for a while now, but I’ve never seen Instance.new() return nil… Even if the map is big and there’s a lot going on in the scripts.

#14

You don’t have any idea what you are talking about. Instance.new will return an Instance as long as it does not throw a error. Yes, it’s true that creating an Instance doesn’t mean it will successfully replicate to all or any clients, but you still created an Instance and it’s not going to randomly be nil. It will exist on the server even if the “local” “allocation” “fails”.

This cannot cause Instance.new to return nil.

1 Like
#15

So therefore using it locally is undefined!

1 Like
#16

No. Instance.new is perfectly defined behavior. If it does not throw an error, an instance was created. It cannot possibly be nil. It does not matter what you say about server or “local” “allocations”. There’s no point in checking if the return value is nil because if you have a return value it’s absolutely guaranteed not to be nil. If the server side is out of memory then it will throw an error and you won’t even get to check if the return value is nil or not because it won’t exist.

1 Like
#17

When anyone starts a sentence with “You don’t have any idea what you are talking about” says to me I have nothing left to say. You are beyond reproach. Continue relying on documented well defined behaviours and never check anything yourself. Good luck with that.

1 Like
#18

I’m not sure what the point is of telling me that I’m “beyond reproach” here. I said you don’t have any idea what you are talking about because you don’t. If that makes you close up and refuse to listen to what I have to say, then so be it.

Yeah, I totally believe that you’ve seen Instance.new return nil with your very own eyes.

1 Like
#19

Whether I have or not, I will never utterly rely on it. It is bad practice to assume anything when it comes to code, clients, and servers.

1 Like
#20

You seem to be assuming that Instance.new can return nil, though. Because “it is bad practice to assume anything”, you assume that infallible operations can fail. How ironic.

Here’s a funny thing: if instance.new returns an object with a metatable, it can override ~= nil to do something ridiculous like throw an error or crash your game. Since you can’t assume anything, you can’t rule out this happening. Maybe you should check for metatables too, or use rawequal.