How to optimize my code?

Help and Feedback Code Review
#1
  • the code is for upadeting a book ui
  • i am looking to make it faster + easier to read
local Frame = script.Parent
local Seciton1 = Frame.Seciton1
local Seciton2 = Frame.Seciton2

local NextButton = Frame.Next
local BackButton = Frame.Back
local FrameModle = script.Frame
local Page = Frame.Page
local rbx = "rbxassetid://"
local Pages = {
	Page1 = {Frame1 = {
		text = "جبالي اجا و سلمى",
		ImageId = rbx.."18222933997",
		Size = UDim2.new(0.944, 0,0.505, 0),
		Position = UDim2.new(0.015, 0,0.086, 0)
	},Frame2 = {
		text ="جديس",
		ImageId = rbx.."18222773535",
		Size = UDim2.new(0.944, 0,0.505, 0),
		Position = UDim2.new(0.066, 0,0.389, 0)
	}},
	Page2 = {Frame1 = {
		text = "جبالي اجا و سلمى2",
		ImageId = rbx.."18222933997",
		Size = UDim2.new(0.944, 0,0.505, 0),
		Position = UDim2.new(0.015, 0,0.086, 0)
	},Frame2 = {
		text ="جديس2",
		ImageId = rbx.."18222773535",
		Size = UDim2.new(0.944, 0,0.505, 0),
		Position = UDim2.new(0.066, 0,0.389, 0)
	}},
}
local legnthOfDictionary = function(dictionary)
	local HowManyItems = 0
	for i in pairs(dictionary) do
		HowManyItems += 1
	end
	return HowManyItems
end

local MaxPage = legnthOfDictionary(Pages)
local CurrentPage = 1
Page.Text  = "صفحه"..tostring(MaxPage).."/"..tostring(CurrentPage)
print(Page)
local Updateing = function(PageFrame,key,IsItFirstTime)
	print(PageFrame)
	Page.Text  = "صفحه"..tostring(MaxPage).."/"..tostring(CurrentPage)
	if Seciton1:FindFirstChild("Clone") and not IsItFirstTime then

		local Clone = Seciton1:FindFirstChild("Clone")
		print(Clone,"Edited?")
		Clone.Name = "Clone"
		Clone.Position =  PageFrame.Position
		Clone.Size = PageFrame.Size
		Clone:WaitForChild("image").Image = PageFrame.ImageId
		Clone:WaitForChild("Name").Text = PageFrame.text
		return
	elseif Seciton2:FindFirstChild("Clone") and not IsItFirstTime  then
		print("D")
		local Clone = Seciton2:FindFirstChild("Clone")

		Clone.Name = "Clone"
		Clone.Position =  PageFrame.Position
		Clone.Size = PageFrame.Size
		Clone:WaitForChild("image").Image = PageFrame.ImageId
		Clone:WaitForChild("Name").Text = PageFrame.text
		return
	end
	local Clone = FrameModle:Clone()
	Clone.Name = "Clone"
	Clone.Position =  PageFrame.Position
	Clone.Size = PageFrame.Size
	Clone:WaitForChild("image").Image = PageFrame.ImageId
	Clone:WaitForChild("Name").Text = PageFrame.text
	print(key)
	if key == 1 then
		Clone.Parent = Seciton1
	else
		Clone.Parent = Seciton2
	end
end
Updateing(Pages["Page1"].Frame1,1,true)
Updateing(Pages["Page1"].Frame2,2,true)

local NextBackFunction = function(Next)
	if Next then
		if CurrentPage ~= MaxPage then
			CurrentPage += 1
			local TargetPage = Pages["Page"..CurrentPage]
			for key,PageFrame in pairs(TargetPage) do
				if key == "Frame1" then
					Updateing(PageFrame,1,false)
				else
					Updateing(PageFrame,2,false)

				end
				
			end
		end
	else
		if CurrentPage ~= 1 then
			CurrentPage -= 1
			local TargetPage = Pages["Page"..CurrentPage]
			for key,PageFrame in pairs(TargetPage) do
				if key == "Frame1" then
					Updateing(PageFrame,1)
				else
					Updateing(PageFrame,2)

				end
			end
		end
		
	end
end

NextButton.MouseButton1Click:Connect(function()
	NextBackFunction(true)
end)
BackButton.MouseButton1Click:Connect(function()
	NextBackFunction(false)
end)
--for Key,Value in pairs(Pages) do
--	for _,FramePropites in pairs(Value) do
		
--	end
--end

image

1 Like
#2

i will be greatful of any answer ;D thanks

#3

In cases where if statement chains lead to code that is similar and mostly the same, generally it means that there is a better way to write that code where to reduce the copy and paste

For example the NextBackFunction(Next)

image
image1005×895 154 KB

As outlined in the, the only differences between the code are a clamping function and addition / subtraction. We can rewrite the function here to have the copied code only done once.

Note Added IsValidPage as a helper function to increase code clarity

local IsValidPage = function(Page)
	return Page < MaxPage and Page >= 1
end

local NextBackFunction = function(Next)
	local AddPages = Next and 1 or -1 -- lua logical operators (https://www.lua.org/pil/3.3.html)

	if not IsValidPage(CurrentPage + AddPages) then 
		return
	end

    -- this code is only done once now
	local TargetPage = Pages["Page"..CurrentPage]
	for key,PageFrame in pairs(TargetPage) do
		if key == "Frame1" then
			Updateing(PageFrame,1)
		else
			Updateing(PageFrame,2)
		end
	end
end

Similarly within this section, there is more copied and pasted code with only 1 change. You should use your if statement to change a variable that holds the item that gets cloned

image
image1411×1036 259 KB 

local Updateing = function(PageFrame,key,IsItFirstTime)
	print(PageFrame)
	Page.Text  = "صفحه"..tostring(MaxPage).."/"..tostring(CurrentPage)

	local SectionToClone = nil
	if Seciton1:FindFirstChild("Clone") and not IsItFirstTime then
		SectionToClone = Section1
	elseif Seciton2:FindFirstChild("Clone") and not IsItFirstTime  then
		SectionToClone = Section2
	else
		SectionToClone = FrameModle
	end

    -- this code is done only once now
	local Clone = SectionToClone:FindFirstChild("Clone")
	Clone.Name = "Clone"
	Clone.Position =  PageFrame.Position
	Clone.Size = PageFrame.Size
	Clone:WaitForChild("image").Image = PageFrame.ImageId
	Clone:WaitForChild("Name").Text = PageFrame.text

	if SectionToClone ~= Section1 and SectionToClone ~= Section2 then
		if key == 1 then
			Clone.Parent = Seciton1
		else
			Clone.Parent = Seciton2
		end
	end
end
2 Likes
#4

Thanks Bro it helps a lot ,
it a bit confusing for me for now but i will add them and try to understand it ;D