Tales of Eloriara - Code Review

[Xovain]

I’m working on a combat game and am busy setting up all the core systems I’m going to need, however I feel like there are much better ways and practices to do this.

The attached game consists of a server script and a few modules, I am primarily concerned with the server script but would also appreciate feedback on the modules. I am relatively unexperienced so again all feedback is highly appreciated.
Tales of Eloriara.rbxl (71.5 KB)

[LividDreaming (fish)]

Looks good to me, only thing I saw was on line 7 of the NameGenerator module it has a typo and because you reference the CharacterData table so much I would make it its own variable, personally I think it would make your scripts look neater and it could save some work if you ever reformat your data structure.

I would also add something to handle when data fails to save but I assume that’s in the works

[Xovain]

I appreciate your insight and will implement it shortly, thank you!

[InfiniteYield (infinite)]

this is nitpicky so don’t feel like its some grand problem or anything

1. the : on all of your module functions is redundant. its a function, not a method, so it needs to be a . . methods provide the self argument, which isn’t used here and thus can be confusing to another coder. also theres a typo, as Livid pointed out.
2. storing the entire Service of game.ReplicatedStorage in MasterRaceTable seems kinda redundant, especially if youre not gonna let it cache with :GetService() by indexing it so many times. make it a variable.
3. start using some typechecking to help yourself and other programmers. things like

function SpawnHandler:SpawnCharacter(player, datatable, Character)

can be

function SpawnHandler:SpawnCharacter(player : Player, datatable : {[number] : any}, Character : Model)

so you can know what its supposed to be. especially with datatable, which is inherently confusing from an outside perspective as to what it is and whats inside it and what those things are supposed to be.
4.

	if success and Data then
		LivePlayerData[UUID]["CharacterData"]["MaleName"] = Data.MaleName
		LivePlayerData[UUID]["CharacterData"]["FemaleName"] = Data.FemaleName
		LivePlayerData[UUID]["CharacterData"]["GuildName"] = Data.GuildName
		LivePlayerData[UUID]["CharacterData"]["Currency"] = Data.Currency
		LivePlayerData[UUID]["CharacterData"]["Health"] = Data.Health
		LivePlayerData[UUID]["CharacterData"]["Lives"] = Data.Lives
		LivePlayerData[UUID]["CharacterData"]["Alignment"] = Data.Alignment
		LivePlayerData[UUID]["CharacterData"]["ManaAbilities"]["ManaSpeed"] = Data.ManaAbilities_ManaSpeed
		LivePlayerData[UUID]["CharacterData"]["ManaAbilities"]["ManaRun"] = Data.ManaAbilities_ManaRun
		LivePlayerData[UUID]["CharacterData"]["ManaAbilities"]["ManaClimb"] = Data.ManaAbilities_ManaClimb
		LivePlayerData[UUID]["CharacterData"]["ManaAbilities"]["ManaClimbSpeed"] = Data.ManaAbilities_ManaClimbSpeed
		LivePlayerData[UUID]["CharacterData"]["ManaAbilities"]["ManaPunch"] = Data.ManaAbilities_ManaPunch
		LivePlayerData[UUID]["CharacterData"]["ManaAbilities"]["HasMana"] = Data.ManaAbilities_HasMana
		LivePlayerData[UUID]["CharacterData"]["Race"]["RaceType"] = Data.Race_RaceType
		LivePlayerData[UUID]["CharacterData"]["Race"]["RaceVar"] = Data.Race_RaceVar
		LivePlayerData[UUID]["CharacterData"]["SpawnData"]["SpawnPoint"] = Data.SpawnPoint
		LivePlayerData[UUID]["CharacterData"]["SpawnData"]["SavedPosition"] = Data.SpawnPosition

	else
		warn(player.Name.. " HAS NO DATA! / DATA DD NOT SAVE! ( NEW USER? )", err)

	end

you should differentiate your warning/failsafe detection between “they dont have data” versus “the datastore service is literally down and we’re hopeless”, kind of a big difference
5. stop calling player.UserId ‘UUID’, that is not the same thing, that term has a meaning and its even generatable in Roblox via HttpService.
6. make your own constructor for stuff like this, mainly because its ugly, also its pointless to define an index as nil.

ActionBarData = {Slot1 = nil, Slot2 = nil, Slot3 = nil, Slot4 = nil, Slot5 = nil, Slot6 = nil, Slot7 = nil, Slot8 = nil, Slot9 = nil, Slot10 = nil, Slot11 = nil}

as in like

local function construct(max : number, value : any): {[number] : any}
	local t = {}
	for i = 1, max do
		t["Slot" .. tostring(i)] = value
	end
	return t
end

ActionBarData = construct(11, false) -- sets every value to false

[Xovain]

Hey, this is exactly the stuff I was looking for and honestly is so helpful, I really appreciate the response! Some of what you mentioned is just stuff I left in cause I am in mid development so I apologize for that, my fault haha, also I didn’t know about the : part, so I’ll correct and and same with the misleading UUID. I’ve actually not looked into typechecking before so I’l be reading up all about that tonight.

Again I really appreciate this and am glad you took the time to point this all out to me.

[Xovain]

Question, In my functions I pass through " datatable " which is simply just the users data in the “LivePlayerDataTable” so the function can read and react to it as needed, is there a better way of doing this?