Help with making this code a bit cleaner

[Konjointed]

Hello, I’ve been trying to work on my match code, and it’s kind of making me annoyed, but I finally came to a working version and I’d like to get some help with how I can maybe make it a bit cleaner.

Code Explination: I’ve decided to use collection service for tagging the players/ai as player1 or player2 as well as what the opponent should be and the reason for using collection service is because it replicates and so on the client all I have to do is CS:GetTagged(localplayer.Name…“Opponent”)[1] (which would be player2). And lastly I’ve used an OOP approach for creating a new match and use loleris replica module to replicate it to the client for whenever something changes such as the match state.

local function SetupCharacter(character)
	if not character then return end
	
	warn("[SMM] Setting up character")
	
	CS:AddTag(character.HumanoidRootPart,"HRPForCam")
	if CS:HasTag(character,"Player1") then CS:AddTag(character,CS:GetTagged("Player2")[1].Name.."Opponent") end
	if CS:HasTag(character,"Player2") then CS:AddTag(character,CS:GetTagged("Player1")[1].Name.."Opponent") end
	
	--might come up with another method instead of using int values
	Utils:MakeTag("Stun",character,"IntValue")
	Utils:MakeTag("Guage",character,"IntValue")
	Utils:MakeTag("Guard",character,"IntValue",1000)		
	
	--TO-DO
	--1) add hurtbox & grabbox 
	
	warn("[SMM] Finished setting up character")
end

function SMM.new()
	warn("[Server Match Manager] Creating a new match")
	
	local instance = {}
	
	local player1 = Players:GetPlayers()[1] or AI.new()
	local player2 = Players:GetPlayers()[2] or AI.new()
	
	CS:AddTag(player1.Character,"Player1")
	CS:AddTag(player2.Character,"Player2")
	
	SetupCharacter(CS:GetTagged("Player1")[1])
	SetupCharacter(CS:GetTagged("Player2")[1])

	--All of this will be getting replicated to the client using the replica module
	instance.stage = StageManager:SpawnStage(nil,true)
	instance.totalRounds = 3
	instance.currentRound = 1
	instance.time = 99
	--instance.playersInMatch = { now that I'm using collection service I don't think I really need to be using this
	--	player1 = player1,
	--	player2 = player2,
	--}
	instance.state = FSM.create({
		initial = "idle",
		
		events = {
			{name = "begin",  from = "*",  to = "begin"},
			{name = "reset",  from = "begin",  to = "reset"},
		},	
		
		callbacks = {
			onenterbegin = function(self, event, from, to, msg)
				warn("[Server Match Manager] Entered Begin State")
				ServStorage.Events.ReplicateUpdate:Fire("MatchTable",{"Value","state","current"}, instance.state.current)
				StageManager:TeleportToStage(player1,player2)
			end,
			
			onenterreset = function(self, event, from, to, msg)
				ServStorage.Events.ReplicateUpdate:Fire("MatchTable",{"Value","state","current"}, instance.state.current)
				warn("[Server Match Manager] Entered Reset State")
			end,
		}
	})
	
	warn("[Server Match Manager] Successfully created a new match")

	return setmetatable(instance,SMM)
end

[FD_BL]

One of the ways that I am seeing is when you use the following;

	instance.stage = StageManager:SpawnStage(nil,true)
	instance.totalRounds = 3
	instance.currentRound = 1
	instance.time = 99

you can get the same achieved with using the following and getting the same exact result;

instance.stage, instance.totalRounds, instance.currentRound, instance.time = StageManager:SpawnStage(nil, true), 3, 1, 99

other than that where you use

	local instance = {}
	
	local player1 = Players:GetPlayers()[1] or AI.new()
	local player2 = Players:GetPlayers()[2] or AI.new()
	
	CS:AddTag(player1.Character,"Player1")
	CS:AddTag(player2.Character,"Player2")
	
	SetupCharacter(CS:GetTagged("Player1")[1])
	SetupCharacter(CS:GetTagged("Player2")[1])

I feel as if instead of directly defining player1 and player 2 every time you could instead pass it directly inside of whe functions you do, but is not directly necessary. Other than that to my knowledge everything else seems well to be.

[tomsterBG]

I don’t find this cleaner, it could be better to just replace the new lines with spaces since the compiler finds no difference and it’ll be safer to read and edit.

Does compressing them in one line like you do make the compiler find everything a bit easier? If so then that’s a performance improvement!.. and I’ve heard comments slow down the compiler even if that’s just minor

[Nonaz_jr]

No, the compiler doesn’t care at all how long or short your lines are. The only ones who care are the people who need to read the script, and long lines are notoriously harder to read, especially if it involves scrolling to the right.

It’s perfectly fine to create extra variables, just so that your lines are shorter. Give them a clear descriptive name, so that anyone looking at the script understands at a glance what is happening there. That’s my advice on programming style.

Note that extra cpu or ram use for such things is completely irrelevant, unless maybe its in a very quickly repeated core loop. No one will ever notice those nanoseconds. What is noticed is the hours of headache trying to read code where many operations are crammed on one line, without any hints from variable names and in-between steps. Future you will thank you for organising your code in a nicely readable way.

[tomsterBG]

I have text wrapping to avoid side scrolling because I like everything to be visible.

Is it better to make a lot of scripts with their own use (<200 lines) or one script that’s bigger (>200 lines), and how should you involve module script libraries to make everything easier?

I can think of one way to improve workflow and it would involve making two modules for server and client use. They would contain all functions and methods as well as the most used loops, variables, etc. I rarely use modules, but their potential is huge especially with oop.

[Nonaz_jr]

That’s a good question, but first I should say that I find this one of the most difficult and important parts of programming. The basic guidelines are simple, and have a huge impact on more complicated projects.

You may have heard that the ‘industry standard’ in roblox games is ‘single script architecture’. That doesn’t mean all your code goes into one script, but rather, that you have only one script (and one localscript) and almost all your code goes into modulescripts which are all orchestrated from your main script.

In general, the best advice is to keep (module)scripts short, anything in them should be related to a single subject. To make this work nicely, you will have to think carefully about your architecture, and organise everything so that you can easily find where a certain functionality happens.

Using modules, there are several options. You can do an OOP approach and define classes and constructors, but you can also simply stick to a more functional programming-like approach and write your modulescripts as simple function libraries. Practically that just means you define a table, and add functions to it with the function name as key. The script (or modulescript) that requires it can then directly call those functions, and pass the arguments that are needed for the function to know about the outside world.

It’s not advised to directly execute code in modulescripts when they are required, and just to only define functions and classes. That way, requiring the module doesn’t ‘do’ anything, until you call the functions inside it.

Again, like every script or modulescript relates to a single subject, every function or method should also be short and do only one or a few closely-related operations. If you find yourself doing a lot in one function, it may be better to split it into multiple functions.

However, it is key to organise all this carefully. Just putting random bits of code in functions doesn’t necessarily help. Naming a function in such a way that it clearly describes the task it is performing makes the script more readable. Naming your function a() and having random operations in it however is probably worse than just having a long list of random operations.

Bottom line is, there is always an even better way to do the same, you will keep learning all your lifetime, and a lot of time on big projects is spent refactoring. Refactoring means you rewrite code that already works, to make it do exactly the same thing, in a better organised structure. First steps of refactoring usually involve taking your quickly-written long spaghetti script, and separating it into different files and functions. Naming everything properly will hopefully give you a nicely readable, intuitive, well organized, maintainable and reusable set of scripts and modules. Naturally, if you were good enough, you would write it like that right away, but as mentioned, most people need to spend a lot of time refactoring because organizing it nicely is very hard and important, and there is always an even better way.

To be clear, at first keeping everything in one long script may seem easy and more manageable, but as your projects gets big and complex, you will find yourself going back and forth through endless lines of code, not finding what you need. The extra organisation level of properly named functions and files is crucial, helps you prevent duplicated code, conceptualizes steps and stages and keeps related code grouped together and unrelated code in a separate location. Eventually, questions of how to organise code and modules will become a major part of your design process, as you learn more about so called ‘code patterns’ and best practices/idioms for particular types of tasks.