Review of a Duelling System

Help and Feedback Code Review
#1

Hey, I’m currently working on a system wherein two players are spawned on a map with weapons and they fight to a certain score. I mean, I’m sure I could write a solution for this that works but is hacky. I’m just wondering how you would approach this if you were writing the same thing. Should I set up a “Duel” object which takes two players and the map as an argument and it handles the process from there?

Anyways, here’s my whole function. I know. The entire duel code is in a function. This initially started off as something temporary and I need ideas on how I could do this better. I need to figure out the best way to track who won a round etc.

Here it is, in all of its disgusting glory:

local startDuel = function(player1, player2)
	--it is assumed that player1 and player2 are both not duelling and marked as playing
	
	--remove all of their UI
	network.sendToMany("DisableUIForDuel", {player1, player2})
	
	--remove their characters so nothing weird can happen
	if player1.Character then player1.Character:Destroy() end
	if player2.Character then player2.Character:Destroy() end
	
	--get a random map then spawn it in
	local ranMap = maps[RNG:NextInteger(1, #maps)]
	local map = mapSpawningHandler.spawnMap(ranMap)
	
	--create their duel data
	duelData[#duelData + 1] = {
		player1 = player1,
		player2 = player2,
		map = map
	}
	
	local mapData = map:FindFirstChild("MapData")
	
	--get the clients to process the MapData
	print("on server", mapData)
	network.sendToMany("ProcessMapData", {player1, player2}, mapData)
	
	--tell both clients to do a quick preview of the map
	network.sendToMany("StartMapPreview", {player1, player2}, map)
	
	wait(10) --changed at some point?
	
	network.sendToMany("StopMapPreview", {player1, player2})
	
	--establish spawn locations for players
	local spawn1, spawn2 = map:FindFirstChild("Spawn1"), map:FindFirstChild("Spawn2")
	if not spawn1 or not spawn2 then return end --what the hell
	
	do --clean up the guis on the spawns
		local sg1, sg2 = spawn1:FindFirstChildWhichIsA("SurfaceGui"), spawn2:FindFirstChildWhichIsA("SurfaceGui")
		if sg1 then sg1:Destroy() end
		if sg2 then sg2:Destroy() end
	end
	
	--sort out character data
	local player1Character, player2Character = util.loadCharacterAsync(player1), util.loadCharacterAsync(player2)
	local player1CharData = getCharacterData(player1)
	local player2CharData = getCharacterData(player2)
	if not player1CharData.humanoid or not player1CharData.primaryPart or not player2CharData.humanoid or not player2CharData.primaryPart then
		return
	end
	
	--freeze them in place
	player1CharData.humanoid.WalkSpeed, player1CharData.humanoid.JumpPower = 0, 0
	player2CharData.humanoid.WalkSpeed, player2CharData.humanoid.JumpPower = 0, 0
	
	--spawn them
	player1Character:SetPrimaryPartCFrame(spawn1.CFrame + Vector3.new(0, 6, 0))
	player2Character:SetPrimaryPartCFrame(spawn2.CFrame + Vector3.new(0, 6, 0))
	
	--TODO: interactive countdown ui
	wait(3)
	
	--give walkspeed and jump + swords
	player1CharData.humanoid.WalkSpeed, player1CharData.humanoid.JumpPower = 16, 50
	player2CharData.humanoid.WalkSpeed, player2CharData.humanoid.JumpPower = 16, 50
	
	swordGiver.giveSword(player1)
	swordGiver.giveSword(player2)
	
	--at this point, the duel is underway and we need to somehow start tracking 
end

Any feedback and constructive criticism is greatly appreciated.

Regards,
bricknemesis

2 Likes
#2

Your function looks pretty good. I just don’t like how it doesn’t follow the D.R.Y principle that well at the bottom half of the function.
D.R.Y = Don’t Repeat Yourself

There are a few methods to prevent being repetitive in your code:

  1. Have a separate function to handle the player1 and player2 stuff.
  • Provides a new layer of abstraction + improved readability
  • Easier to debug errors in your code
  1. To go even further from step 1, you can use variadic arguments (…)
  • Could possibly reduce repetitive function calling depending on use case
  • ex.
local function example(...)
    local arguments = {...}

    for index, value in ipairs(arguments) do
       print(index, value)
    end
end

example('playerOne', 'playerTwo')

https://www.lua.org/pil/5.2.html
Constantly packing a table is not a cheap operation

	network.sendToMany("ProcessMapData", {player1, player2}, mapData)
	
	--tell both clients to do a quick preview of the map
	network.sendToMany("StartMapPreview", {player1, player2}, map)
	
	wait(10) --changed at some point?
	
	network.sendToMany("StopMapPreview", {player1, player2})
  • I recommend caching {player1, player2} in a variable to prevent the unnecessary packing.
local packedPlayers = {player1, player2}
1 Like