- Issue has been resolved -

Help and Feedback Code Review
#1

Issue has been resolved. Original post has been removed and replaced. Thank you for the help!

Edited on January 25, 2020. Reason: Targeted

1 Like
#2

You could always just have one StringReturn function then take in the number as a parameter and have the targets in a table. That may shorten your code quite a bit.

1 Like
#3

Instead of representing each teleport point as a variable, create a table that contains each one in order.

local Targets = {
     workspace:WaitForChild("Misc"):WaitForChild("TPfloors"):WaitForChild("TPsec7"),
     workspace:WaitForChild("Misc"):WaitForChild("TPfloors"):WaitForChild("TPsec1"),
     workspace:WaitForChild("Misc"):WaitForChild("TPfloors"):WaitForChild("TPsec2"),
     etc...
}

Then you can do:

function module.Teleport(player, index, button)
   local Target = Targets[index]
   player.Character:SetPrimaryPartCFrame(Target.CFrame*CFrame.new(0, 3, 0))
   
   for i = 2, 1, -1 do
      button.Text = i
      wait(1)
   end

   button.Text = button.Name
end

Now, to teleport all you have to do is:

local module = require(...)

module.Teleport(MyPlayer, 1, MyButton) -- 2nd paramater goes from 1 to the number of targets
4 Likes
#4

you can put all the targets into an array and make a single function that acts on a target its given, then feed the entire array to that function using a loop.

If these are in a folder, you can use folder:GetChildren()

4 Likes
#6

If you have an error you need to show which line of code it errors on. I don’t understand the problem with the button? I included the button code you had.

#7

The code works completely fine, he simply wants feedback on how he could optimize it. Just because a piece of code works dosen’t mean it can’t be improved.

#8

I wasn’t trying to fix anything, I gave him a different way of achieving what he needs

#10

You can just have this:

local module = {}
local tpFloors = workspace:WaitForChild("Misc"):WaitForChild("TPfloors")

for i = 1, 8 do
	local target = tpFloors:WaitForChild("TPsec"..i)
	
	module["StringReturn" ..i] = function(button, player)
		 player.Character:SetPrimaryPartCFrame(target.CFrame * CFrame.new(0, 3, 0))
		for i = delay, 1, -1 do
	        button.Text = i
	        wait(1)
	    end
	    button.Text = button.Name
	    return(true)
	end
end

return module

I may not added all, but there is an example.

1 Like
#12

You are calling literally module.StringReturn()?

Because that function does not exist, you must use StringReturn1 to StringReturn8.

I also repeat that the code that i show you is incomplete, you have to put the missing functions.

#13

First of all, get set Misc and TPfloors to variables, it’ll clean things up a bit.

local misc = workspace:WaitForChild("Misc")
local tpFloors = misc:WaitForChild("TPfloors")

So you can do this:

local target1 = tpFloors:WaitForChild("TPsec7")
local target1a = tpFloors:WaitForChild("TPsec1")
-- ...

I am not the best with clean code myself, but ModuleScripts also clean things up a bit.

1 Like
#14

The best, and most organized way to go about doing this is to use a for loop through adding each to a table, and connecting whatever you have to from there on.

This resolves the whole cluster of target2,target3,target4,target5 and turns it into this instead (which isn’t as messy);

local tableName = {}
for _,selection in pairs(folder:GetChildren()) do
tableName[selection] = true
end

unless you really think this is less messy;

local target1 = workspace:WaitForChild("Misc"):WaitForChild("TPfloors"):WaitForChild("TPsec7")
local target1a = workspace:WaitForChild("Misc"):WaitForChild("TPfloors"):WaitForChild("TPsec1")
local target2 = workspace:WaitForChild("Misc"):WaitForChild("TPfloors"):WaitForChild("TPsec2")
local target3 = workspace:WaitForChild("Misc"):WaitForChild("TPfloors"):WaitForChild("TPsec3")
local target4 = workspace:WaitForChild("Misc"):WaitForChild("TPfloors"):WaitForChild("TPsec4")
local target5 = workspace:WaitForChild("Misc"):WaitForChild("TPfloors"):WaitForChild("TPsec5")
local target6 = workspace:WaitForChild("Misc"):WaitForChild("TPfloors"):WaitForChild("TPsec6")
local target8 = workspace:WaitForChild("Misc"):WaitForChild("TPfloors"):WaitForChild("TPsec8")
local target9 = workspace:WaitForChild("Misc"):WaitForChild("TPfloors"):WaitForChild("TPMC")
#15

This would make it hard to index individual objects on demand. It’s better you table.insert(tableName, selection) as that way you can just directly access table indices for the objects.

#16

I don’t see the problem, you can just index it by typing selection = tableName[selection]