How can I improve this function?

Help and Feedback Code Review
#1

After a long time of work, I am finally ready to release my ProfileService wrapper; but theres one thing that bothers me. The function I am going to share below seems (to me) very long and not clean at all. Could you please tell me how to fix it?

function DataStoreObject:LoadDataAsync(player: Player, reconcileData: boolean?, claimedHandler: (placeId: number, gameJobId: string) -> () | "ForceLoad" | "Cancel"?)
	reconcileData = OptionalParam(reconcileData, true)
	claimedHandler = OptionalParam(claimedHandler, "ForceLoad")

	local PlayerDataObject = setmetatable({ }, {__index = PlayerDataObject})
	local PlayerData = self.DataStore:LoadProfileAsync(string.format(Settings.KeyStringPattern, player.UserId), claimedHandler)
	
	local Created = os.time()

	self.SessionLockClaimed:Fire(player)
	
	PlayerDataObject.LoadedAt = Created

	PlayerDataObject.Name = self.Name :: string
	PlayerDataObject.Player = player :: Player
	PlayerDataObject.Loaded = false :: boolean

	PlayerDataObject.KeyChanged = Signal.new() :: ScriptSignal
	PlayerDataObject.KeyAdded = Signal.new() :: ScriptSignal
	PlayerDataObject.KeyRemoved = Signal.new() :: ScriptSignal
	PlayerDataObject.GlobalKeyAdded = Signal.new() :: ScriptSignal

	task.defer(function()
		if PlayerData then
			PlayerData:AddUserId(player.UserId)

			if reconcileData then
				PlayerData:Reconcile()
			end

			PlayerData:ListenToRelease(function()
				LoadedPlayers[self.Name][player] = nil
				
				setmetatable(PlayerDataObject, nil)
				table.clear(PlayerDataObject)

				player:Kick(`Could not save data for user {player.UserId}; possible data corruption. Retry later.`)
			end)

			if player:IsDescendantOf(Players) then
				LoadedPlayers[self.Name][player] = PlayerData
				
				for _, globalKey in ipairs(PlayerData.GlobalUpdates:GetActiveUpdates()) do
					PlayerData.GlobalUpdates:LockActiveUpdate(globalKey[1])
				end
				
				PlayerData.GlobalUpdates:ListenToNewActiveUpdate(function(id, data)
					PlayerData.GlobalUpdates:LockActiveUpdate(id)
				end)
				
				PlayerData.GlobalUpdates:ListenToNewLockedUpdate(function(id, data)
					PlayerDataObject.GlobalKeyAdded:Fire(data.key_type, data.sent_data)
					PlayerData.GlobalUpdates:ClearLockedUpdate(id)
				end)
				
				PlayerDataObject.Loaded = true
			else
				PlayerData:Release()
			end
		else
			player:Kick(`Could not load data for user {player.UserId}; retry later.`)
			console.warn(`Could not load data for user {player.UserId}; retry later.`)
		end
	end)
	
	repeat task.wait() until PlayerDataObject.Loaded

	return PlayerDataObject
end

In the latter, I am using string interpolation. The main problem I have is all those pesky indexes and the not so neat code overall.

#2

Your code is actually fairly neat and clean in my opinion. One thing though, instead of using repeat task.wait() you can just use the signal module you’re using instead of repeat task.wait()

Code:

function DataStoreObject:LoadDataAsync(player: Player, reconcileData: boolean?, claimedHandler: (placeId: number, gameJobId: string) -> () | "ForceLoad" | "Cancel"?)
	reconcileData = OptionalParam(reconcileData, true)
	claimedHandler = OptionalParam(claimedHandler, "ForceLoad")

	local PlayerDataObject = setmetatable({ }, {__index = PlayerDataObject})
	local PlayerData = self.DataStore:LoadProfileAsync(string.format(Settings.KeyStringPattern, player.UserId), claimedHandler)

	local Created = os.time()

	self.SessionLockClaimed:Fire(player)

	PlayerDataObject.LoadedAt = Created

	PlayerDataObject.Name = self.Name :: string
	PlayerDataObject.Player = player :: Player
	PlayerDataObject.Loaded = false :: boolean

	PlayerDataObject.KeyChanged = Signal.new() :: ScriptSignal
	PlayerDataObject.KeyAdded = Signal.new() :: ScriptSignal
	PlayerDataObject.KeyRemoved = Signal.new() :: ScriptSignal
	PlayerDataObject.GlobalKeyAdded = Signal.new() :: ScriptSignal
	
	local loadedEvent = Signal.new()

	task.defer(function()
		if not PlayerData then
			player:Kick(`Could not load data for user {player.UserId}; retry later.`)
			console.warn(`Could not load data for user {player.UserId}; retry later.`)
			
			return
		end
		
		PlayerData:AddUserId(player.UserId)

		if reconcileData then
			PlayerData:Reconcile()
		end

		PlayerData:ListenToRelease(function()
			LoadedPlayers[self.Name][player] = nil

			setmetatable(PlayerDataObject, nil)
			table.clear(PlayerDataObject)

			player:Kick(`Could not save data for user {player.UserId}; possible data corruption. Retry later.`)
		end)
		
		if not player:IsDescendantOf(Players) then
			PlayerData:Release()
			
			return
		end
		
		LoadedPlayers[self.Name][player] = PlayerData

		for _, globalKey in ipairs(PlayerData.GlobalUpdates:GetActiveUpdates()) do
			PlayerData.GlobalUpdates:LockActiveUpdate(globalKey[1])
		end

		PlayerData.GlobalUpdates:ListenToNewActiveUpdate(function(id, data)
			PlayerData.GlobalUpdates:LockActiveUpdate(id)
		end)

		PlayerData.GlobalUpdates:ListenToNewLockedUpdate(function(id, data)
			PlayerDataObject.GlobalKeyAdded:Fire(data.key_type, data.sent_data)
			PlayerData.GlobalUpdates:ClearLockedUpdate(id)
		end)
		
		loadedEvent:Fire()
		PlayerDataObject.Loaded = true
	end)
	
	loadedEvent:Wait()

	return PlayerDataObject
end
1 Like
#3

Alright thanks for the tip! I’ll be marking this as a solution. Any other tips?

#4

It wasn’t too bad here but you should use some guard clauses if you have many overlapping if statements.

#5

What are those? (Guard clauses)

#6

Found this explanation: Improve your code using guard clauses: refactor nested conditionals #shorts - YouTube

1 Like
#7

This topic was automatically closed 14 days after the last reply. New replies are no longer allowed.