[Solved] Simplifying/shortening functions [Solved]

Help and Feedback Code Review
1

Hello everyone!

Is there any way to reasonably shorten/simplify this code?

image
image749×346 19.7 KB

Code to copy & paste 

local XYZ = {"X","Y","Z"} -- used a bunch of times elsewhere, hence why it's its own local variable

local function createDictionaryFromAdjustOffsetFrame(AdjustOffsetFrame:Frame):{["XYZ"]:string}
	
	local dictionary = {}

	for _, xyz in XYZ do	
		dictionary[xyz] = AdjustOffsetFrame[xyz].TextBox.Text
	end
	
	return dictionary
	
end

local function inputAdjustOffsetFrameReturnVector3(AdjustOffsetFrame:Frame):Vector3
	local dictionary = createDictionaryFromAdjustOffsetFrame(AdjustOffsetFrame)
	return Vector3.new(dictionary["X"],dictionary["Y"],dictionary["Z"])
end

local function inputAdjustOffsetFrameReturnCFrameWithNoRotation(AdjustOffsetFrame:Frame):CFrame
	local dictionary = createDictionaryFromAdjustOffsetFrame(AdjustOffsetFrame)
	return CFrame.new(dictionary["X"],dictionary["Y"],dictionary["Z"])
end

local function inputAdjustOffsetFrameReturnCFrameAngles(AdjustOffsetFrame:Frame):CFrame
	local dictionary = createDictionaryFromAdjustOffsetFrame(AdjustOffsetFrame)
	return CFrame.Angles(math.rad(dictionary["X"]),math.rad(dictionary["Y"]),math.rad(dictionary["Z"]))
end
Screenshots of where the code is used

The first function is only used inside the other three functions.

image
image
image
image
image

This is everywhere these functions appear so far, might be more to come.

Thanks in advance!

2 Likes
2

If you have no other use for the dictionary created you can just do

Vector.new(Function(Frame))

Where Function returns X,Y,Z. Although you would need to make some changes from the Angles part

3

why did ya use string indexing?

local function inputAdjustOffsetFrameReturnVector3(AdjustOffsetFrame:Frame):Vector3
	local dictionary = createDictionaryFromAdjustOffsetFrame(AdjustOffsetFrame)
	return Vector3.new(dictionary.X,dictionary.Y,dictionary.Z)
end
4

Just a habit. You’re correct, the way you’ve done it works as well.

5

So basically this?

local XYZ = {"X","Y","Z"} -- used a bunch of times elsewhere, hence why it's its own local variable

local function createDictionaryFromAdjustOffsetFrame(AdjustOffsetFrame:Frame, toRad:boolean)
	
	local dictionary = {}

	for _, xyz in XYZ do	
		dictionary[xyz] = AdjustOffsetFrame[xyz].TextBox.Text
	end
	
	if toRad == true then
		return math.rad(dictionary.X), math.rad(dictionary.Y), math.rad(dictionary.Z)
	else
		return dictionary.X, dictionary.Y, dictionary.Z
	end

end

local function inputAdjustOffsetFrameReturnVector3(AdjustOffsetFrame:Frame):Vector3
	return Vector3.new(createDictionaryFromAdjustOffsetFrame(AdjustOffsetFrame))
end

local function inputAdjustOffsetFrameReturnCFrameWithNoRotation(AdjustOffsetFrame:Frame):CFrame
	return CFrame.new(createDictionaryFromAdjustOffsetFrame(AdjustOffsetFrame))
end

local function inputAdjustOffsetFrameReturnCFrameAngles(AdjustOffsetFrame:Frame):CFrame
	return CFrame.Angles(createDictionaryFromAdjustOffsetFrame(AdjustOffsetFrame), true)
end
6

Yea that is fine, I would personally name the functions something shorter but that’s up to you. Apart from that this is fine.

1 Like
7

Using a loop for XYZ, maybe that gets unrolled I hope…
Dictionary is still pointless.

local function extractXYZFromAdjustOffsetFrame(AdjustOffsetFrame:Frame):(number, number, number)
	return AdjustOffsetFrame.X.TextBox.Text, AdjustOffsetFrame.Y.TextBox.Text, AdjustOffsetFrame.Z.TextBox.Text
end

Faster and oneliner :v
toRad part will just be an if statement on top of that.

8

So, something like this?

local function extractXYZFromAdjustOffsetFrame(AdjustOffsetFrame:Frame, toRad:boolean):(number, number, number)

	if toRad == true then
		return math.rad(AdjustOffsetFrame.X.TextBox.Text), math.rad(AdjustOffsetFrame.Y.TextBox.Text), math.rad(AdjustOffsetFrame.Z.TextBox.Text)
	else
		return AdjustOffsetFrame.X.TextBox.Text, AdjustOffsetFrame.Y.TextBox.Text, AdjustOffsetFrame.Z.TextBox.Text
	end

end

Not sure man… kinda hard to look at :sweat_smile:
I will keep the name and the (number, number, number) though :D

9

Well, you can adjust the spacing or do some other formatting changes, but this is really the most compact I believe it will get without shortening the names or doing some weird hacks like storing math.rad in a local.

You can remove == true from the if statement since the type hints assume it will never be anything but a boolean.

Actually, if we want to be pedantic, the return type might not need to be explicitly stated since it can be inferred from your code.

1 Like
10

Yo, so I was trying to tweak your code, and was considering that you simply store the (what I assume) coordinate values from the TextBox Texts. With that, you can simply table.unpack whatever you return:

local XYZ = {"X", "Y", "Z"} -- Specify order here

local function createListFromAdjustOffsetFrame(AdjustOffsetFrame:Frame):{string}
	local list = table.create(3)

	for _, xyz in XYZ do	
		table.insert(list, tonumber(AdjustOffsetFrame[xyz].TextBox.Text))
	end

	return list
end

local function inputAdjustOffsetFrameReturnVector3(AdjustOffsetFrame:Frame):vector
	return vector.create(table.unpack(createListFromAdjustOffsetFrame(AdjustOffsetFrame)))
end

print(inputAdjustOffsetFrameReturnVector3(script.Frame)) -- I just used this to test

I also recommend vector.create instead of Vector3.new, but vector.create causes typechecking errors; they produce the same result though..?

image
image573×143 7.01 KB

image

Just a note: using the boolean toRad in the above replies would increase cyclomatic complexity and future confusion of how your code works