Hello, I’m working on a turn-based RPG battle system thing and I have this really big function and for the most part, it works the way I want it to, but I wanna see if I could maybe shorten it and improve it? If anyone feels like giving me a hand :s
Basically, the first 2 for loops check if the players action value inside the playersInBattle table isn’t " " and then I know they’re ready, and then after that, we do the same for the enemiesInBattle as well with selecting a target for the enemy. In the next part, the loop saves their spot so I can move then back to it and I basically just move them to the enemy they selected and back to their spot and then the same for the enemy I essentially just use the same code twice once for the players and second for the enemies.
function startTurn(battleName)
local turnFinished = false
local availableTargets = {} --will be used for enemy targeting
local isReady = 0
while wait() do
for i,v in pairs(activeBattles[battleName].playersInBattle) do
if i ~= "numOfPlayers" then
table.insert(availableTargets,i)
if v.action ~= "" then
isReady +=1
end
end
end
for i,v in pairs(activeBattles[battleName].enemiesInBattle) do
if i ~= "numOfEnemies" then
activeBattles[battleName].enemiesInBattle[i].action = "Attack" --always make the enemy attack (for now at least)
local value = math.random(1,#availableTargets)
local picked_value = availableTargets[value]
activeBattles[battleName].enemiesInBattle[i].target = picked_value
if v.action ~= "" then
isReady +=1
end
end
end
availableTargets = {} --reset the table (although I think it already resets the next this functions runs oof)
if isReady == activeBattles[battleName].playersInBattle.numOfPlayers+activeBattles[battleName].enemiesInBattle.numOfEnemies then
print("Everyone is Ready")
--Players Turn
for name,_ in pairs(activeBattles[battleName].playersInBattle) do --players first
if game.Players:FindFirstChild(name) then
local humanoid = workspace:FindFirstChild(name).Humanoid --their humanoid
local theirSpot --will be used to move the character back to their spot
--gets their spot
for i,v in pairs(workspace.Battles[battleName]["Friendly Taken"]:GetChildren()) do
if v:FindFirstChild("Taken By").Value == name then
theirSpot = v
end
end
local target = activeBattles[battleName].playersInBattle[name].target --the players current target
if workspace.Enemies[target].Humanoid.Health ~= 0 and humanoid.Health ~= 0 then
humanoid:MoveTo(workspace.Enemies[target].HumanoidRootPart.Position) --moves player to enemy
humanoid.MoveToFinished:Wait()
workspace.Enemies[target].Humanoid:TakeDamage(40)
humanoid:MoveTo(theirSpot.Position) --move them back
--character.MoveToFinished:Wait()
else
warn("Target has been killed")
end
--repeat for the next players until everyone has finished.
end
spawn(function()
repeat wait() until turnFinished == true
if name ~= "numOfPlayers" then
if workspace[name]:FindFirstChild("In Battle") then
test:FireClient(game.Players[name])
end
end
turnFinished = false
end)
end
wait(2)
--Enemies Turn
for name,_ in pairs(activeBattles[battleName].enemiesInBattle) do
if workspace.Enemies:FindFirstChild(name) then
local humanoid = workspace.Enemies:FindFirstChild(name).Humanoid --their humanoid
local theirSpot --will be used to move the character back to their spot
--gets their spot
for i,v in pairs(workspace.Battles[battleName]["Enemy Taken"]:GetChildren()) do
if v:FindFirstChild("Taken By").Value == name then
theirSpot = v
end
end
local target = activeBattles[battleName].enemiesInBattle[name].target --the enemys current target
if workspace[target].Humanoid.Health ~= 0 and humanoid.Health ~= 0 then
humanoid:MoveTo(workspace[target].HumanoidRootPart.Position) --moves player to enemy
humanoid.MoveToFinished:Wait()
workspace[target].Humanoid:TakeDamage(25)
humanoid:MoveTo(theirSpot.Position) --move them back
--character.MoveToFinished:Wait()
else
warn("Target has been killed")
end
--repeat for the next enemies until everyone has finished.
end
end
turnFinished = true
end
break
end
end
why are u inserting all of that code in a function when you aren’t even returning anything. Also please don’t do while wait() do. Its considered bad practice. You should always do while true do and wait the amount u want to in there. One last thing, using wait() is a bad idea. You should always do runservice.Heartbeat:Wait() as an alternative since its much faster and better performance wise.
This while loop is extremely expensive and could be replaced by events and other things. Try thinking about how many times you want things refreshed to accuracy than getting it done without consideration. If it doesn’t improve performance, it’s just readability(debugging can be difficult).
Replacing wait() with RunService.Stepped:Wait() or any other event in RunService is recommended.
You should localize activeBattles[battleName] as a variable if you frequently use it in the code.
This part you can take out, the table does reset on its own because the function defines the variable anew.
availableTargets = {} --reset the table (although I think it already resets the next this functions runs oof)
This is a horrible practice:
local humanoid = workspace.Enemies:FindFirstChild(name).Humanoid
Using :FindFirstChild means the possibility of getting nil so you are risking indexing nil to find Humanoid. Either index the name normally if it will always exist or check if the child exists first before finding Humanoid.
Another example of poor FindFirstChild usage:
v:FindFirstChild("Taken By").Value
This would be considered bad practice below as wait() is generally not good to use in a loop. It is not a constant yield.
Also, if you need turnFinished = true for this spawned function to run, you could just put the function’s code after the line of code where you set turnFinished to true. Although turnFinished would become useless due to this but it seems this boolean is yield-based anyways so this would make the most sense.
repeat wait() until turnFinished == true
Is there a reasoning to using a while loop in the first place? It seems useless because it eventually breaks no matter what.
Yeah there were few parts of this where I wasn’t sure what to do and so I had just kind of threw together some random stuff together and hope it works for example the spawn part I had no idea what I was doing but I just needed away to like enable the battlegui for the player so they can take their next turn. For using the while loop though I’m actually not sure why I used it and haven’t actually tested what happens if I removed it I think I might have added it because activeBattles is constantly changing as a new battle is added i to it everytime a battle starts and then the table for the battle it self also always changes as players and enemies are able to join a battle by walking into it and so I need to like I guess make sure I have an updated version as the for loop will only run once.
Hello, I took the time to re-write your “function” with critisim.
function startTurn(battleName)
local turnFinished = false
local availableTargets = {} --will be used for enemy targeting
local isReady = 0
local playersInBattle = activeBattles[battleName].playersInBattle -- You have a lot of extraneous indexing in your loops. Consider allocating variables to reduce indexing.
local enemiesInBattle = activeBattles[battleName].enemiesInBattle
while wait() do
for i,v in pairs(playersInBattle) do
if i ~= "numOfPlayers" then
--table.insert(availableTargets,i)
availableTargets[#availableTargets + 1] = i -- consider doing this to append instead since your inside nested loops.
if v.action ~= "" then
isReady +=1
end
end
end
for i,v in pairs(enemiesInBattle) do
if i ~= "numOfEnemies" then
enemiesInBattle[i].action = "Attack"
local value = math.random(1,#availableTargets)
local picked_value = availableTargets[value]
enemiesInBattle[i].target = picked_value
if v.action ~= "" then
isReady +=1
end
end
end
availableTargets = {}
if isReady == playersInBattle.numOfPlayers + enemiesInBattle.numOfEnemies then
print("Everyone is Ready")
--Players Turn
for name,_ in pairs(playersInBattle) do --players first
if game.Players:FindFirstChild(name) then
local humanoid = workspace:FindFirstChild(name).Humanoid
local theirSpot
--gets their spot
for _,v in ipairs(workspace.Battles[battleName]["Friendly Taken"]:GetChildren()) do -- consider using ipairs as the table your traversing as non numerical indices.
-- also consider saving up scope by replacing variables you dont need with _.
--if v:FindFirstChild("Taken By").Value == name then
-- theirSpot = v
--end
theirSpot = v:FindFirstChild("Taken by").Value == name and v -- You have a lot of nested if statements, consider lua idioms as they will improve performance and readability in some cases.
end
local target = playersInBattle[name].target --the players current target
local EnemiesTarget = workspace.Enemies[target]
--if workspace.Enemies[target].Humanoid.Health ~= 0 and humanoid.Health ~= 0 then
-- humanoid:MoveTo(workspace.Enemies[target].HumanoidRootPart.Position) --moves player to enemy
-- humanoid.MoveToFinished:Wait()
-- workspace.Enemies[target].Humanoid:TakeDamage(40)
-- humanoid:MoveTo(theirSpot.Position) --move them back
-- --character.MoveToFinished:Wait()
--else
-- warn("Target has been killed")
--end
EnemiesTarget = ( EnemiesTarget.Humanoid.Health ~= 0 and humanoid.Health ~= 0 and EnemiesTarget ) or warn("Target has been killed") -- Reduce your if statements, your nested style can impact readability and performance.
humanoid:MoveTo(workspace.Enemies[target].HumanoidRootPart.Position)
humanoid.MoveToFinished:Wait()
workspace.Enemies[target].Humanoid:TakeDamage(40)
humanoid:MoveTo(theirSpot.Position)
end
spawn(function() -- wont talk much about spawn, contriversial topic.
repeat wait() until turnFinished == true
--if name ~= "numOfPlayers" then
-- if workspace[name]:FindFirstChild("In Battle") then
-- test:FireClient(game.Players[name])
-- end
--end
test = ( name ~= "numOfPlayers" and workspace[name]:FindFirstChild("In Battle") ) and test:FireClient(game.Players[name])
turnFinished = false
end)
end
wait(2)
for name,_ in pairs(enemiesInBattle) do
if workspace.Enemies:FindFirstChild(name) then
local humanoid = workspace.Enemies:FindFirstChild(name).Humanoid
local theirSpot
for _,v in ipairs(workspace.Battles[battleName]["Enemy Taken"]:GetChildren()) do -- You dont need i, clean up your scope. Also use ipairs here
--if v:FindFirstChild("Taken By").Value == name then
-- theirSpot = v
--end
theirspot = v:FindFirstChild("Taken by").Value == name and v; -- reduce your if statement, consider idioms here specially cause of the loop.
end
local target = activeBattles[battleName].enemiesInBattle[name].target
--if workspace[target].Humanoid.Health ~= 0 and humanoid.Health ~= 0 then
-- humanoid:MoveTo(workspace[target].HumanoidRootPart.Position)
-- humanoid.MoveToFinished:Wait()
-- workspace[target].Humanoid:TakeDamage(25)
-- humanoid:MoveTo(theirSpot.Position) --move them back
-- --character.MoveToFinished:Wait()
--else
-- warn("Target has been killed")
--end
-- same concept here to be applied from line 65
end
end
turnFinished = true
end
break
end
end
you need to consider allocating variables and reduce indexing specially cause your in nested loops. You shouldn’t be shy to trade off space complexity for run time complexity. You also need to consider limiting your scope, you have unneeded variables. I would also encourge you to not use (while wait() do), instead use this runservice.Heartbeat:Wait(). Technically this would not be called a function. Its a subroutine, your not really returning anything. Theres still a lot of more problems but it would lead into your semantics.