[WIP] server backend review

Help and Feedback Code Review
1

Hey everyone. I just started using the Rojo framework and have been finding it great so far. I’ve been working on an fps game with perk combos, classes, etc. and I want to see if any of the backend code I’ve written below can be improved.

SERVER
Datastore script

local PlayerService = game:GetService("Players")
local ServerStorage = game:GetService("ServerStorage")
local ReplicatedStorage = game:GetService("ReplicatedStorage")

local ServerEvents = ServerStorage:WaitForChild("serverEvents")
local SharedEvents = ReplicatedStorage.Shared:WaitForChild("events")

local DSS = game:GetService("DataStoreService")
local HTTPService = game:GetService("HttpService")

local userDataStore = DSS:GetDataStore("test10")
local weaponDS = DSS:GetDataStore("weapons2") -- Probably gonan change all this

local blankUserData = require(game.ReplicatedStorage.Shared.templates:WaitForChild("blankData"))
local userData = require(game.ReplicatedStorage.Shared.types:WaitForChild("userData"))

local weaponCache = {}
local MAXIMUM_REPETITIONS = 15

type userData = userData.userData

local function shallowCopy(tbl) 
	local copy = {}
	for k, v in pairs(tbl) do
		copy[k] = v
	end

	return copy
end

local function serializeWeapon(tbl)
	local keys = {}

	for i, _ in pairs(tbl) do
		table.insert(keys, i)
	end

	table.sort(keys, function(a, b) return tostring(a) < tostring(b) end)

	local sorted = {}
	for _, k in ipairs(keys) do
		sorted[k] = tbl[k]
	end

	return HTTPService:JSONEncode(sorted)
end

--[[ Weapon hashing where each hash indicates a unique weapon roll. For example
	{name="AK47", perks={"x", "y"}} will have a different hash than
	{name="AK47", perks={"a","b"}} 

	I'm not sure if this is useful, but it also helps secure the client as the only data the client can access regarding the weapon is their hashes.
--]]
local function getWeaponHash(weapon: string)
        -- Hashing algorithm I found online, ported it here.
	local hash = 5381

	for i = 1, #weapon do
		hash = bit32.band((hash * 33 + string.byte(weapon, i)), 0xFFFFFFFF)
	end

	return string.format("weapon-%08x", hash)
end

-- In the next changes I don't think this function will be here anymore
local function decodeWeapon(v: string)
	if weaponCache[v] then return weaponCache[v] end

	local success, result = pcall(function()
		return weaponDS:GetAsync(v)
	end)

	if success and result then
		weaponCache[v] = result
		return result
	else
		warn("Failed to recover weapon @ hash "..v)
	end
end

-- Hashing a weapon
local function encodeWeapon(weapon: {[any]: any})
	local serialized = serializeWeapon(weapon)
	local hash = getWeaponHash(serialized)

	local _, result = pcall(function()
		return weaponDS:GetAsync(hash)
	end)

	if not result then
		local success2, _ = pcall(function()
			weaponDS:SetAsync(hash, weapon) -- Probably will remove the datastore
		end)

		if not success2 then
			print("Failed to encode weapon with hash "..hash)
		end
	end

	return hash
end

-- Constructing user data from datastore
local function constructData(data: {[any]: any})
    if not data or not data.weapons then return end

    for i, v in pairs(data.weapons) do
		if type(v) == "table" then
			data.weapons[i] = v
			continue
		end
		data.weapons[i] = decodeWeapon(v)
    end

	data.states = {
		reloading = false,
		sprinting = false,
		firing = false,
		aiming = false
	}

	data.sessionID = HTTPService:GenerateGUID(false)

    return data
end

-- Player added logic
PlayerService.PlayerAdded:Connect(function(player)
	local data
	local clientData = {}
	local ProcessedData
	local missedSave = false
	local new = false
	
	local success, result = pcall(function()
		return userDataStore:UpdateAsync("data-"..player.UserId, function(last: userData)
			if not last then last = {} new = true end
			
			-- Checking for error during last save
			if last.lastlogin and last.lastsave then
				if last.lastlogin > last.lastsave then
					missedSave = true
				end
			end
			
			last.lastlogin = DateTime.now().UnixTimestampMillis

			return last
		end)
	end)

	print(missedSave)

	if success and result then
		data = result

		if new then
			ProcessedData = blankUserData()

			clientData.weapons = {}

			for i, v in pairs(ProcessedData.weapons) do
				clientData.weapons[i] = encodeWeapon(v)
			end	
		else
			clientData.weapons = shallowCopy(data.weapons)
			ProcessedData = constructData(data)
		end

		clientData.sessionID = ProcessedData.sessionID
	end
	
	ServerEvents.joined:Fire(player, ProcessedData, clientData, missedSave, new)
end)

-- Leaving logic
ServerEvents.leaving.Event:Connect(function(player, current: userData)
	local serializedWeapons = {}
	for _, v in pairs(current.weapons) do
		table.insert(serializedWeapons, encodeWeapon(v))
	end
	for _=1, MAXIMUM_REPETITIONS, 1 do
		local success, _ = pcall(function()
			userDataStore:UpdateAsync("data-"..player.UserId, function(data: userData)
				data = data or {}
				
				data.weapons = serializedWeapons

				data.stats = current.stats
				data.lastlogin = current.lastlogin
				data.lastsave = DateTime.now().UnixTimestampMillis
				
				print(data)
				return data
			end)
		end)

		if not success then 
			task.wait(0.2)
			continue 
		else
			break
		end
	end
end)

Here is the main server script (So far this only has basic structure):

local ServerStorage = game:GetService("ServerStorage")
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local Players = game:GetService("Players")

local ServerEvents = ServerStorage.serverEvents
local SharedEvents = ReplicatedStorage.Shared.events.server

local Events = ReplicatedStorage.Shared.events

local userData = require(game.ReplicatedStorage.Shared.types:WaitForChild("userData"))
local clientInfo = require(game.ReplicatedStorage.Shared.types:WaitForChild("clientData"))
type userData = userData.userData
type clientInfo = clientInfo.clientData

local players = {}
local clientData = {}

-- Server -> Server logic
ServerEvents.joined.Event:Connect(function(plr: Player, data: userData, ClientData: clientInfo, leak: boolean, new: boolean)
    if players[plr.UserId] then error("Player already found") end
    
    players[plr.UserId] = data
    clientData[plr.UserId] = ClientData

    print("Received data...")
    print(data)

    if new then
        SharedEvents.newPlayer:FireClient(plr)
    end

    Events.server.initClient:FireClient(plr)
end)

-- Client logic
Events.client.fetchData.OnServerInvoke = function(player)
    if not players[player.UserId] then error("Player not in server base") end
    if not clientData[player.UserId] then
        error("Client data not found for player "..player.UserId)
    end

    return clientData[player.UserId]
end

Events.client.decodeWeapon.OnServerInvoke = function(player, index, sessionID)
    if not players[player.UserId] then error("Player data not found in server") end
    if players[player.UserId].sessionID ~= sessionID then
        error("Session ID mismatch for player "..player.UserId)
    end

    if not players[player.UserId].weapons then return nil end
    return players[player.UserId].weapons[index]
end

-- Leaving logic
game:BindToClose(function()
    for _, plr in pairs(Players:GetPlayers()) do
        if not players[plr.UserId] then continue end
        ServerEvents.leaving:Fire(plr, players[plr.UserId])
    end
end)

Players.PlayerRemoving:Connect(function(plr)
    if not players[plr.UserId] then return end

    ServerEvents.leaving:Fire(plr, players[plr.UserId])
    players[plr.UserId] = nil
    clientData[plr.UserId] = nil
end)

SHARED
Services:

  • PlayerService (Still in dev, very basic)
local module = {}
module.__index = module

local ReplicatedStorage = game:GetService("ReplicatedStorage")
local ClientEvents = ReplicatedStorage.Shared.events.client

local CharacterService = require(ReplicatedStorage.Shared.services.CharacterService)

function module.new()
    local self = ClientEvents.fetchData:InvokeServer()

    print(self)

    self.player = game.Players.LocalPlayer
    self.character = CharacterService.new(self)

    self.character:setup()

    return setmetatable(self, module)
end

return module
  • CharacterService
local module = {}

local ReplicatedStorage = game:GetService("ReplicatedStorage")
local Events = ReplicatedStorage.Shared.events.client
local Weapons = ReplicatedStorage.Shared.weapons

type weapon = {
    name: string,
    settings: {[any]: any},
    perks: {string}
}

function module.new(root) 
    local self = {}

    self.character = root.player.Character or root.player.CharacterAdded:Wait()
    self.__index = root

    local combined = {}
    for i, v in pairs(module) do
        combined[i] = v
    end

    for i, v in pairs(root) do
        combined[i] = v
    end

    local meta = {
        __index = combined
    }

    return setmetatable(self, meta)
end

function module:setup() -- Basically just holstering weapons to character model
    local weps = self.weapons :: {[any]: string}

    for i, _ in pairs(weps) do
        local model = Events.decodeWeapon:InvokeServer(i, self.sessionID) :: weapon
        if not model then continue end

        model = Weapons:FindFirstChild(model.name) :: Model
        if not model then continue end

        model = model:Clone()
        model.Parent = self.character

        local rootPart = model.PrimaryPart
        local weld = rootPart:FindFirstChild("holster") :: Motor6D

        weld.Part0 = self.character:FindFirstChild("Torso")
        weld.Part1 = rootPart
    end
end


return module

Templates (Will be used more, only one thing for now)

  • blankData
local HTTPService = game:GetService("HttpService")
local userData = require(game.ReplicatedStorage.Shared.types:WaitForChild("userData"))

type userData = userData.userData
 
return function() : userData
    return {
        weapons = {
            [1] = {name="AK47"},
            [2] = {name="Glock-17"}
        },

        sessionID = HTTPService:GenerateGUID(false),

        states = {
            reloading = false,
            firing = false,
            aiming = false,
            sprinting = false,
        },

        stats = {
            kills = 0,
            deaths = 0,
            level = 0,
            xp = 0,
        },

        lastlogin = DateTime.now().UnixTimestampMillis,
        lastsave = DateTime.now().UnixTimestampMillis
    }
end

Types:

  • clientData
export type clientData = {
    weapons: {[number]: string},
    sessionID: string
}

return nil
  • userData
export type userData = {
    weapons: {[number]: {[any]: any}},
    sessionID: string,

    states: {
        reloading: boolean,
        firing: boolean,
        aiming: boolean,
        sprinting: boolean
    },
    
    stats: {
        kills: number,
        deaths: number,
        level: number,
        xp: number
    },

    lastlogin: number,
    lastsave: number
}

return nil

CLIENT
Main client script:

repeat task.wait() until game:IsLoaded()

local ReplicatedStorage = game:GetService("ReplicatedStorage")
local plr = game.Players.LocalPlayer
local camera = workspace.CurrentCamera
local character = plr.Character or plr.CharacterAdded:Wait()

local events = ReplicatedStorage.Shared:WaitForChild("events")

local PlayerService = require(ReplicatedStorage.Shared.services.PlayerService)

-- PlayerService logic
events.server.initClient.OnClientEvent:Connect(function()
    local handler = PlayerService.new()
end)

-- New player logic, not finished
events.server.newPlayer.OnClientEvent:Connect(function()
    print("Received request from server")
    camera.CameraType = Enum.CameraType.Scriptable
    camera.CFrame = workspace.newPlayer:WaitForChild("camPart").CFrame
end)

Input Script:

local CAS = game:GetService("ContextActionService")
local binds = {
    [Enum.KeyCode.One] = 1,
    [Enum.KeyCode.Two] = 2
}

local function equip(_, state: Enum.UserInputState, inputObject: InputObject)
    if state == Enum.UserInputState.Begin then
        print("Keybind: ", binds[inputObject.KeyCode])
    end
end

for i, v in pairs(binds) do
    CAS:BindAction("Equip"..v, equip, false, i)
end

Haven’t ever posted on code review, so please let me know if I did anything wrong! Sorry if this is long, just wanted other perspectives to improve my code. I appreciate the feedback!

2 Likes
2

Bumping, also, I have updated the code to remove the weaponDS and instead just using hashing purely for client security. The entire weapon tables will now be stored in the user data store, not the hashes

3

Hi, @BaconHair16773.

I think what is missing is removing pairs you don’t need for them to be in for loop, it’s optional and was created for some other feature and also wastes some performance (see it’s bytecode).

-- you can do this
for i,v in (tablehere) do

end
2 Likes
4

Wait, this works? I had no clue, thanks a lot! Thought lua didn’t support this

2 Likes
5

Took a bit of a look-through. I second @Microlosofto’s recommendation; for me, I tend to eschew pairs and ipairs just because it makes my code more concise. I’ve always structured my tables in such a way that they are either numerical indices only, or strong indices only, so I’ve never had need to distinguish them.

I only really have nitpicks to offer, but here goes.

  • Within userDataStore:UpdateAsync("data-"..player.UserId, the added "data-" isn’t necessary; you can just have the UserId be the key. Not really a big deal here, but with more complex datastore operations details like these can add up a bit.
  • Regarding the line for _=1, MAXIMUM_REPETITIONS, 1 do, the third argument for the for loop (the step) is not needed, as 1 is the default. Secondarily, the constant MAXIMUM_REPETITIONS would be more aptly named as something like MAX_UPDATE_ATTEMPTS. Particularly, for constants, you want your variable names to be as nicely descriptive as possible while still remaining concise.
  • There seems to be a bit of case-inconsistency in the naming of variables/fields. For example, there’s encodeWeapon and blankUserData, but there’s also lastlogin and lastsave. It’s a tiny bit of an eyesore.
  • weaponDS should be named weaponDataStore as it matches the naming of userDataStore; weaponDS is also more prone to misreading (“weapons”, “weapon5s”, etc.)
1 Like
6

Thanks for the feedback! I actually ended up going through with @Microlosofto’s idea. I’ll also change up my code to incorporate these practices! Thanks a ton for the the feedback. I ended up just removing weaponDS, as storing the hashes in the datastore isn’t really practical anyways, because I don’t think I’m hitting the 1MB limit anytime soon. Other than that, I’ll look around and try to incorporate these changes as much as I can, thanks!

1 Like