Notes Module (Code Feedback)

Help and Feedback Code Review
, ,
#1

Hello Code-Reviewers!
I’m currently working on a module that will be released inside of Community-Resources.

Right now I’m working on a single module for my project that handles long term storage of player-notes.

I’m looking for code feedback, possible improvements and anything that might look out of place or scuffed.
If you could leave me a comment with a method or way to improve this module please let me know, that would be nice.

  • Thanks BugleBoy/Pingu (Braiden)

Notes Module - 151 lines

--[[
 _______             __                       __                       ______   _______  ______ 
|       \           |  \                     |  \                     /      \ |       \|      \
| $$$$$$$\  ______   \$$ __     __  ______  _| $$_     ______        |  $$$$$$\| $$$$$$$\\$$$$$$
| $$__/ $$ /      \ |  \|  \   /  \|      \|   $$ \   /      \       | $$__| $$| $$__/ $$ | $$  
| $$    $$|  $$$$$$\| $$ \$$\ /  $$ \$$$$$$\\$$$$$$  |  $$$$$$\      | $$    $$| $$    $$ | $$  
| $$$$$$$ | $$   \$$| $$  \$$\  $$ /      $$ | $$ __ | $$    $$      | $$$$$$$$| $$$$$$$  | $$  
| $$      | $$      | $$   \$$ $$ |  $$$$$$$ | $$|  \| $$$$$$$$      | $$  | $$| $$      _| $$_ 
| $$      | $$      | $$    \$$$   \$$    $$  \$$  $$ \$$     \      | $$  | $$| $$     |   $$ \
 \$$       \$$       \$$     \$     \$$$$$$$   \$$$$   \$$$$$$$       \$$   \$$ \$$      \$$$$$$

				Author: Roblox: NotBugle | Discord: pingulovesyou
				Last Modified: 7/8/2023
--]]


-- // Services \\ -- 
local DataStoreService			= 			game:GetService("DataStoreService")
local HttpService				=			game:GetService("HttpService")
local RunService				=			game:GetService("RunService")
local Players 					= 			game:GetService("Players")


-- // Consants \\ --
local PrivateAPI_DataStore		= 			DataStoreService:GetDataStore("PrivateAPI")


-- // Module \\ -- 
local PrivateAPI 				= 			{}
local PrivateAPI_DataTemplate 	= 			{Notes = {}} -- Can be updated with more values later aka {Notes = {}, Bans = {}}


function PrivateAPI.AddNote(Player: Player, Title: string, Note: string)
	local KnownData
	local OperationalSuccess
	local NoteID = HttpService:GenerateGUID(false)
	local DataTemplate = PrivateAPI_DataTemplate

	local Success,Failure = pcall(function()KnownData=PrivateAPI_DataStore:GetAsync(Player.UserId)end)
	
	if Success and KnownData then
		-- // Data was already stored for this user, lets update it with the new note and set OperationalSuccess based off of Datastore.
		KnownData.Notes[NoteID] = {}
		KnownData.Notes[NoteID].Title = Title
		KnownData.Notes[NoteID].Note = Note
		KnownData.Notes[NoteID].Timestamp = os.time()

		OperationalSuccess = PrivateAPI_DataStore:SetAsync(
			Player.UserId,
			KnownData
		)
	else
		-- // Data was not found for this user, lets create it with the note and set OperationalSuccess based off of Datastore.
		DataTemplate.Notes[NoteID] = {}
		DataTemplate.Notes[NoteID].Title = Title
		DataTemplate.Notes[NoteID].Note = Note
		DataTemplate.Notes[NoteID].Timestamp = os.time()
		
		OperationalSuccess = PrivateAPI_DataStore:SetAsync(
			Player.UserId,
			DataTemplate
		)
	end
	
	return {Success,OperationalSuccess,KnownData}
end


function PrivateAPI.RemoveNote(Player: Player, NoteID: string, Bulk: boolean)
	local KnownData
	local OperationalSuccess
	local DataTemplate = PrivateAPI_DataTemplate

	local Success,Failure = pcall(function()KnownData=PrivateAPI_DataStore:GetAsync(Player.UserId)end)
	
	if Bulk then
		if Success and KnownData then
			-- // Bulk option was true, destory every note for the user and set OperationalSuccess to true to let the user know all the data was removed.
			for i,v in KnownData.Notes do
				KnownData.Notes[i] = nil
				OperationalSuccess = true
			end

			PrivateAPI_DataStore:SetAsync(
				Player.UserId,
				KnownData
			)
		else
			-- // Data could not located, create data for the user and set OperationalSuccess to false to let the user know the data was not removed.
			PrivateAPI_DataStore:SetAsync(
				Player.UserId,
				DataTemplate
			)
			OperationalSuccess = false
		end
	else
		if Success and KnownData then
			-- // Data was located, destory data for the user and set OperationalSuccess to true to let the user know the data was removed.
			if KnownData.Notes[NoteID] then
				KnownData.Notes[NoteID] = nil
				OperationalSuccess = true
			else
				OperationalSuccess = false
			end

			PrivateAPI_DataStore:SetAsync(
				Player.UserId,
				KnownData
			)
		else
			-- // Data could not located, create data for the user and set OperationalSuccess to false to let the user know the data was not removed.
			PrivateAPI_DataStore:SetAsync(
				Player.UserId,
				DataTemplate
			)
			OperationalSuccess = false
		end
	end
	
	return {Success,OperationalSuccess,KnownData}
end


function PrivateAPI.FindNote(Player: Player, NoteID: string, Bulk: boolean)
	-- // This feels a bit scuffed...
	-- // I feel like I need a better method of doing/checking OperationalSuccess.
	local KnownData
	local LocatedDara
	local OperationalSuccess
	
	local Success,Failure = pcall(function()KnownData=PrivateAPI_DataStore:GetAsync(Player.UserId)end)
	
	OperationalSuccess = false
	
	if Bulk then
		if Success and KnownData then
			OperationalSuccess = true
		end
	else
		for i,v in KnownData.Notes do
			if i == NoteID then
				KnownData = KnownData.Notes[i]
				OperationalSuccess = true
				break
			end
		end
	end
	
	return {Success,KnownData}
end


return PrivateAPI

Private_API.lua (5.0 KB)

#2

You can save all current note ids for a player so you can pull up all notes and sort them by a filter.
ideally you would also want to have guis for this as well so you can easily make notes, and for sharing it could be worth having another store similar to that so players dont need to write down note ids.

#3

You could clean up the lines that read as

local Success,Failure = pcall(function()KnownData=PrivateAPI_DataStore:GetAsync(Player.UserId)end)

By doing this instead:

local Success, KnownData = pcall(PrivateAPI_DataStore.GetAsync, PrivateAPI_DataStore, Player.UserId)

You could utilize UpdateAsync instead of SetAsync. You should also look into retries incase any of the datastore operations fail, you can automatically retry.

I also found a good tutorial that explains and walks you through some good practices when it comes to datastores. I’m not sure if all of it is applicable in this use case, but it’s definitely still worth the read.