Saving data module

Help and Feedback Code Review
#1

thougs on my data module?

local Settings = require(script.settings)	
local RunService = game:GetService('RunService')
local DataStore = game:GetService('DataStoreService'):GetDataStore(Settings.DataKey)
local sav = {}
local Data = {}

function Data.CatchData(p)
	local am = 1
	local r = nil
	repeat am +=1
		local success,result = pcall(function()
			return DataStore:GetAsync(p.UserId..Settings.Key)
		end)
		if (result and success) then
			r=result
			print("successfully catched data.")
			break
		elseif (not result and not success or not success)  then
			if (am >= 3) then
				return
			end
			print(success)
			r = nil
			am += 1
		end
	until result or am >= 3
	if (r) then
		for i,v in pairs(r) do
			if (p:WaitForChild("Stats"):FindFirstChild(i)) then
				if (p.Stats[i]:IsA("ValueBase")) then
					p.Stats[i].Value = v
					sav[p] = r
				
					end
			end
		end
		return r
	end
	return nil
end

function Data.SaveData(p,data)
	
	if (not Settings.SaveInStudio) then
		if (RunService:IsStudio()) then
			return
		end
	end
	print("saving",data,"for",p.Name)
	if (sav[p]) then
		sav[p] = nil
		local am = 1
		repeat
			wait()
			am += 1
		local succ,err = pcall(function()
			DataStore:UpdateAsync(p.UserId..Settings.Key,function(OldData)
				return data
			end)
			end)
		until succ and not err or am >= 3 and warn(err,succ)
	else
		pcall(function()
			DataStore:SetAsync(p.UserId..Settings.Key,data)
			Data.SaveData(p,data)
		end)
	end
end

return Data
3 Likes
#2

You’re code could look a lot better, and there’s some pretty redundant stuff like this.

The main problem I see with your code is your variable naming convention, as someone reading through your code is probably not gonna understand what, ‘r’ or “am” is unless they reading your code throughly to understand what you’re doing.

Your main goal should be fixing your naming convention and getting rid of the redundant things in your code for now.

#3

I don’t really need people ot understand that.

This has a purpose and its clear

#4

But that seems very redundant to me.

You’re checking if there’s no result and if it’s not successful, and also checking if it’s not successful.

You should just change your success check into a if-else block it’s slightly more efficient and it’s pretty clear and easy to read.

#5

No, I am checking if there’s no result and no success, or if no success

#6

An if-else block would be able to do that perfectly fine, and it wouldn’t have to do that.

Because you might as well just do.

if not success or not result
#7

it’s just equal so no need to do it anyway

#8

For the pcall statement, you don’t need to wrap the DataStore.GetAsync in another function. pcall supports passing in arguments:

local success, result = pcall(DataStore.GetAsync, DataStore, p.UserId..Settings.Key)

In this case you would need to pass in DataStore as the second parameter because you are calling a : function as a . function. (AKA the self parameter).

A good read would be this article

1 Like
#9

smartt, didn’t though about that